linux-kernel - Re: [PATCH 1/2] mm: fix missing handler for __GFP

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: <c44769b3-a132-63a5-fd40-4d483497dff2@bytedance.com>
Date:   Wed, 11 May 2022 10:19:48 +0800
From:   Qi Zheng <zhengqi.arch@...edance.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     akinobu.mita@...il.com, vbabka@...e.cz, gregkh@...uxfoundation.org,
        jirislaby@...nel.org, rostedt@...dmis.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH 1/2] mm: fix missing handler for __GFP_NOWARN



On 2022/5/11 2:59 AM, Andrew Morton wrote:
> On Tue, 10 May 2022 19:38:08 +0800 Qi Zheng <zhengqi.arch@...edance.com> wrote:
> 
>> We expect no warnings to be issued when we specify __GFP_NOWARN, but
>> currently in paths like alloc_pages() and kmalloc(), there are still
>> some warnings printed, fix it.
> 
> Looks sane to me.
> 
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -35,6 +35,17 @@ struct folio_batch;
>>   /* Do not use these with a slab allocator */
>>   #define GFP_SLAB_BUG_MASK (__GFP_DMA32|__GFP_HIGHMEM|~__GFP_BITS_MASK)
>>   
>> +#define WARN_ON_ONCE_GFP(cond, gfp)	({				\
>> +	static bool __section(".data.once") __warned;			\
>> +	int __ret_warn_once = !!(cond);					\
>> +									\
>> +	if (unlikely(!(gfp & __GFP_NOWARN) && __ret_warn_once && !__warned)) { \
>> +		__warned = true;					\
>> +		WARN_ON(1);						\
>> +	}								\
>> +	unlikely(__ret_warn_once);					\
>> +})
> 
> I don't think WARN_ON_ONCE_GFP is a good name for this.  But
> WARN_ON_ONCE_IF_NOT_GFP_NOWARN is too long :(
> 
> WARN_ON_ONCE_NOWARN might be better.  No strong opinion here, really.

I've thought about WARN_ON_ONCE_NOWARN, but I feel a little weird 
putting 'WARN' and 'NOWARN' together, how about WARN_ON_ONCE_IF_ALLOWED?

> 
>> @@ -4902,8 +4906,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>>   	 * We also sanity check to catch abuse of atomic reserves being used by
>>   	 * callers that are not in atomic context.
>>   	 */
>> -	if (WARN_ON_ONCE((gfp_mask & (__GFP_ATOMIC|__GFP_DIRECT_RECLAIM)) ==
>> -				(__GFP_ATOMIC|__GFP_DIRECT_RECLAIM)))
>> +	if (WARN_ON_ONCE_GFP((gfp_mask & (__GFP_ATOMIC|__GFP_DIRECT_RECLAIM)) ==
>> +				(__GFP_ATOMIC|__GFP_DIRECT_RECLAIM), gfp_mask))
>>   		gfp_mask &= ~__GFP_ATOMIC;
>>   
>>   retry_cpuset:
> 
> I dropped this hunk - Neil's "mm: discard __GFP_ATOMIC"
> (https://lkml.kernel.org/r/163712397076.13692.4727608274002939094@noble.neil.brown.name)
> deleted this code.
> 

This series is based on v5.18-rc5, I will rebase it to the latest next
branch and check if there are any missing WARN_ON_ONCEs that are not
being handled.

Thanks,
Qi

-- 
Thanks,
Qi