linux-kernel - Re: [PATCH v4] mm: Override mTHP "enabled" defaults at kernel cmdline

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: <6e23c705-3d67-419b-b085-f19f5101124c@redhat.com>
Date: Thu, 15 Aug 2024 12:26:37 +0200
From: David Hildenbrand <david@...hat.com>
To: Barry Song <21cnbao@...il.com>
Cc: akpm@...ux-foundation.org, baohua@...nel.org,
 baolin.wang@...ux.alibaba.com, corbet@....net, ioworker0@...il.com,
 linux-kernel@...r.kernel.org, linux-mm@...ck.org, ryan.roberts@....com,
 v-songbaohua@...o.com
Subject: Re: [PATCH v4] mm: Override mTHP "enabled" defaults at kernel cmdline

>>> +static inline int get_order_from_str(const char *size_str)
>>> +{
>>> +     unsigned long size;
>>> +     char *endptr;
>>> +     int order;
>>> +
>>> +     size = memparse(size_str, &endptr);
>>
>> Do we have to also test if is_power_of_2(), and refuse if not? For
>> example, what if someone would pass 3K, would the existing check catch it?
> 
> no, the existing check can't catch it.
> 
> I passed thp_anon=15K-64K:always, then I got 16K enabled:
> 
> / # cat /sys/kernel/mm/transparent_hugepage/hugepages-16kB/enabled
> [always] inherit madvise never
> 

Okay, so we should document then that start/end of the range must be 
valid THP sizes.

> I can actually check that by:
> 
> static inline int get_order_from_str(const char *size_str)
> {
> 	unsigned long size;
> 	char *endptr;
> 	int order;
> 
> 	size = memparse(size_str, &endptr);
> 
> 	if (!is_power_of_2(size >> PAGE_SHIFT))

No need for the shift.

if (!is_power_of_2(size))

Is likely even more correct if someone would manage to pass something 
stupid like

16385 (16K + 1)

> 		goto err;
> 	order = get_order(size);
> 	if ((1 << order) & ~THP_ORDERS_ALL_ANON)
> 		goto err;
> 
> 	return order;
> err:
> 	pr_err("invalid size %s in thp_anon boot parameter\n", size_str);
> 	return -EINVAL;
> }
> 
>>
>>> +     order = fls(size >> PAGE_SHIFT) - 1;
>>
>> Is this a fancy way of writing
>>
>> order = log2(size >> PAGE_SHIFT);
>>
>> ? :)
> 
> I think ilog2 is implemented by fls ?

Yes, so we should have used that instead. But get_order()
is even better.

> 
>>
>> Anyhow, if get_order() wraps that, all good.
> 
> I guess it doesn't check power of 2?
> 
>>
>>> +     if ((1 << order) & ~THP_ORDERS_ALL_ANON) {
>>> +             pr_err("invalid size %s(order %d) in thp_anon boot parameter\n",
>>> +                     size_str, order);
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     return order;
>>> +}
>>
>> Apart from that, nothing jumped at me.
> 
> Please take a look at the new get_order_from_str() before I
> send v5 :-)

Besides the shift for is_power_of_2(), LGTM, thanks!

-- 
Cheers,

David / dhildenb