Message-ID: <20251022143906.2780172-1-joshua.hahnjy@gmail.com>
Date: Wed, 22 Oct 2025 07:39:05 -0700
From: Joshua Hahn <joshua.hahnjy@...il.com>
To: Shameer Kolothum <skolothumtho@...dia.com>
Cc: linux-mm@...ck.org,
	linux-kernel@...r.kernel.org,
	muchun.song@...ux.dev,
	osalvador@...e.de,
	vivek.kasireddy@...el.com,
	jgg@...dia.com,
	nicolinc@...dia.com,
	nathanc@...dia.com,
	mochs@...dia.com
Subject: Re: [PATCH] mm/hugetlb: Fix incorrect error return from hugetlb_reserve_pages()

On Wed, 22 Oct 2025 11:29:56 +0100 Shameer Kolothum <skolothumtho@...dia.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@...dia.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

Powered by blists - more mailing lists