Message-ID: <c658a72a-b3f6-4504-a940-7ff946c81559@sdfg.com.ar>
Date:   Wed, 27 Sep 2023 15:06:03 +0200
From:   Rodrigo Campos <rodrigo@...g.com.ar>
To:     Thomas Weißschuh <linux@...ssschuh.net>
Cc:     Willy Tarreau <w@....eu>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] tools/nolibc: Add workarounds for centos-7

On 9/27/23 01:30, Thomas Weißschuh wrote:
> thanks for your patch!

Thank you for nolibc :)

> On 2023-09-26 15:36:47+0200, Rodrigo Campos wrote:
>> Centos-7 doesn't include statx on its linux/stat.h file. So, let's just
>> define it if the include doesn't define STATX_BASIC_STATS.
> 
> Could you mention which version of the kernel headers you compiled this
> with and with which version you tested it?

It was on CI using vagrant, it is:

  kernel-headers   x86_64 3.10.0-1160.99.1.el7

And this kernel:

Linux cirrus-task-6368858685046784 3.10.0-1160.95.1.el7.x86_64 #1 SMP 
Mon Jul 24 13:59:37 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Luckily this info is exposed in their CI easily, see here: 
https://cirrus-ci.com/task/6368858685046784?logs=host_info#L1. The 
host_info and "Run install_dependencies" have this kind of info.

> Also which is the exact revision you use to extract nolibc?

As from Linux 6.6-rc3 (6465e260f48790807eef06b583b38ca9789b6072)

> Does nolibc actually support statx()/stat() on centos-7 with these changes?

Oh, I haven't tried that, sorry I didn't mention. We are basically just 
using exceve(), but the thing is that "sutrct statx" and 
"STATX_BASIC_STATS" are used in sys.h, so compilation fails.

And just a forward declaration of the struct won't help, as it is not 
just a pointer, in stat() we instantiate it.


> I'm asking because I tried to reproduce it and for me CentOS 7 with
> kernel-headers 3.10.0-1160.99.1.el7 doesn't define __NR_statx.
> Without this symbol the statx() and stat() functions should just always
> return -ENOSYS.
> It seems a bit wasteful to introduce 200 new lines of code for a "feature"
> that will not do anything.
> 
> FYI the hard requirement for the statx syscall is fairly new, it was
> added in commit af93807eaef6 ("tools/nolibc: remove the old sys_stat support").

Oh, great pointer, thanks!

> As you are vendoring nolibc, if you don't need stat/statx support in
> for your usecase you could drop the support for it in your vendored
> copy.
> Or we try to reintroduce compatibility for stat() without the statx()
> syscall. But given the really limited applicability, personally I'm
> against that.

We can definitely remove that struct statx bits in our vendoring. It 
will simplify updating if we don't have to patch it, so if we can't 
include a fix in nolibc, I think we will continue doing the hack 
ourselves and that is all. It is not too bad :)

I don't think it is worth for nolibc, at least for this use case, to 
reintroduce compatibility for stat() without statx().

For now we are work-arounding it by doing basically the same thing I'm 
doing here:
	https://github.com/opencontainers/runc/blob/96a61d3bf0dcc26343bfafe5112934d73d280dd3/libcontainer/dmz/xstat.h

We then include this file before nolibc.h, and then the type works as fine:
	https://github.com/opencontainers/runc/blob/96a61d3bf0dcc26343bfafe5112934d73d280dd3/libcontainer/dmz/_dmz.c

Would it be acceptable for nolibc if I just define what we use:
  * struct statx
  * struct statx_timestamp (used inside struct statx)
  * STATX_BASIC_STATS (or STATX_* constants too, as you prefer)

?

> 
> Some more notes below.
> 
>> This makes nolibc work on centos-7 just fine, before this patch it
>> failed with:
>>
>> 	nolibc/sys.h:987:78: warning: ‘struct statx’ declared inside parameter list [enabled by default]
>> 	 int sys_statx(int fd, const char *path, int flags, unsigned int mask, struct statx *buf)
>>
>> Please note that while on types.h we can still include linux/stat.h
>> and it won't cause any issues, it seems simpler if we just always
>> include "statx.h" instead of that file and be safe. That is why I
>> changed types.h too.
> 
> All of nolibc will end up included into the same namespace by design.
> It seems weird that it would make a difference from where this file is
> included.

No, sorry, I just wanted to say that we should include "statx.h" instead 
of "<linux/stat.h>" as that does the define for old distros that don't 
do it.

It is true that we can include "statx.h" in nolibc.h and that should do 
the trick too.

>> diff --git tools/include/nolibc/statx.h tools/include/nolibc/statx.h
>> new file mode 100644
>> index 000000000000..d05528754154
>> --- /dev/null
>> +++ tools/include/nolibc/statx.h
>> @@ -0,0 +1,218 @@
> 
> Below you mention that this was copied from
> tools/include/uapi/linux/stat.h, but...
> 
>> +/* SPDX-License-Identifier: LGPL-2.1 OR MIT */
> 
> The original code was "GPL-2.0 WITH Linux-syscall-note".

Oh, great point, sorry I didn't realize. What should we do, then?

Do you know what is the proper way to just define the same types here 
(i.e. struct statx and what I mentioned is what we really need a few 
lines above)?

>> +#define S_IFMT  00170000
>> +#define S_IFSOCK 0140000
>> +#define S_IFLNK	 0120000
>> +#define S_IFREG  0100000
>> +#define S_IFBLK  0060000
>> +#define S_IFDIR  0040000
>> +#define S_IFCHR  0020000
>> +#define S_IFIFO  0010000
>> +#define S_ISUID  0004000
>> +#define S_ISGID  0002000
>> +#define S_ISVTX  0001000
>> +
>> +#define S_ISLNK(m)	(((m) & S_IFMT) == S_IFLNK)
>> +#define S_ISREG(m)	(((m) & S_IFMT) == S_IFREG)
>> +#define S_ISDIR(m)	(((m) & S_IFMT) == S_IFDIR)
>> +#define S_ISCHR(m)	(((m) & S_IFMT) == S_IFCHR)
>> +#define S_ISBLK(m)	(((m) & S_IFMT) == S_IFBLK)
>> +#define S_ISFIFO(m)	(((m) & S_IFMT) == S_IFIFO)
>> +#define S_ISSOCK(m)	(((m) & S_IFMT) == S_IFSOCK)
>> +
>> +#define S_IRWXU 00700
>> +#define S_IRUSR 00400
>> +#define S_IWUSR 00200
>> +#define S_IXUSR 00100
>> +
>> +#define S_IRWXG 00070
>> +#define S_IRGRP 00040
>> +#define S_IWGRP 00020
>> +#define S_IXGRP 00010
>> +
>> +#define S_IRWXO 00007
>> +#define S_IROTH 00004
>> +#define S_IWOTH 00002
>> +#define S_IXOTH 00001
> 
> We already have all of these in types.h.

Yes, I realized but I thought it was simpler conceptually if we include 
the whole file. This way there are no differences if the host has this 
or not.

I can definitely remove this if you prefer and just define what we 
strictly need.

>> diff --git tools/include/nolibc/sys.h tools/include/nolibc/sys.h
>> index fdb6bd6c0e2f..d3e45793682a 100644
>> --- tools/include/nolibc/sys.h
>> +++ tools/include/nolibc/sys.h
>> @@ -20,9 +20,9 @@
>>   #include <linux/time.h>
>>   #include <linux/auxvec.h>
>>   #include <linux/fcntl.h> /* for O_* and AT_* */
>> -#include <linux/stat.h>  /* for statx() */
> 
> So this means that compatibility with user applications that also
> include <linux/stat.h> on their own is broken?
> That would not be good.


Hmm, no, it just means that if we want to get struct statx in all 
distros, including centos-7, we should use the other include that will 
define it for centos.

We can keep this here as long as we also include xstat.h, in some other 
part.




Best,
Rodrigo

Powered by blists - more mailing lists