mm/hugetlb.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
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.0On 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
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
© 2016 - 2026 Red Hat, Inc.