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

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

[Shameer Kolothum]

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.

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;
+		}
 
 		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;
 		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),
+						&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;
 }
 
 long hugetlb_unreserve_pages(struct inode *inode, long start, long end,
-- 
2.43.0

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

[Joshua Hahn]

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.

> +		}
>  
>  		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.

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;
}

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.

I hope all of my comments make sense. Please feel free to correct me if
I've misunderstood any of the code or your intent with the patch as well : -)
Have a great day!

Joshua

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

[Shameer Kolothum]

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

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

[Oscar Salvador]

On Wed, Oct 22, 2025 at 11:29:56AM +0100, Shameer Kolothum 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.
> 
> 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;

Why is this one EINVAL? Should not this be ENOMEM?


-- 
Oscar Salvador
SUSE Labs

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

[Shameer Kolothum]

> -----Original Message-----
> From: Oscar Salvador <osalvador@suse.de>
> Sent: 25 November 2025 09:34
> To: Shameer Kolothum <skolothumtho@nvidia.com>
> Cc: linux-mm@kvack.org; linux-kernel@vger.kernel.org;
> muchun.song@linux.dev; 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, Oct 22, 2025 at 11:29:56AM +0100, Shameer Kolothum 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.
> >
> > 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;
> 
> Why is this one EINVAL? Should not this be ENOMEM?

Yes, looking at it again it should be ENOMEM. I will change that and
send out a v3 soon.

Btw, there is a v2 of the patch here,
https://lore.kernel.org/linux-mm/20251024094240.337630-1-skolothumtho@nvidia.com/

Please take a look if you haven't already.

Thanks,
Shameer