linux-kernel - Re: [PATCH v1 2/2] mm/highmem: make nr_free

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: <04b3dda2-c6a8-4f26-90b8-75fe7580d63e@redhat.com>
Date: Tue, 11 Jun 2024 11:20:00 +0200
From: David Hildenbrand <david@...hat.com>
To: Wei Yang <richard.weiyang@...il.com>
Cc: Oscar Salvador <osalvador@...e.de>, linux-kernel@...r.kernel.org,
 linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v1 2/2] mm/highmem: make nr_free_highpages() return
 "unsigned long"

On 11.06.24 02:56, Wei Yang wrote:
> On Mon, Jun 10, 2024 at 10:22:49AM +0200, David Hildenbrand wrote:
>> On 10.06.24 05:40, Oscar Salvador wrote:
>>> On Fri, Jun 07, 2024 at 10:37:11AM +0200, David Hildenbrand wrote:
>>>> It looks rather weird that totalhigh_pages() returns an
>>>> "unsigned long" but nr_free_highpages() returns an "unsigned int".
>>>>
>>>> Let's return an "unsigned long" from nr_free_highpages() to be
>>>> consistent.
>>>>
>>>> While at it, use a plain "0" instead of a "0UL" in the !CONFIG_HIGHMEM
>>>> totalhigh_pages() implementation, to make these look alike as well.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@...hat.com>
>>> ...
>>>> -static inline unsigned int nr_free_highpages(void) { return 0; }
>>>> -static inline unsigned long totalhigh_pages(void) { return 0UL; }
>>>> +static inline unsigned long nr_free_highpages(void) { return 0; }
>>>> +static inline unsigned long totalhigh_pages(void) { return 0; }
>>>
>>> Although I doubt it has any consequences, I would just leave them both with UL,
>>> so the return type is consistent with what we are returning.
>>
>> These suffixes are only required when using constants that would not fit
>> into the native (int) type, or converting from that native (int) type to
>> something else automatically by the compiler would mess things up (for example,
>> undesired sign extension). For 0 that is certainly impossible :)
>>
>>
>> That's also the reason why in include/linux we now have:
>>
>> t14s: ~/git/linux/include/linux $ git grep "return 0UL;"
>> skbuff.h:       return 0UL;
>> uaccess.h:static inline unsigned long user_access_save(void) { return 0UL; }
>> t14s: ~/git/linux/include/linux $ git grep "0UL;"
>> bitmap.h:               *dst = ~0UL;
>> dax.h:          return ~0UL;
>> mtd/map.h:                      r.x[i] = ~0UL;
>> netfilter.h:    return ((ul1[0] ^ ul2[0]) | (ul1[1] ^ ul2[1])) == 0UL;
>> skbuff.h:       return 0UL;
>> uaccess.h:static inline unsigned long user_access_save(void) { return 0UL; }
>>
>>
>> ... compared to a long list if "unsigned long" functions that simply "return 0;"
>>
> 
> Seems this is the current status.
> 
> Then my question is do we have a guide line for this? Or 0 is the special
> case? Sounds positive value has no sign extension problem. If we need to
> return 1, we suppose to use 1 or 1UL? I found myself confused.
> 
> I grepped "return 1" and do find some cases without UL:
> 
> backing-dev.h: wb_stat_error() return 1 for unsigned long.
> pgtable.h: pte_batch_hint() return 1 for unsigned int.
> 
> So the guide line is for positive value, it is not necessary to use UL?

I think when returning simple values (0/1/-1), we really don't need 
these suffices at all. The standard says "The type of an integer 
constant is the first of the corresponding list in which its value can 
be represented.". I thought it would always use an "int", but that is 
not the case.

So, if we use "-1", the compiler will use an "int", and sign extension 
to "unsigned" long will do the right thing.

Simple test:

-1 results in: 0xffffffffffffffff
-1U results in: 0xffffffff
-1UL results in: 0xffffffffffffffff
0xffffffff results in: 0xffffffff
0xffffffffU results in: 0xffffffff
0xffffffffUL results in: 0xffffffff
~0xffffffff results in: 0x0
~0xffffffffU results in: 0x0
~0xffffffffUL results in: 0xffffffff00000000
0xffffffffffffffff results in: 0xffffffffffffffff
0xffffffffffffffffU results in: 0xffffffffffffffff
0xffffffffffffffffUL results in: 0xffffffffffffffff


I thought that "0xffffffff" could be a problem (sign-extending to 
0xffffffffffffffff), but that does not seem to be the case -- likely 
using "unsigned int" as type. Also, I'm surprised that 
0xffffffffffffffffU works as expected, I would have thought the "U" 
would make the compiler complain about the value not fitting into an 
unsigned int.


When only returning values, the compiler usually does the right thing. 
Only when performing operations on the constant (see ~ example above), 
we might have to use the suffixes, depending on the intended outcome.

-- 
Cheers,

David / dhildenb