Re: [PATCH] Documentation: syscalls: add a note about ABI-agnostic types

[Yury Norov <yury.norov@gmail.com>]

On Wed, Apr 14, 2021 at 08:14:22AM +0200, Mauro Carvalho Chehab wrote:
> Em Tue, 13 Apr 2021 21:40:20 -0700
> Yury Norov <yury.norov@gmail.com> escreveu:
> 
> > Ping?
> > 
> > On Fri, Apr 09, 2021 at 01:43:04PM -0700, Yury Norov wrote:
> > > Recently added memfd_secret() syscall had a flags parameter passed
> > > as unsigned long, which requires creation of compat entry for it.
> > > It was possible to change the type of flags to unsigned int and so
> > > avoid bothering with compat layer.
> > > 
> > > https://www.spinics.net/lists/linux-mm/msg251550.html
> > > 
> > > Documentation/process/adding-syscalls.rst doesn't point clearly about
> > > preference of ABI-agnostic types. This patch adds such notification.
> > > 
> > > Signed-off-by: Yury Norov <yury.norov@gmail.com>
> > > ---
> > >  Documentation/process/adding-syscalls.rst | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/Documentation/process/adding-syscalls.rst b/Documentation/process/adding-syscalls.rst
> > > index 9af35f4ec728..46add16edf14 100644
> > > --- a/Documentation/process/adding-syscalls.rst
> > > +++ b/Documentation/process/adding-syscalls.rst
> > > @@ -172,6 +172,13 @@ arguments (i.e. parameter 1, 3, 5), to allow use of contiguous pairs of 32-bit
> > >  registers.  (This concern does not apply if the arguments are part of a
> > >  structure that's passed in by pointer.)
> > >  
> > > +Whenever possible, try to use ABI-agnostic types for passing parameters to
> > > +a syscall in order to avoid creating compat entry for it. Linux supports two
> > > +ABI models - ILP32 and LP64. 
> 
> > > + The types like ``void *``, ``long``, ``size_t``,
> > > +``off_t`` have different size in those ABIs;
> 
> In the case of pointers, the best is to use __u64. The pointer can then
> be read on Kernelspace with something like this:
> 
> 	static inline void __user *media_get_uptr(__u64 arg)
> 	{
> 		return (void __user *)(uintptr_t)arg;
> 	}
 
For 32-bit userspace reserving 64-bit type for pointers looks
excessive, isn't? And anyways, how could this help to prevent
malicious/broken compat userspace from passing pointers with
dirty top 32 bits?

From what I can see, in case of compat ABI, the 'void *' args
are cast to compat_uptr_t in the compat layer, and then passed
to native handlers. Bypassing compat layer in the example above
would break consistency for a syscall.
 
> > > types like ``char`` and  ``int``
> > > +have the same size and don't require a compat layer support. For flags, it's
> > > +always better to use ``unsigned int``.
> > > +
> 
> I don't think this is true for all compilers on userspace, as the C
> standard doesn't define how many bits an int/unsigned int has. 
> So, even if this is today's reality, things may change in the future.

Agree, it's not a standard in C, but this is pretty much a standard in
Linux. Introducing a new ABI nor ILP32, neither LP64 would require huge
amount of work, especially on a maintenance level, and I bet it will be
blocked by Arnd. :) In practice it's correct to recommend using unsigned
int for flags now, and if in future someone will introduce new ABI, it
will be his responsibility to explain us how to design syscalls in a
compatible and unified way.

> For instance, I remember we had to replace "int" and "enum" by "__u32" 
> and "long" by "__u64" at the media uAPI in the past, when we start
> seeing x86_64 Kernels with 32-bits userspace and when cameras started 
> being supported on arm32.
> 
> We did have some real bugs with "enum", as, on that time, some
> compilers (gcc, I guess) were optimizing them to have less than
> 32 bits on certain architectures, when it fits.

I think this example agrees with what I said - if userspace has
nonstandard ABI, it has to use kernel types to communicate with
kernel, which are exposed as __u32-style typedefs. For me, it's
a compatibility layer implemented in userspace.

This patch is about good practices for standard 32, 64 and compat 
ABIs supported by kernel.

(Or if I missed you point, can you please explain in more details?)

Thanks,
Yury