Re: [PATCH v2] huge_mm.h: disallow is_huge_zero_folio(NULL)

[David Hildenbrand <david@redhat.com>]

> This seems not so great.
> 
> I really think the VM_WARN_ON_ONCE() should be in folios_put_refs() based
> on what you've said.

No.

Everything in folio_put_refs() will dereference the folio and properly 
crash the machine when doing something stupid.

This function, however, will silently swallow a "NULL" pointer.

> 
>>
>> Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
>> ---
>>   include/linux/huge_mm.h | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>> index 7748489fde1b..bc9ca7798f2e 100644
>> --- a/include/linux/huge_mm.h
>> +++ b/include/linux/huge_mm.h
>> @@ -2,6 +2,7 @@
>>   #ifndef _LINUX_HUGE_MM_H
>>   #define _LINUX_HUGE_MM_H
>>
>> +#include <linux/mmdebug.h> // for VM_WARN_ON_ONCE()
> 
> Please don't do //.
> 
> This include is suspect though, huge_mm.h is included from mm.h and thus
> this very easily might break some arch that is weird about this stuff,
> because a ton of stuff includes mm.h including things that might absolutely
> baulk at mmdebug.

Jup. Very likely this is not required.

> 
> I've had this kind of thing happen several times before.
> 
>>   #include <linux/mm_types.h>
>>
>>   #include <linux/fs.h> /* only for vma_is_dax() */
>> @@ -479,6 +480,8 @@ extern unsigned long huge_zero_pfn;
>>
>>   static inline bool is_huge_zero_folio(const struct folio *folio)
>>   {
>> +	VM_WARN_ON_ONCE(folio == NULL);
>> +
> 
> Convention is VM_WARN_ON_ONCE(!folio);

Wanted to comment the same thing.

-- 
Cheers

David / dhildenb