Re: [PATCH v3 1/5] mm: fix off-by-one error in VMA count limit checks

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* Hugh Dickins <hughd@google.com> [251014 02:28]:
> On Mon, 13 Oct 2025, Kalesh Singh wrote:
> 
> > The VMA count limit check in do_mmap() and do_brk_flags() uses a
> > strict inequality (>), which allows a process's VMA count to exceed
> > the configured sysctl_max_map_count limit by one.

...

> >  	/* Too many mappings? */
> > -	if (mm->map_count > sysctl_max_map_count)
> > +	if (mm->map_count >= sysctl_max_map_count)
> >  		return -ENOMEM;
> >  
> >  	/*
> > diff --git a/mm/vma.c b/mm/vma.c
> > index a2e1ae954662..fba68f13e628 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -2797,7 +2797,7 @@ int do_brk_flags(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >  	if (!may_expand_vm(mm, vm_flags, len >> PAGE_SHIFT))
> >  		return -ENOMEM;
> >  
> > -	if (mm->map_count > sysctl_max_map_count)
> > +	if (mm->map_count >= sysctl_max_map_count)
> >  		return -ENOMEM;
> >  
> >  	if (security_vm_enough_memory_mm(mm, len >> PAGE_SHIFT))
> > -- 
> > 2.51.0.760.g7b8bcc2412-goog
> 
> Sorry for letting you go so far before speaking up (I had to test what
> I believed to be true, and had hoped that meanwhile one of your many
> illustrious reviewers would say so first, but no): it's a NAK from me.
> 
> These are not off-by-ones: at the point of these checks, it is not
> known whether an additional map/vma will have to be added, or the
> addition will be merged into an existing map/vma.  So the checks
> err on the lenient side, letting you get perhaps one more than the
> sysctl said, but not allowing any more than that.
> 
> Which is all that matters, isn't it? Limiting unrestrained growth.
> 
> In this patch you're proposing to change it from erring on the
> lenient side to erring on the strict side - prohibiting merges
> at the limit which have been allowed for many years.
> 
> Whatever one thinks about the merits of erring on the lenient versus
> erring on the strict side, I see no reason to make this change now,
> and most certainly not with a Fixes Cc: stable. There is no danger
> in the current behaviour; there is danger in prohibiting what was
> allowed before.

Thanks Hugh.

I'm left wondering if the issue is that we are checking in the wrong
location.  That is, should we be checking so early in the process or
later when we know the count adjustment?

But then again, later we may be in mid-operation and find out we're out
of room.  Other places are even more lenient and allow us to exceed the
count for a potential limited time, and we really don't know what's
going to happen until we examine what already exists.. So it seems like
a lot of effort to avoid one extra vma.

> 
> As to the remainder of your series: I have to commend you for doing
> a thorough and well-presented job, but I cannot myself see the point in
> changing 21 files for what almost amounts to a max_map_count subsystem.
> I call it misdirected effort, not at all to my taste, which prefers the
> straightforward checks already there; but accept that my taste may be
> out of fashion, so won't stand in the way if others think it worthwhile.

I'm not sure which way I favour, it does seem like a large change to
avoid an issue that never existed.

In either case, it seems like a good idea to adjust the comments to
state that the count may not change.

Thanks,
Liam