linux-kernel - Re: [PATCH 1/5] prctl: extend PR_SET_THP_DISABLE to optionally exclude VM

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <41d8154f-7646-4cca-8b65-218827c1e7e4@lucifer.local>
Date: Thu, 31 Jul 2025 09:29:12 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Usama Arif <usamaarif642@...il.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, david@...hat.com,
        linux-mm@...ck.org, linux-fsdevel@...r.kernel.org, corbet@....net,
        rppt@...nel.org, surenb@...gle.com, mhocko@...e.com,
        hannes@...xchg.org, baohua@...nel.org, shakeel.butt@...ux.dev,
        riel@...riel.com, ziy@...dia.com, laoar.shao@...il.com,
        dev.jain@....com, baolin.wang@...ux.alibaba.com, npache@...hat.com,
        Liam.Howlett@...cle.com, ryan.roberts@....com, vbabka@...e.cz,
        jannh@...gle.com, Arnd Bergmann <arnd@...db.de>, sj@...nel.org,
        linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
        kernel-team@...a.com, Matthew Wilcox <willy@...radead.org>
Subject: Re: [PATCH 1/5] prctl: extend PR_SET_THP_DISABLE to optionally
 exclude VM_HUGEPAGE

Just a ping on the man page stuff - you will do that right? :>)

Probably best at end of 6.18 cycle when this is about to go to Linus.

I can give you some details on how that all works if it'd be helpful.

On Wed, Jul 30, 2025 at 08:42:04PM +0100, Usama Arif wrote:
> >> Acked-by: Usama Arif <usamaarif642@...il.com>
> >> Tested-by: Usama Arif <usamaarif642@...il.com>
[snip]
> >> Signed-off-by: David Hildenbrand <david@...hat.com>

All looks good aside from nits, so:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>

> >> +/*
> >> + * Check whether THPs are explicitly disabled for this VMA, for example,
> >> + * through madvise or prctl.
> >> + */
> >>  static inline bool vma_thp_disabled(struct vm_area_struct *vma,
> >>  		vm_flags_t vm_flags)
> >>  {
> >> +	/* Are THPs disabled for this VMA? */
> >> +	if (vm_flags & VM_NOHUGEPAGE)
> >> +		return true;
>
> VM_NOHUGEPAGE will cause the THP being disabled here.
>
> >> +	/* Are THPs disabled for all VMAs in the whole process? */
> >> +	if (test_bit(MMF_DISABLE_THP_COMPLETELY, &vma->vm_mm->flags))
> >> +		return true;
> >>  	/*
> >> -	 * Explicitly disabled through madvise or prctl, or some
> >> -	 * architectures may disable THP for some mappings, for
> >> -	 * example, s390 kvm.
> >> +	 * Are THPs disabled only for VMAs where we didn't get an explicit
> >> +	 * advise to use them?
> >>  	 */
> >> -	return (vm_flags & VM_NOHUGEPAGE) ||
> >> -	       test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags);
> >> +	if (vm_flags & VM_HUGEPAGE)
> >> +		return false;
> >
> > Hm is this correct? This means that VM_NOHUGEPAGE no longer results in THP being
> > disabled here no?
>
> Lorenzo, pointed to VM_NOHUGEPAGE case above.>

Haha doh! OK cool, obviously I did this review a little too late in the day
when Mr. Brain was not on best form :P

All good, thanks!

And of course this now enforces the 'except advised' logic below it.

> >> +/*
> >> + * Don't disable THPs when explicitly advised (e.g., MADV_HUGEPAGE /
> >> + * VM_HUGEPAGE).
> >> + */
> >> +# define PR_THP_DISABLE_EXCEPT_ADVISED	(1 << 1)
> >
> > NO space after # please.
> >
>
> I think this is following the file convention, the space is there in other flags
> all over this file. I dont like the space as well.

Yeah yuck. It's not a big deal, but ideally I'd prefer us to be sane even
if the rest of the header is less so here.

>
> >>  #define PR_GET_THP_DISABLE	42
> >>
> >>  /*
> >> diff --git a/kernel/sys.c b/kernel/sys.c
> >> index b153fb345ada..b87d0acaab0b 100644
> >> --- a/kernel/sys.c
> >> +++ b/kernel/sys.c
> >> @@ -2423,6 +2423,50 @@ static int prctl_get_auxv(void __user *addr, unsigned long len)
> >>  	return sizeof(mm->saved_auxv);
> >>  }
> >>
> >> +static int prctl_get_thp_disable(unsigned long arg2, unsigned long arg3,
> >> +				 unsigned long arg4, unsigned long arg5)
> >> +{
> >> +	unsigned long *mm_flags = &current->mm->flags;
> >> +
> >> +	if (arg2 || arg3 || arg4 || arg5)
> >> +		return -EINVAL;
> >> +
> >
> > Can we have a comment here about what we're doing below re: the return
> > value?
> >
>
> Do you mean add returning 1 for MMF_DISABLE_THP_COMPLETELY and 3 for MMF_DISABLE_THP_EXCEPT_ADVISED?

Well more so something about we return essentially flags indicating what is
enabled or not, if bit 0 is set then it's disabled, if bit 1 is set then
it's that with the exception of VM_HUGEPAGE VMAs.

>
> I will add it in next revision.

Thanks!