| 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: <3ec2bd80-50e0-4256-a34b-dd44bdc1ae31@lucifer.local>
Date: Fri, 13 Jun 2025 06:18:07 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Baolin Wang <baolin.wang@...ux.alibaba.com>
Cc: David Hildenbrand <david@...hat.com>, akpm@...ux-foundation.org,
hughd@...gle.com, Liam.Howlett@...cle.com, npache@...hat.com,
ryan.roberts@....com, dev.jain@....com, ziy@...dia.com,
linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] mm: huge_memory: disallow hugepages if the
system-wide THP sysfs settings are disabled
On Fri, Jun 13, 2025 at 10:07:20AM +0800, Baolin Wang wrote:
>
>
> On 2025/6/12 22:49, Lorenzo Stoakes wrote:
> > On Thu, Jun 12, 2025 at 04:09:27PM +0200, David Hildenbrand wrote:
> > >
> > >
> > > > > @@ -265,6 +265,42 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma,
> > > > > unsigned long tva_flags,
> > > > > unsigned long orders);
> > > > > +/* Strictly mask requested anonymous orders according to sysfs settings. */
> > > > > +static inline unsigned long __thp_mask_anon_orders(unsigned long vm_flags,
> > > > > + unsigned long tva_flags, unsigned long orders)
> > > > > +{
> > > > > + const unsigned long always = READ_ONCE(huge_anon_orders_always);
> > > > > + const unsigned long madvise = READ_ONCE(huge_anon_orders_madvise);
> > > > > + const unsigned long inherit = READ_ONCE(huge_anon_orders_inherit);
> > > > > + const unsigned long never = ~(always | madvise | inherit);
> > > > > +
> > > > > + /* Disallow orders that are set to NEVER directly ... */
> > > > > + orders &= ~never;
> > > > > +
> > > > > + /* ... or through inheritance (global == NEVER). */
> > > > > + if (!hugepage_global_enabled())
> > > > > + orders &= ~inherit;
> > > > > +
> > > > > + /*
> > > > > + * Otherwise, we only enforce sysfs settings if asked. In addition,
> > > > > + * if the user sets a sysfs mode of madvise and if TVA_ENFORCE_SYSFS
> > > > > + * is not set, we don't bother checking whether the VMA has VM_HUGEPAGE
> > > > > + * set.
> > > > > + */
> > > > > + if (!(tva_flags & TVA_ENFORCE_SYSFS))
> > > > > + return orders;
> > > >
> > > > This implicitly does a & mask as per suggested previous version, which I think
> > > > is correct but worth pointing out.
> > >
> > > Yes.
> > >
> > > >
> > > > > +
> > > > > + if (!(vm_flags & VM_HUGEPAGE)) {
> > > >
> > > > Don't love this sort of mega negation here. I read this as _does_ have huge
> > > > page...
> > >
> > > Well, it's very common to do that, but not objecting to something that is
> > > clearer ;)
> > >
> > > I assume you spotted the
> > >
> > > if (!(tva_flags & TVA_ENFORCE_SYSFS))
> > >
> > > :P
> >
> > Lol yeah I know I know, I just think I guess in this case because you're
> > negating elsewhere it makes it harder...
> >
> > >
> > > if (vm_flags & VM_HUGEPAGE)
> > > return orders;
> > >
> > >
> > > Would have been easier.
> > >
> > > >
> > > > > + /* Disallow orders that are set to MADVISE directly ... */
> > > > > + orders &= ~madvise;
> > > > > +
> > > > > + /* ... or through inheritance (global == MADVISE). */
> > > > > + if (!hugepage_global_always())
> > > > > + orders &= ~inherit;
> > > >
> > > > I hate this implicit 'not hugepage global always so this means either never or
> > > > madvise and since we cleared orders for never this means madvise' mental
> > > > gymnastics required here.
> > > >
> > > > Yeah I feel this is a bridge too far, we're getting into double negation and I
> > > > think that's more confusiong.
> > >
> > >
> > > Same here ... I think we should just have hugepage_global_madvise(). :)
> >
> > Ideally in future not have these stupid globals all over the place and rework
> > this whole damn thing...
> >
> > >
> > > >
> > > >
> > > > > + }
> > > >
> > > > I propose a compromise as I rather like your 'exclude never' negation bit.
> > > >
> > > > So:
> > > >
> > > > /* Strictly mask requested anonymous orders according to sysfs settings. */
> > > > static inline unsigned long __thp_mask_anon_orders(unsigned long vm_flags,
> > > > unsigned long tva_flags, unsigned long orders)
> > > > {
> > > > const unsigned long always = READ_ONCE(huge_anon_orders_always);
> > > > const unsigned long madvise = READ_ONCE(huge_anon_orders_madvise);
> > > > const unsigned long inherit = READ_ONCE(huge_anon_orders_inherit);;
> > > > const unsigned long never = ~(always | madvise | inherit);
> > > > const bool inherit_enabled = hugepage_global_enabled();
> > >
> > > Can we just have hugepage_global_never/disabled() to use instead?
> >
> > This would be nice!
> >
> > Could be a follow up... though again would be nice to somehow do away with all
> > this crap altogether.
> >
> > >
> > > >
> > > > /* Disallow orders that are set to NEVER directly ... */
> > > > orders &= ~never;
> > > >
> > > > /* ... or through inheritance (global == NEVER). */
> > > > if (!inherit_enabled)
> > > > orders &= ~inherit;>
> > > > /*
> > > > * Otherwise, we only enforce sysfs settings if asked. In addition,
> > > > * if the user sets a sysfs mode of madvise and if TVA_ENFORCE_SYSFS
> > > > * is not set, we don't bother checking whether the VMA has VM_HUGEPAGE
> > > > * set.
> > > > */
> > > > if (!(tva_flags & TVA_ENFORCE_SYSFS))
> > > > return orders;
> > > >
> > > > if (hugepage_global_always())
> > > > return orders & (always | inherit);
> > > >
> > > > /* We already excluded never inherit above. */
> > > > if (vm_flags & VM_HUGEPAGE)
> > > > return orders & (always | madvise | inherit);
> > > >
> > > > return orders & always;
> > > > }
> > > >
> > > > What do you think?
> > >
> > > With the fixup, it would work for me. No magical "mask" variables :D
> >
> > Thanks!
>
> Fair enough. You both prefer the 'exclude never' logic, and I will follow
> that logic.
Appreciate it, sorry to be a pain and thanks a lot!
>
> > > > > + return orders;
> > > > > +}
> > > > > +
> > > > > /**
> > > > > * thp_vma_allowable_orders - determine hugepage orders that are allowed for vma
> > > > > * @vma: the vm area to check
> > > > > @@ -287,16 +323,8 @@ unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
> > > > > unsigned long orders)
> > > > > {
> > > > > /* Optimization to check if required orders are enabled early. */
> > > > > - if ((tva_flags & TVA_ENFORCE_SYSFS) && vma_is_anonymous(vma)) {
> > > > > - unsigned long mask = READ_ONCE(huge_anon_orders_always);
> > > > > -
> > > > > - if (vm_flags & VM_HUGEPAGE)
> > > > > - mask |= READ_ONCE(huge_anon_orders_madvise);
> > > > > - if (hugepage_global_always() ||
> > > > > - ((vm_flags & VM_HUGEPAGE) && hugepage_global_enabled()))
> > > > > - mask |= READ_ONCE(huge_anon_orders_inherit);
> > > > > -
> > > > > - orders &= mask;
> > > > > + if (vma_is_anonymous(vma)) {
> > > > > + orders = __thp_mask_anon_orders(vm_flags, tva_flags, orders);
> > > > > if (!orders)
> > > > > return 0;
> > > >
> > > > I pointed out to Baolin that __thp_vma_allowable_orders() handles the orders ==
> > > > 0 case almost immediately so there's no need to do this, it just makes the code
> > > > noisier.
> > >
> > > The reason we added it in the first place was to not do the (expensive)
> > > function call.
> >
> > Ack point taken!
> >
> > >
> > >
> > > --
> > > Cheers,
> > >
> > > David / dhildenb
> > >
> >
> > For convenience, I enclose the fixed version + tweaked the inherit local bool to
> > be inherit_never to be clearer:
> >
> > /* Strictly mask requested anonymous orders according to sysfs settings. */
> > static inline unsigned long __thp_mask_anon_orders(unsigned long vm_flags,
> > unsigned long tva_flags, unsigned long orders)
> > {
> > const unsigned long always = READ_ONCE(huge_anon_orders_always);
> > const unsigned long madvise = READ_ONCE(huge_anon_orders_madvise);
> > const unsigned long inherit = READ_ONCE(huge_anon_orders_inherit);;
> > const unsigned long never = ~(always | madvise | inherit);
> > const bool inherit_never = !hugepage_global_enabled();
> >
> > /* Disallow orders that are set to NEVER directly ... */
> > orders &= ~never;
> >
> > /* ... or through inheritance (global == NEVER). */
> > if (inherit_never)
> > orders &= ~inherit;
> >
> > /*
> > * Otherwise, we only enforce sysfs settings if asked. In addition,
> > * if the user sets a sysfs mode of madvise and if TVA_ENFORCE_SYSFS
> > * is not set, we don't bother checking whether the VMA has VM_HUGEPAGE
> > * set.
> > */
> > if (!(tva_flags & TVA_ENFORCE_SYSFS))
> > return orders;
> >
> > /* We already excluded never inherit above. */
> > if (vm_flags & VM_HUGEPAGE)
> > return orders & (always | madvise | inherit);
> >
> > if (hugepage_global_always())
> > return orders & (always | inherit);
> >
> > return orders & always;
> > }
>
> Thanks Lorenzo. Let me follow this logic and do some testing.
Thanks, much appreciated!
Powered by blists - more mailing lists