RE: [PATCH] mm/hugetlb: Fix incorrect error return from hugetlb_reserve_pages()

[Shameer Kolothum <skolothumtho@nvidia.com>]

Hi Joshua,

Thanks for taking a look.

> -----Original Message-----
> From: Joshua Hahn <joshua.hahnjy@gmail.com>
> Sent: 22 October 2025 15:39
> To: Shameer Kolothum <skolothumtho@nvidia.com>
> Cc: linux-mm@kvack.org; linux-kernel@vger.kernel.org;
> muchun.song@linux.dev; osalvador@suse.de; vivek.kasireddy@intel.com;
> Jason Gunthorpe <jgg@nvidia.com>; Nicolin Chen <nicolinc@nvidia.com>;
> Nathan Chen <nathanc@nvidia.com>; Matt Ochs <mochs@nvidia.com>
> Subject: Re: [PATCH] mm/hugetlb: Fix incorrect error return from
> hugetlb_reserve_pages()
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, 22 Oct 2025 11:29:56 +0100 Shameer Kolothum
> <skolothumtho@nvidia.com> wrote:
> 
> > The function hugetlb_reserve_pages() returns the number of pages added
> > to the reservation map on success and a negative error code on failure
> > (e.g. -EINVAL, -ENOMEM). However, in some error paths, it may return -1
> > directly.
> >
> > For example, a failure at:
> >
> >     if (hugetlb_acct_memory(h, gbl_reserve) < 0)
> >         goto out_put_pages;
> >
> > results in returning -1 (since add = -1), which may be misinterpreted
> > in userspace as -EPERM.
> >
> > Fix this by explicitly capturing and propagating the return values from
> > helper functions, and using -EINVAL for all other failure cases.
> 
> Hello Shameer,
> 
> Thank you for the patch! The goal of this patch makes a lot of sense to me.
> I just have a few comments:
> 
> > Fixes: 986f5f2b4be3 ("mm/hugetlb: make hugetlb_reserve_pages() return
> nr of entries updated")
> > Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
> > ---
> >  mm/hugetlb.c | 25 ++++++++++++++++++-------
> >  1 file changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 795ee393eac0..1767f7599f91 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -7269,6 +7269,7 @@ long hugetlb_reserve_pages(struct inode *inode,
> >       struct resv_map *resv_map;
> >       struct hugetlb_cgroup *h_cg = NULL;
> >       long gbl_reserve, regions_needed = 0;
> > +     int ret;
> >
> >       /* This should never happen */
> >       if (from > to) {
> > @@ -7308,8 +7309,10 @@ long hugetlb_reserve_pages(struct inode
> *inode,
> >       } else {
> >               /* Private mapping. */
> >               resv_map = resv_map_alloc();
> > -             if (!resv_map)
> > +             if (!resv_map) {
> > +                     ret = -EINVAL;
> >                       goto out_err;
> 
> NIT: What if we just initialize ret = -EINVAL in the declaration? Then we
> can just preserve the original code and goto out_err here.

I personally think assigning the error code makes it more readable with what err
we are returning.

> > +             }
> >
> >               chg = to - from;
> >
> > @@ -7317,11 +7320,15 @@ long hugetlb_reserve_pages(struct inode
> *inode,
> >               set_vma_resv_flags(vma, HPAGE_RESV_OWNER);
> >       }
> >
> > -     if (chg < 0)
> > +     if (chg < 0) {
> > +             ret = -EINVAL;
> 
> Is there a reason we set ret = -EINVAL here? I think it makes sense to
> preserve what chg would have been returned here. There are two ways this
> value
> gets set; for shared mappings, we have chg = region_chg(), and for private
> mappings we have chg = to - from.

Ah..I missed that region_chg().

> For shared mappings, region_chg() may return -ENOMEM, and I think this is
> something that we would like to propagate, as the commit message of this
> patch suggests. For private mappings, it does make sense to set it to
> -EINVAL, since we don't want to return a random negative value there.
> 
> So maybe something like this? (Including the initialization of ret to -EINVAL
> from above)
> 
> if (chg < 0) {
>         if (chg == -ENOMEM)
>                 ret = -ENOMEM;
>         goto out_err;
> }

Makes sense. I will update.
> 
> I'm sure there is also a more elegant way to handle this : -)
> 
> Although, there is a rare case that region_chg returns -ENOMEM because
> add_reservation_in_range returns a negative value equal to -ENOMEM. In that
> case we want to overwrite it with -EINVAL but will return -ENOMEM instead.
> 
> But this seems too rare : -) Maybe it makes sense to change region_chg or
> add_reservation_in_range to check for a negative chg, and return -EINVAL
> there instead as well.
> 
> >               goto out_err;
> > +     }
> >
> > -     if (hugetlb_cgroup_charge_cgroup_rsvd(hstate_index(h),
> > -                             chg * pages_per_huge_page(h), &h_cg) < 0)
> > +     ret = hugetlb_cgroup_charge_cgroup_rsvd(hstate_index(h),
> > +                                             chg * pages_per_huge_page(h),
> 
> NIT: Should we just unindent this line once and include &h_cg on the same
> line? It seems like the original line also pushed the chg * pages... to
> before the open parenthesis in the first line anyways.
> 
> > +                                             &h_cg);
> > +     if (ret < 0)
> >               goto out_err;
> >
> >       if (vma && !(vma->vm_flags & VM_MAYSHARE) && h_cg) {
> > @@ -7337,14 +7344,17 @@ long hugetlb_reserve_pages(struct inode
> *inode,
> >        * reservations already in place (gbl_reserve).
> >        */
> >       gbl_reserve = hugepage_subpool_get_pages(spool, chg);
> > -     if (gbl_reserve < 0)
> > +     if (gbl_reserve < 0) {
> > +             ret = gbl_reserve;
> >               goto out_uncharge_cgroup;
> > +     }
> >
> >       /*
> >        * Check enough hugepages are available for the reservation.
> >        * Hand the pages back to the subpool if there are not
> >        */
> > -     if (hugetlb_acct_memory(h, gbl_reserve) < 0)
> > +     ret = hugetlb_acct_memory(h, gbl_reserve);
> > +     if (ret < 0)
> >               goto out_put_pages;
> >
> >       /*
> > @@ -7363,6 +7373,7 @@ long hugetlb_reserve_pages(struct inode *inode,
> >
> >               if (unlikely(add < 0)) {
> >                       hugetlb_acct_memory(h, -gbl_reserve);
> > +                     ret = -EINVAL;
> >                       goto out_put_pages;
> >               } else if (unlikely(chg > add)) {
> >                       /*
> > @@ -7423,7 +7434,7 @@ long hugetlb_reserve_pages(struct inode *inode,
> >               kref_put(&resv_map->refs, resv_map_release);
> >               set_vma_resv_map(vma, NULL);
> >       }
> > -     return chg < 0 ? chg : add < 0 ? add : -EINVAL;
> > +     return ret;
> 
> We only return ret in the error case. If I understand the function correctly,
> this part is only reached if there is an error somewhere. Should we rename
> the 'ret' variable to 'err' instead? I think that will actually
> add some clarity to this code path, since it's not immediately obvious that
> this portion is just error handling & propagation otherwise.

Ok. I will update.

Thanks,
Shameer