Date:   Tue, 4 May 2021 20:54:07 +0200
From:   "Alejandro Colomar (man-pages)" <alx.manpages@...il.com>
To:     Zack Weinberg <zackw@...ix.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Daniel Borkmann <daniel@...earbox.net>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        "Michael Kerrisk (man-pages)" <mtk.manpages@...il.com>,
        linux-man <linux-man@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        glibc <libc-alpha@...rceware.org>, GCC <gcc-patches@....gnu.org>,
        bpf <bpf@...r.kernel.org>,
        Joseph Myers <joseph@...esourcery.com>,
        David Laight <David.Laight@...lab.com>
Subject: Re: [RFC v2] bpf.2: Use standard types and attributes

Hi Greg, Daniel,

On 5/4/21 6:06 PM, Greg KH wrote:
 > There's a very old post from Linus where he describes the difference
 > between things like __u32 and uint32_t.  They are not the same, they
 > live in different namespaces, and worlds, and can not always be swapped
 > out for each other on all arches.>
 > Dig it up if you are curious, but for user/kernel apis you HAVE to use
 > the __uNN and can not use uintNN_t variants, so don't try to mix/match
 > them, it's good to just follow the kernel standard please.
I found these:

* [RFC] Splitting kernel headers and deprecating __KERNEL__ 
<https://lore.kernel.org/lkml/Pine.LNX.4.58.0412140734340.3279@ppc970.osdl.org/T/>

* coding style 
<https://lore.kernel.org/lkml/alpine.LFD.0.98.0706160840290.14121@woody.linux-foundation.org/>

* [patch] Small input fixes for 2.5.29 
<https://lore.kernel.org/lkml/Pine.LNX.4.33.0207301417190.2051-100000@penguin.transmeta.com/T/>

I already knew the first one, and now found the other two.  If there's 
any other thread that is relevant, I couldn't find it.

The thing is, in all of those threads, the only reasons to avoid 
<stdint.h> types in the kernel (at least, the only explicitly mentioned 
ones) are (a bit simplified, but this is the general idea of those threads):

* Possibly breaking something in such a big automated change.
* Namespace collision with userspace (the C standard allows defining 
uint32_t for nefarious purposes as long as you don't include <stdint.h>. 
  POSIX prohibits that, though)
* Uglier

But

* The manual pages only document the variable size and signedness by 
using either '__u32' or 'uint32_t'.  We state that the variable is an 
unsigned integer of exactly 32 bits; nothing more and nothing less.  It 
doesn't specify that those types are defined in <linux/bpf.h> (or 
whatever header a specific manual page uses).  In fact, in uint32_t(3) 
we clearly state the headers that shall provide the type.  In the end, 
the kernel will receive a 32 bit number.  I'm not exactly sure about 
what is wrong with this.  Is there any magic in the kernel/user 
interface beyond what the standard and the compiler define that I ignore?

* At that time (~2004), the C99 and POSIX.1-2001 standards were quite 
young, and it was likely to find code that defined uint32_t.  Currently, 
it is hard to find something that compiles without C99, and even if C99 
allows you to define uint32_t as long as you don't include <stdint.h>, 
it would be really stupid to do so.  And POSIX, which completely 
prohibits defining uint32_t, is also very present in Linux and other 
UNIX systems.  So we can probably guarantee that using <stdint.h> in the 
kernel wouldn't break anything.  But yet this isn't trying to do so. 
This is only about the manual pages.

I haven't read it in any of those threads, but suspect that the static 
analyzer used for the kernel might use extra information from the 
different 'u32'/'__u32' type names to do some extra checks.  Does it?

 > and can not always be swapped out for each other on all arches.

Really?  'uint32_t' is defined as "an unsigned integer type of a fixed 
width of exactly 32 bits".  How is that different from '[__]u32'? 
Aren't the kernel types guaranteed to be unsigned integers of exactly 32 
bits?  AFAICT, they are 100% binary compatible; and if not, it's 
probably a kernel bug.

Yes there are archs that don't provide 64 bit integers (I ignore if any 
of the archs supported by Linux does though), but if an arch doesn't 
provide 'uint64_t', it will neither be possible to have '__u64'.

[
        uintN_t
               Include: <stdint.h>.  Alternatively, <inttypes.h>.

               uint8_t, uint16_t, uint32_t, uint64_t

               An unsigned integer type of a fixed width  of  ex‐
               actly  N  bits, N being the value specified in its
               type name.  According to the C language  standard,
               they  shall  be  capable  of storing values in the
               range [0, UINTN_MAX], substituting N by the appro‐
               priate number.

               According   to   POSIX,   uint8_t,  uint16_t,  and
               uint32_t are required; uint64_t is  only  required
               in implementations that provide integer types with
               width 64; and all other types of this form are op‐
               tional.

] -- uint32_t(3)


 >
 > So consider this my:
 >
 > Nacked-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
 >
 > as well.
Okay.

On 5/4/21 6:08 PM, Daniel Borkmann wrote:
 >
 > But what /problem/ is this really solving? Why bother to change this 
/now/
 > after so many years?! I think this is causing more confusion than solving
 > anything, really. Moreover, what are you doing with all the
 > __{le,be}{16,32,64}
 > types in uapi? Anyway, NAK for bpf.2 specifically, and the idea 
generally..
 >

I'm trying to clarify the manual pages as much as possible, by using 
standard conventions and similar structure all around the pages.  Not 
everyone understands kernel conventions.  Basically, Zack said very much 
what I had in mind with this patch.


Thanks for your reviews!

Regards,

Alex

--
Alejandro Colomar
Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

Powered by blists - more mailing lists