| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<CH3PR12MB754879F2AD53D8317BA13AB1ABF1A@CH3PR12MB7548.namprd12.prod.outlook.com>
Date: Fri, 24 Oct 2025 09:02:17 +0000
From: Shameer Kolothum <skolothumtho@...dia.com>
To: Joshua Hahn <joshua.hahnjy@...il.com>
CC: "linux-mm@...ck.org" <linux-mm@...ck.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "muchun.song@...ux.dev"
<muchun.song@...ux.dev>, "osalvador@...e.de" <osalvador@...e.de>,
"vivek.kasireddy@...el.com" <vivek.kasireddy@...el.com>, Jason Gunthorpe
<jgg@...dia.com>, Nicolin Chen <nicolinc@...dia.com>, Nathan Chen
<nathanc@...dia.com>, Matt Ochs <mochs@...dia.com>
Subject: RE: [PATCH] mm/hugetlb: Fix incorrect error return from
hugetlb_reserve_pages()
Hi Joshua,
Thanks for taking a look.
> -----Original Message-----
> From: Joshua Hahn <joshua.hahnjy@...il.com>
> Sent: 22 October 2025 15:39
> 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;
> Jason Gunthorpe <jgg@...dia.com>; Nicolin Chen <nicolinc@...dia.com>;
> Nathan Chen <nathanc@...dia.com>; Matt Ochs <mochs@...dia.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@...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.
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
Powered by blists - more mailing lists