[musl] [PATCH] add statx

[musl] [PATCH] add statx

[Ben Noordhuis]

glibc exposes a wrapper for this system call. it's inconvenient that
musl doesn't so let's add one.

ref: https://github.com/rust-lang/rust/pull/67774
---
 include/fcntl.h    | 15 +++++++++++++++
 include/sys/stat.h | 34 ++++++++++++++++++++++++++++++++++
 src/stat/fstatat.c | 27 +--------------------------
 src/stat/statx.c   |  8 ++++++++
 4 files changed, 58 insertions(+), 26 deletions(-)
 create mode 100644 src/stat/statx.c

diff --git a/include/fcntl.h b/include/fcntl.h
index b664cdc4..21050a65 100644
--- a/include/fcntl.h
+++ b/include/fcntl.h
@@ -106,6 +106,21 @@ int posix_fallocate(int, off_t, off_t);
 #define AT_STATX_DONT_SYNC 0x4000
 #define AT_RECURSIVE 0x8000
 
+#define STATX_TYPE 1U
+#define STATX_MODE 2U
+#define STATX_NLINK 4U
+#define STATX_UID 8U
+#define STATX_GID 0x10U
+#define STATX_ATIME 0x20U
+#define STATX_MTIME 0x40U
+#define STATX_CTIME 0x80U
+#define STATX_INO 0x100U
+#define STATX_SIZE 0x200U
+#define STATX_BLOCKS 0x400U
+#define STATX_BASIC_STATS 0x7ffU
+#define STATX_BTIME 0x800U
+#define STATX_ALL 0xfffU
+
 #define FAPPEND O_APPEND
 #define FFSYNC O_SYNC
 #define FASYNC O_ASYNC
diff --git a/include/sys/stat.h b/include/sys/stat.h
index 10d446c4..5db71590 100644
--- a/include/sys/stat.h
+++ b/include/sys/stat.h
@@ -5,6 +5,7 @@ extern "C" {
 #endif
 
 #include <features.h>
+#include <stdint.h>
 
 #define __NEED_dev_t
 #define __NEED_ino_t
@@ -70,6 +71,38 @@ extern "C" {
 #define UTIME_NOW  0x3fffffff
 #define UTIME_OMIT 0x3ffffffe
 
+#if defined(_GNU_SOURCE) || defined(_BSD_SOURCE)
+struct statx_timestamp {
+	int64_t tv_sec;
+	uint32_t tv_nsec;
+	int32_t __pad;
+};
+
+struct statx {
+	uint32_t stx_mask;
+	uint32_t stx_blksize;
+	uint64_t stx_attributes;
+	uint32_t stx_nlink;
+	uint32_t stx_uid;
+	uint32_t stx_gid;
+	uint16_t stx_mode;
+	uint16_t __pad0[1];
+	uint64_t stx_ino;
+	uint64_t stx_size;
+	uint64_t stx_blocks;
+	uint64_t stx_attributes_mask;
+	struct statx_timestamp stx_atime;
+	struct statx_timestamp stx_btime;
+	struct statx_timestamp stx_ctime;
+	struct statx_timestamp stx_mtime;
+	uint32_t stx_rdev_major;
+	uint32_t stx_rdev_minor;
+	uint32_t stx_dev_major;
+	uint32_t stx_dev_minor;
+	uint64_t __pad1[14];
+};
+#endif
+
 int stat(const char *__restrict, struct stat *__restrict);
 int fstat(int, struct stat *);
 int lstat(const char *__restrict, struct stat *__restrict);
@@ -93,6 +126,7 @@ int utimensat(int, const char *, const struct timespec [2], int);
 
 #if defined(_GNU_SOURCE) || defined(_BSD_SOURCE)
 int lchmod(const char *, mode_t);
+int statx(int, const char *__restrict, int, unsigned, struct statx *__restrict);
 #define S_IREAD S_IRUSR
 #define S_IWRITE S_IWUSR
 #define S_IEXEC S_IXUSR
diff --git a/src/stat/fstatat.c b/src/stat/fstatat.c
index de165b5c..ab22e4c6 100644
--- a/src/stat/fstatat.c
+++ b/src/stat/fstatat.c
@@ -8,36 +8,11 @@
 #include "syscall.h"
 #include "kstat.h"
 
-struct statx {
-	uint32_t stx_mask;
-	uint32_t stx_blksize;
-	uint64_t stx_attributes;
-	uint32_t stx_nlink;
-	uint32_t stx_uid;
-	uint32_t stx_gid;
-	uint16_t stx_mode;
-	uint16_t pad1;
-	uint64_t stx_ino;
-	uint64_t stx_size;
-	uint64_t stx_blocks;
-	uint64_t stx_attributes_mask;
-	struct {
-		int64_t tv_sec;
-		uint32_t tv_nsec;
-		int32_t pad;
-	} stx_atime, stx_btime, stx_ctime, stx_mtime;
-	uint32_t stx_rdev_major;
-	uint32_t stx_rdev_minor;
-	uint32_t stx_dev_major;
-	uint32_t stx_dev_minor;
-	uint64_t spare[14];
-};
-
 static int fstatat_statx(int fd, const char *restrict path, struct stat *restrict st, int flag)
 {
 	struct statx stx;
 
-	int ret = __syscall(SYS_statx, fd, path, flag, 0x7ff, &stx);
+	int ret = __syscall(SYS_statx, fd, path, flag, STATX_BASIC_STATS, &stx);
 	if (ret) return ret;
 
 	*st = (struct stat){
diff --git a/src/stat/statx.c b/src/stat/statx.c
new file mode 100644
index 00000000..36b45d33
--- /dev/null
+++ b/src/stat/statx.c
@@ -0,0 +1,8 @@
+#define _GNU_SOURCE
+#include <sys/stat.h>
+#include "syscall.h"
+
+int statx(int dirfd, const char *restrict path, int flags, unsigned mask, struct statx *restrict stx)
+{
+	return syscall(SYS_statx, dirfd, path, flags, mask, stx);
+}
-- 
2.23.0

[musl] Re: [PATCH] add statx

[Ben Noordhuis]

On Sun, Jan 19, 2020 at 1:13 PM Ben Noordhuis <info@bnoordhuis.nl> wrote:
>
> glibc exposes a wrapper for this system call. it's inconvenient that
> musl doesn't so let's add one.
>
> ref: https://github.com/rust-lang/rust/pull/67774
> ---
>  include/fcntl.h    | 15 +++++++++++++++
>  include/sys/stat.h | 34 ++++++++++++++++++++++++++++++++++
>  src/stat/fstatat.c | 27 +--------------------------
>  src/stat/statx.c   |  8 ++++++++
>  4 files changed, 58 insertions(+), 26 deletions(-)
>  create mode 100644 src/stat/statx.c
>
> diff --git a/include/fcntl.h b/include/fcntl.h
> index b664cdc4..21050a65 100644
> --- a/include/fcntl.h
> +++ b/include/fcntl.h
> @@ -106,6 +106,21 @@ int posix_fallocate(int, off_t, off_t);
>  #define AT_STATX_DONT_SYNC 0x4000
>  #define AT_RECURSIVE 0x8000
>
> +#define STATX_TYPE 1U
> +#define STATX_MODE 2U
> +#define STATX_NLINK 4U
> +#define STATX_UID 8U
> +#define STATX_GID 0x10U
> +#define STATX_ATIME 0x20U
> +#define STATX_MTIME 0x40U
> +#define STATX_CTIME 0x80U
> +#define STATX_INO 0x100U
> +#define STATX_SIZE 0x200U
> +#define STATX_BLOCKS 0x400U
> +#define STATX_BASIC_STATS 0x7ffU
> +#define STATX_BTIME 0x800U
> +#define STATX_ALL 0xfffU
> +
>  #define FAPPEND O_APPEND
>  #define FFSYNC O_SYNC
>  #define FASYNC O_ASYNC
> diff --git a/include/sys/stat.h b/include/sys/stat.h
> index 10d446c4..5db71590 100644
> --- a/include/sys/stat.h
> +++ b/include/sys/stat.h
> @@ -5,6 +5,7 @@ extern "C" {
>  #endif
>
>  #include <features.h>
> +#include <stdint.h>
>
>  #define __NEED_dev_t
>  #define __NEED_ino_t
> @@ -70,6 +71,38 @@ extern "C" {
>  #define UTIME_NOW  0x3fffffff
>  #define UTIME_OMIT 0x3ffffffe
>
> +#if defined(_GNU_SOURCE) || defined(_BSD_SOURCE)
> +struct statx_timestamp {
> +       int64_t tv_sec;
> +       uint32_t tv_nsec;
> +       int32_t __pad;
> +};
> +
> +struct statx {
> +       uint32_t stx_mask;
> +       uint32_t stx_blksize;
> +       uint64_t stx_attributes;
> +       uint32_t stx_nlink;
> +       uint32_t stx_uid;
> +       uint32_t stx_gid;
> +       uint16_t stx_mode;
> +       uint16_t __pad0[1];
> +       uint64_t stx_ino;
> +       uint64_t stx_size;
> +       uint64_t stx_blocks;
> +       uint64_t stx_attributes_mask;
> +       struct statx_timestamp stx_atime;
> +       struct statx_timestamp stx_btime;
> +       struct statx_timestamp stx_ctime;
> +       struct statx_timestamp stx_mtime;
> +       uint32_t stx_rdev_major;
> +       uint32_t stx_rdev_minor;
> +       uint32_t stx_dev_major;
> +       uint32_t stx_dev_minor;
> +       uint64_t __pad1[14];
> +};
> +#endif
> +
>  int stat(const char *__restrict, struct stat *__restrict);
>  int fstat(int, struct stat *);
>  int lstat(const char *__restrict, struct stat *__restrict);
> @@ -93,6 +126,7 @@ int utimensat(int, const char *, const struct timespec [2], int);
>
>  #if defined(_GNU_SOURCE) || defined(_BSD_SOURCE)
>  int lchmod(const char *, mode_t);
> +int statx(int, const char *__restrict, int, unsigned, struct statx *__restrict);
>  #define S_IREAD S_IRUSR
>  #define S_IWRITE S_IWUSR
>  #define S_IEXEC S_IXUSR
> diff --git a/src/stat/fstatat.c b/src/stat/fstatat.c
> index de165b5c..ab22e4c6 100644
> --- a/src/stat/fstatat.c
> +++ b/src/stat/fstatat.c
> @@ -8,36 +8,11 @@
>  #include "syscall.h"
>  #include "kstat.h"
>
> -struct statx {
> -       uint32_t stx_mask;
> -       uint32_t stx_blksize;
> -       uint64_t stx_attributes;
> -       uint32_t stx_nlink;
> -       uint32_t stx_uid;
> -       uint32_t stx_gid;
> -       uint16_t stx_mode;
> -       uint16_t pad1;
> -       uint64_t stx_ino;
> -       uint64_t stx_size;
> -       uint64_t stx_blocks;
> -       uint64_t stx_attributes_mask;
> -       struct {
> -               int64_t tv_sec;
> -               uint32_t tv_nsec;
> -               int32_t pad;
> -       } stx_atime, stx_btime, stx_ctime, stx_mtime;
> -       uint32_t stx_rdev_major;
> -       uint32_t stx_rdev_minor;
> -       uint32_t stx_dev_major;
> -       uint32_t stx_dev_minor;
> -       uint64_t spare[14];
> -};
> -
>  static int fstatat_statx(int fd, const char *restrict path, struct stat *restrict st, int flag)
>  {
>         struct statx stx;
>
> -       int ret = __syscall(SYS_statx, fd, path, flag, 0x7ff, &stx);
> +       int ret = __syscall(SYS_statx, fd, path, flag, STATX_BASIC_STATS, &stx);
>         if (ret) return ret;
>
>         *st = (struct stat){
> diff --git a/src/stat/statx.c b/src/stat/statx.c
> new file mode 100644
> index 00000000..36b45d33
> --- /dev/null
> +++ b/src/stat/statx.c
> @@ -0,0 +1,8 @@
> +#define _GNU_SOURCE
> +#include <sys/stat.h>
> +#include "syscall.h"
> +
> +int statx(int dirfd, const char *restrict path, int flags, unsigned mask, struct statx *restrict stx)
> +{
> +       return syscall(SYS_statx, dirfd, path, flags, mask, stx);
> +}
> --
> 2.23.0
>

Can I get some feedback on this patch, even if it's just "no because"? Thanks.

Re: [musl] Re: [PATCH] add statx

[Rich Felker]

On Fri, Jan 24, 2020 at 09:38:49AM +0100, Ben Noordhuis wrote:
> 
> Can I get some feedback on this patch, even if it's just "no because"? Thanks.

Sorry aboout that; I'd just had my mind on other things and hadn't
taken the time to make a good review yet.

Re: [musl] Re: [PATCH] add statx

[Ben Noordhuis]

On Fri, Jan 24, 2020 at 3:01 PM Rich Felker <dalias@libc.org> wrote:
>
> On Fri, Jan 24, 2020 at 09:38:49AM +0100, Ben Noordhuis wrote:
> >
> > Can I get some feedback on this patch, even if it's just "no because"? Thanks.
>
> Sorry aboout that; I'd just had my mind on other things and hadn't
> taken the time to make a good review yet.

Thanks for the feedback and no worries, I'm no saint in that regard either.

Before I post a v2, did I understand the following issues correctly?

1. Switch _GNU_SOURCE || _BSD_SOURCE -> just _GNU_SOURCE? FWIW,
_BSD_SOURCE currently exposes the AT_STATX_* flags in fcntl.h.

2. uint64_t -> unsigned long long guarded by #ifdef __GNUC__
__extension__? Or just leave it as-is?

4. An ENOSYS fallback to fstatat()? glibc's fallback returns EINVAL
for AT_* flags it doesn't understand and ignores all STATX_* flags: it
sets stx_mask to STATX_BASIC_STATS, fills in stx_uid/stx_gid/etc. and
sets stx_btime to zero. Does that sound reasonable?

Re: [musl] Re: [PATCH] add statx

[Rich Felker]

On Tue, Jan 28, 2020 at 09:59:56AM +0100, Ben Noordhuis wrote:
> On Fri, Jan 24, 2020 at 3:01 PM Rich Felker <dalias@libc.org> wrote:
> >
> > On Fri, Jan 24, 2020 at 09:38:49AM +0100, Ben Noordhuis wrote:
> > >
> > > Can I get some feedback on this patch, even if it's just "no because"? Thanks.
> >
> > Sorry aboout that; I'd just had my mind on other things and hadn't
> > taken the time to make a good review yet.
> 
> Thanks for the feedback and no worries, I'm no saint in that regard either.
> 
> Before I post a v2, did I understand the following issues correctly?
> 
> 1. Switch _GNU_SOURCE || _BSD_SOURCE -> just _GNU_SOURCE?

Yes. 

> FWIW, _BSD_SOURCE currently exposes the AT_STATX_* flags in fcntl.h.

Being that they're "namespaced" under AT_*, I don't think this is a
big deal. I was generally of the opinion that AT_* flags make sense to
always expose since they might be used with standard interfaces as
extensions, rather than with nonstandard interfaces. Even though
AT_STATX_* seem statx-exclusive, maybe they make sense as extension
flags to fstatat too?

In any case I think we can leave any consideration of a change here
for later; it's separate from adding statx().

> 2. uint64_t -> unsigned long long guarded by #ifdef __GNUC__
> __extension__? Or just leave it as-is?

long long does not use __extension__ guard in musl, but I think we
should stick with the semantically correct types, [u]intNN_t.

> 4. An ENOSYS fallback to fstatat()? glibc's fallback returns EINVAL
> for AT_* flags it doesn't understand and ignores all STATX_* flags: it
> sets stx_mask to STATX_BASIC_STATS, fills in stx_uid/stx_gid/etc. and
> sets stx_btime to zero. Does that sound reasonable?

Sounds right.

Note that filling in stx_[r]dev_{major,minor} requires the
sys/sysmacros.h major()/minor()/ macros since statx oddly separated
them out of dev_t.

I don't think setting btime to 0 is strictly necessary, but since it's
in the range of the struct that's unconditionally present, it's okay
to do so, and perhaps guards against info leaks from old stack
contents.

Rich

Re: [musl] [PATCH] add statx

[Rich Felker]

On Sun, Jan 19, 2020 at 01:12:47PM +0100, Ben Noordhuis wrote:
> glibc exposes a wrapper for this system call. it's inconvenient that
> musl doesn't so let's add one.
> 
> ref: https://github.com/rust-lang/rust/pull/67774
> ---
>  include/fcntl.h    | 15 +++++++++++++++
>  include/sys/stat.h | 34 ++++++++++++++++++++++++++++++++++
>  src/stat/fstatat.c | 27 +--------------------------
>  src/stat/statx.c   |  8 ++++++++
>  4 files changed, 58 insertions(+), 26 deletions(-)
>  create mode 100644 src/stat/statx.c
> 
> diff --git a/include/fcntl.h b/include/fcntl.h
> index b664cdc4..21050a65 100644
> --- a/include/fcntl.h
> +++ b/include/fcntl.h
> @@ -106,6 +106,21 @@ int posix_fallocate(int, off_t, off_t);
>  #define AT_STATX_DONT_SYNC 0x4000
>  #define AT_RECURSIVE 0x8000
>  
> +#define STATX_TYPE 1U
> +#define STATX_MODE 2U
> +#define STATX_NLINK 4U
> +#define STATX_UID 8U
> +#define STATX_GID 0x10U
> +#define STATX_ATIME 0x20U
> +#define STATX_MTIME 0x40U
> +#define STATX_CTIME 0x80U
> +#define STATX_INO 0x100U
> +#define STATX_SIZE 0x200U
> +#define STATX_BLOCKS 0x400U
> +#define STATX_BASIC_STATS 0x7ffU
> +#define STATX_BTIME 0x800U
> +#define STATX_ALL 0xfffU
> +

This is under BSD||GNU (i.e. DEFAULT||ALL) rather than just under the
latter. Is there a reason for that? Generally the extensions that are
pretty clearly Linux-only, as opposed to something that other
POSIX-based OS's are likely to adopt, are put under GNU/ALL to
discourage their use without intent and to avoid namespace clutter.

Also, I don't think it makes sense for these to be in fcntl.h at all,
and I don't think there's precedent. glibc has them exposed via
sys/stat.h not fcntl.h. And it looks like glibc exposes them (and all
the statx stuff) only under _GNU_SOURCE:

#ifdef __USE_GNU
# include <bits/statx.h>
#endif

So in summary, unless there's strong reason to do differently, move to
sys/stat.h and make _GNU_SOURCE-only.

>  #define FAPPEND O_APPEND
>  #define FFSYNC O_SYNC
>  #define FASYNC O_ASYNC
> diff --git a/include/sys/stat.h b/include/sys/stat.h
> index 10d446c4..5db71590 100644
> --- a/include/sys/stat.h
> +++ b/include/sys/stat.h
> @@ -5,6 +5,7 @@ extern "C" {
>  #endif
>  
>  #include <features.h>
> +#include <stdint.h>

Extra indirect headers can't be pulled in unconditionally beyond
what's specified for sys/stat.h. Instead you could put it under
_GNU_SOURCE (this is another reason not to put it in default), but
what would be better is putting just the __NEED_* for the types you
want under _GNU_SOURCE here, e.g.

#ifdef _GNU_SOURCE
#define __NEED_uint16_t
#define __NEED_uint32_t
#define __NEED_uint64_t
#define __NEED_int32_t
#define __NEED_int64_t
#endif

>  int stat(const char *__restrict, struct stat *__restrict);
>  int fstat(int, struct stat *);
>  int lstat(const char *__restrict, struct stat *__restrict);
> @@ -93,6 +126,7 @@ int utimensat(int, const char *, const struct timespec [2], int);
>  
>  #if defined(_GNU_SOURCE) || defined(_BSD_SOURCE)
>  int lchmod(const char *, mode_t);
> +int statx(int, const char *__restrict, int, unsigned, struct statx *__restrict);

Also _GNU_SOURCE-only.

>  #define S_IREAD S_IRUSR
>  #define S_IWRITE S_IWUSR
>  #define S_IEXEC S_IXUSR
> diff --git a/src/stat/fstatat.c b/src/stat/fstatat.c
> index de165b5c..ab22e4c6 100644
> --- a/src/stat/fstatat.c
> +++ b/src/stat/fstatat.c
> @@ -8,36 +8,11 @@
>  #include "syscall.h"
>  #include "kstat.h"
>  
> -struct statx {
> -	uint32_t stx_mask;
> -	uint32_t stx_blksize;
> -	uint64_t stx_attributes;
> -	uint32_t stx_nlink;
> -	uint32_t stx_uid;
> -	uint32_t stx_gid;
> -	uint16_t stx_mode;
> -	uint16_t pad1;
> -	uint64_t stx_ino;
> -	uint64_t stx_size;
> -	uint64_t stx_blocks;
> -	uint64_t stx_attributes_mask;
> -	struct {
> -		int64_t tv_sec;
> -		uint32_t tv_nsec;
> -		int32_t pad;
> -	} stx_atime, stx_btime, stx_ctime, stx_mtime;
> -	uint32_t stx_rdev_major;
> -	uint32_t stx_rdev_minor;
> -	uint32_t stx_dev_major;
> -	uint32_t stx_dev_minor;
> -	uint64_t spare[14];
> -};
> -
>  static int fstatat_statx(int fd, const char *restrict path, struct stat *restrict st, int flag)
>  {
>  	struct statx stx;
>  
> -	int ret = __syscall(SYS_statx, fd, path, flag, 0x7ff, &stx);
> +	int ret = __syscall(SYS_statx, fd, path, flag, STATX_BASIC_STATS, &stx);
>  	if (ret) return ret;

This looks ok.

>  	*st = (struct stat){
> diff --git a/src/stat/statx.c b/src/stat/statx.c
> new file mode 100644
> index 00000000..36b45d33
> --- /dev/null
> +++ b/src/stat/statx.c
> @@ -0,0 +1,8 @@
> +#define _GNU_SOURCE
> +#include <sys/stat.h>
> +#include "syscall.h"
> +
> +int statx(int dirfd, const char *restrict path, int flags, unsigned mask, struct statx *restrict stx)
> +{
> +	return syscall(SYS_statx, dirfd, path, flags, mask, stx);
> +}
> -- 
> 2.23.0

Should we do fallback translation from stat if SYS_statx fails with
ENOSYS? My leaning would be yes.

Rich

Re: [musl] [PATCH] add statx

[Florian Weimer]

* Rich Felker:

> This is under BSD||GNU (i.e. DEFAULT||ALL) rather than just under the
> latter. Is there a reason for that? Generally the extensions that are
> pretty clearly Linux-only, as opposed to something that other
> POSIX-based OS's are likely to adopt, are put under GNU/ALL to
> discourage their use without intent and to avoid namespace clutter.

statx is not Linux-specific in glibc, but also available on Hurd.

We received feedback that our headers are not useful due to the
__u64/uint64_t mismatch.

  <https://sourceware.org/bugzilla/show_bug.cgi?id=25292
  <https://sourceware.org/ml/libc-alpha/2019-12/msg00631.html>

Thanks,
Florian

Re: [musl] [PATCH] add statx

[Rich Felker]

On Fri, Jan 24, 2020 at 04:27:47PM +0100, Florian Weimer wrote:
> * Rich Felker:
> 
> > This is under BSD||GNU (i.e. DEFAULT||ALL) rather than just under the
> > latter. Is there a reason for that? Generally the extensions that are
> > pretty clearly Linux-only, as opposed to something that other
> > POSIX-based OS's are likely to adopt, are put under GNU/ALL to
> > discourage their use without intent and to avoid namespace clutter.
> 
> statx is not Linux-specific in glibc, but also available on Hurd.

OK, well GNU/Linux-specific. :-) Some ppl find _GNU_SOURCE odd for
stuff that comes from Linux not GNU, but in this case it seems pretty
appropriate since GNU and Linux are the two systems doing it.

> We received feedback that our headers are not useful due to the
> __u64/uint64_t mismatch.
> 
>   <https://sourceware.org/bugzilla/show_bug.cgi?id=25292
>   <https://sourceware.org/ml/libc-alpha/2019-12/msg00631.html>

Uhg. That change seems unfortunate since it's incompatible with
theoretical future archs having 128-bit long long -- an idea I'm not
much a fan of, but don't actually want to preclude. Is there a reason
to actually care about compatibility with the kernel types? It's not
like both struct definitions can be included in the same source
anyway; the tags would clash. Using the canonical uintN_t types makes
more sense from an API perspective, I think; kernel assumptions about
types should not leak into an API intended to be clean and shared with
non-Linux implementations.

Rich

Re: [musl] [PATCH] add statx

[Florian Weimer]

* Rich Felker:

> On Fri, Jan 24, 2020 at 04:27:47PM +0100, Florian Weimer wrote:
>> * Rich Felker:
>> 
>> > This is under BSD||GNU (i.e. DEFAULT||ALL) rather than just under the
>> > latter. Is there a reason for that? Generally the extensions that are
>> > pretty clearly Linux-only, as opposed to something that other
>> > POSIX-based OS's are likely to adopt, are put under GNU/ALL to
>> > discourage their use without intent and to avoid namespace clutter.
>> 
>> statx is not Linux-specific in glibc, but also available on Hurd.
>
> OK, well GNU/Linux-specific. :-) Some ppl find _GNU_SOURCE odd for
> stuff that comes from Linux not GNU, but in this case it seems pretty
> appropriate since GNU and Linux are the two systems doing it.

Sorry for nit-picking, it's common to both GNU and Linux.

gettid is Linux-specific, and so is pthread_getattr_default_np.

pkey_set is something that is GNU/Linux-specific in the sense that is
something that's only part of glibc, and only on Linux.

>> We received feedback that our headers are not useful due to the
>> __u64/uint64_t mismatch.
>> 
>>   <https://sourceware.org/bugzilla/show_bug.cgi?id=25292
>>   <https://sourceware.org/ml/libc-alpha/2019-12/msg00631.html>
>
> Uhg. That change seems unfortunate since it's incompatible with
> theoretical future archs having 128-bit long long -- an idea I'm not
> much a fan of, but don't actually want to preclude. Is there a reason
> to actually care about compatibility with the kernel types?

Yes, printf format strings. 8-(

> It's not like both struct definitions can be included in the same
> source anyway; the tags would clash. Using the canonical uintN_t types
> makes more sense from an API perspective, I think; kernel assumptions
> about types should not leak into an API intended to be clean and
> shared with non-Linux implementations.

I thought so too, which is why I used them.

But it is fairly compelling to use the kernel types if the header is
available, so that we don't have to patch and rebuild glibc if someone
backports new statx features into the kernel.  That's why I added the
__has_include kludge.

Thanks,
Florian

Re: [musl] [PATCH] add statx

[Rich Felker]

On Fri, Jan 24, 2020 at 05:12:51PM +0100, Florian Weimer wrote:
> * Rich Felker:
> 
> > On Fri, Jan 24, 2020 at 04:27:47PM +0100, Florian Weimer wrote:
> >> * Rich Felker:
> >> 
> >> > This is under BSD||GNU (i.e. DEFAULT||ALL) rather than just under the
> >> > latter. Is there a reason for that? Generally the extensions that are
> >> > pretty clearly Linux-only, as opposed to something that other
> >> > POSIX-based OS's are likely to adopt, are put under GNU/ALL to
> >> > discourage their use without intent and to avoid namespace clutter.
> >> 
> >> statx is not Linux-specific in glibc, but also available on Hurd.
> >
> > OK, well GNU/Linux-specific. :-) Some ppl find _GNU_SOURCE odd for
> > stuff that comes from Linux not GNU, but in this case it seems pretty
> > appropriate since GNU and Linux are the two systems doing it.
> 
> Sorry for nit-picking, it's common to both GNU and Linux.

Yes, I was just making a bad joke about "GNU/Linux", reusing it to
mean "applies to both GNU and to Linux". Sorry for being unclear.

> >> We received feedback that our headers are not useful due to the
> >> __u64/uint64_t mismatch.
> >> 
> >>   <https://sourceware.org/bugzilla/show_bug.cgi?id=25292
> >>   <https://sourceware.org/ml/libc-alpha/2019-12/msg00631.html>
> >
> > Uhg. That change seems unfortunate since it's incompatible with
> > theoretical future archs having 128-bit long long -- an idea I'm not
> > much a fan of, but don't actually want to preclude. Is there a reason
> > to actually care about compatibility with the kernel types?
> 
> Yes, printf format strings. 8-(

Why not let ppl get the warnings and fix them by casting to an
appropriate type to print. I prefer [u]intmax_t anyway to avoid the
PRIu64 etc. mess that makes your format strings unreadable and that
interferes with translation (*).

> > It's not like both struct definitions can be included in the same
> > source anyway; the tags would clash. Using the canonical uintN_t types
> > makes more sense from an API perspective, I think; kernel assumptions
> > about types should not leak into an API intended to be clean and
> > shared with non-Linux implementations.
> 
> I thought so too, which is why I used them.
> 
> But it is fairly compelling to use the kernel types if the header is
> available, so that we don't have to patch and rebuild glibc if someone
> backports new statx features into the kernel.  That's why I added the
> __has_include kludge.

I see. This is something of a tradeoff I guess; for musl users I'd
expect folks to have libc headers newer than kernel headers in most
cases since distros are slow to follow new kernels but fast to follow
new libc, and you want compile-time support for future kernel
features even if you don't have the kernel to use them yet. But on
glibc-based dists it may be the other way around.

Rich



(*) GNU gettext has "SYSDEP" format strings that solve this problem
but do so by requiring non-shareable string memory for all the
affected strings. musl gettext does not support this, but is intended
to be used with gettext-tiny's version of msgfmt that works out the
combinatorics at .mo generation time and emits them all in shareable
form. Still I think it's better to get rid of this practice and just
use %jd etc. everywhere with suitable casts.

Re: [musl] [PATCH] add statx

[Florian Weimer]

* Rich Felker:

>> >> We received feedback that our headers are not useful due to the
>> >> __u64/uint64_t mismatch.
>> >> 
>> >>   <https://sourceware.org/bugzilla/show_bug.cgi?id=25292
>> >>   <https://sourceware.org/ml/libc-alpha/2019-12/msg00631.html>
>> >
>> > Uhg. That change seems unfortunate since it's incompatible with
>> > theoretical future archs having 128-bit long long -- an idea I'm not
>> > much a fan of, but don't actually want to preclude. Is there a reason
>> > to actually care about compatibility with the kernel types?
>> 
>> Yes, printf format strings. 8-(
>
> Why not let ppl get the warnings and fix them by casting to an
> appropriate type to print. I prefer [u]intmax_t anyway to avoid the
> PRIu64 etc. mess that makes your format strings unreadable and that
> interferes with translation (*).

I'm concerned that such casts hide or even introduce bugs, like casting
tv_sec to int.

Here's an example from the MySQL sources:

  int my_timeval_to_str(const struct timeval *tm, char *to, uint dec)
  {
    int len= sprintf(to, "%d", (int) tm->tv_sec);
    if (dec)
      len+= my_useconds_to_str(to + len, tm->tv_usec, dec);
    return len;
  }

That's why the kernel always uses unsigned long long for __u64, which
seems a reasonable trade-off to me.  It will make porting to 128-bit
architectures difficult.  But I think we should focus on the
architectures we have today.

>> > It's not like both struct definitions can be included in the same
>> > source anyway; the tags would clash. Using the canonical uintN_t types
>> > makes more sense from an API perspective, I think; kernel assumptions
>> > about types should not leak into an API intended to be clean and
>> > shared with non-Linux implementations.
>> 
>> I thought so too, which is why I used them.
>> 
>> But it is fairly compelling to use the kernel types if the header is
>> available, so that we don't have to patch and rebuild glibc if someone
>> backports new statx features into the kernel.  That's why I added the
>> __has_include kludge.
>
> I see. This is something of a tradeoff I guess; for musl users I'd
> expect folks to have libc headers newer than kernel headers in most
> cases since distros are slow to follow new kernels but fast to follow
> new libc, and you want compile-time support for future kernel
> features even if you don't have the kernel to use them yet. But on
> glibc-based dists it may be the other way around.

I think it's actually the same for self-built musl on most glibc-based
distributions because for that, developers will usually pick the latest
musl release and use the distribution's kernel headers.

But for glibc itself on glibc distributions, it's likely to have newer
kernel headers.  Fedora rebase kernels (including the UAPI headers) in a
release, but not glibc.  Debian has backport kernel packages for stable
releases (but no glibc backport).  Some enterprise distributions
aggressively backport features (including new userspace APIs) and even
rebase entire kernel subsystems from time to time.

Thanks,
Florian

Re: [musl] [PATCH] add statx

[Rich Felker]

On Tue, Jan 28, 2020 at 11:41:33AM +0100, Florian Weimer wrote:
> * Rich Felker:
> 
> >> >> We received feedback that our headers are not useful due to the
> >> >> __u64/uint64_t mismatch.
> >> >> 
> >> >>   <https://sourceware.org/bugzilla/show_bug.cgi?id=25292
> >> >>   <https://sourceware.org/ml/libc-alpha/2019-12/msg00631.html>
> >> >
> >> > Uhg. That change seems unfortunate since it's incompatible with
> >> > theoretical future archs having 128-bit long long -- an idea I'm not
> >> > much a fan of, but don't actually want to preclude. Is there a reason
> >> > to actually care about compatibility with the kernel types?
> >> 
> >> Yes, printf format strings. 8-(
> >
> > Why not let ppl get the warnings and fix them by casting to an
> > appropriate type to print. I prefer [u]intmax_t anyway to avoid the
> > PRIu64 etc. mess that makes your format strings unreadable and that
> > interferes with translation (*).
> 
> I'm concerned that such casts hide or even introduce bugs, like casting
> tv_sec to int.

As you've found, people will do incorrect casts regardless, in places
where it makes a bigger difference than here, where using fixed basic
types isn't an option. I don't think the solution is throwing away
proper semantic use of types.

> Here's an example from the MySQL sources:
> 
>   int my_timeval_to_str(const struct timeval *tm, char *to, uint dec)
>   {
>     int len= sprintf(to, "%d", (int) tm->tv_sec);
>     if (dec)
>       len+= my_useconds_to_str(to + len, tm->tv_usec, dec);
>     return len;
>   }

In the particular case of time, we should probably teach GCC to issue
warnings (-Wy2038?) for casts from an expression whose type was
declared via a typedef named time_t (I know GCC tracks knowledge of
the typedef name used, since it shows it in other warnings) to a type
smaller than 64-bit.

> That's why the kernel always uses unsigned long long for __u64, which
> seems a reasonable trade-off to me.  It will make porting to 128-bit
> architectures difficult.  But I think we should focus on the
> architectures we have today.

I disagree strongly with the last sentence. Designing an *API* in a
way that's not compatible with anything but long long == 64-bit is bad
API design. Arguably you could get into having something like
k_[u]intNN_t or similar, and use these, but I feel like that's just
gratuitous ugliness. The userspace type for 64-bit fixed-size fields
is [u]int64_t and we should be promoting its consistent usage, not
fragmenting things around kernel uapi mistakes. (Perhaps kernel uapi
headers should be fixed so that, when __KERNEL__ is not defined, they
include <stdint.h> and define __u64 etc. in terms of the stdint
types?)

Rich

Re: [musl] [PATCH] add statx

[Florian Weimer]

* Rich Felker:

>> That's why the kernel always uses unsigned long long for __u64, which
>> seems a reasonable trade-off to me.  It will make porting to 128-bit
>> architectures difficult.  But I think we should focus on the
>> architectures we have today.
>
> I disagree strongly with the last sentence. Designing an *API* in a
> way that's not compatible with anything but long long == 64-bit is bad
> API design.

We don't know what LL128 architectures will look like.  For all we know,
they might have more expressive type descriptors for variadic functions,
so the whole issue of matching integer types with precise format
specifiers becomes moot.

> Arguably you could get into having something like k_[u]intNN_t or
> similar, and use these, but I feel like that's just gratuitous
> ugliness. The userspace type for 64-bit fixed-size fields is
> [u]int64_t and we should be promoting its consistent usage, not
> fragmenting things around kernel uapi mistakes.

I'm not convinced it's a mistake.

> (Perhaps kernel uapi headers should be fixed so that, when __KERNEL__
> is not defined, they include <stdint.h> and define __u64 etc. in terms
> of the stdint types?)

This breaks the existing format specifiers.  The kernel choices have
been deliberately made in such a way that you can use %llu for printing
__u64 values.

Thanks,
Florian

Re: [musl] [PATCH] add statx

[Rich Felker]

On Mon, Feb 17, 2020 at 10:10:40AM +0100, Florian Weimer wrote:
> * Rich Felker:
> 
> >> That's why the kernel always uses unsigned long long for __u64, which
> >> seems a reasonable trade-off to me.  It will make porting to 128-bit
> >> architectures difficult.  But I think we should focus on the
> >> architectures we have today.
> >
> > I disagree strongly with the last sentence. Designing an *API* in a
> > way that's not compatible with anything but long long == 64-bit is bad
> > API design.
> 
> We don't know what LL128 architectures will look like.  For all we know,
> they might have more expressive type descriptors for variadic functions,
> so the whole issue of matching integer types with precise format
> specifiers becomes moot.

I don't follow. Mechanically the wrong format for long long vs
int64_t *already works*; the problem at hand is that per the
language, it's undefined, and rightly produces warnings. None of that
would go away if LL128 archs used a fancy variadic call mechanism.

Rich

[musl] [PATCH 0/1] [musl] [PATCH] add statx

[Duncan Bellamy]

Previous patch was not finished: https://inbox.vuxu.org/musl/20200119121247.37310-1-info@bnoordhuis.nl/

Duncan Bellamy (1):
  resubmitting old statx patch with changes

 include/sys/stat.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 src/stat/fstatat.c | 27 +------------------------
 src/stat/statx.c   | 35 +++++++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+), 26 deletions(-)
 create mode 100644 src/stat/statx.c

-- 
2.34.1

[musl] [PATCH 1/1] resubmitting old statx patch with changes

[Duncan Bellamy]

---
 include/sys/stat.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 src/stat/fstatat.c | 27 +------------------------
 src/stat/statx.c   | 35 +++++++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+), 26 deletions(-)
 create mode 100644 src/stat/statx.c

diff --git a/include/sys/stat.h b/include/sys/stat.h
index 10d446c4..81424462 100644
--- a/include/sys/stat.h
+++ b/include/sys/stat.h
@@ -5,6 +5,7 @@ extern "C" {
 #endif
 
 #include <features.h>
+#include <stdint.h>
 
 #define __NEED_dev_t
 #define __NEED_ino_t
@@ -70,6 +71,54 @@ extern "C" {
 #define UTIME_NOW  0x3fffffff
 #define UTIME_OMIT 0x3ffffffe
 
+#if defined(_GNU_SOURCE)
+#define STATX_TYPE 1U
+#define STATX_MODE 2U
+#define STATX_NLINK 4U
+#define STATX_UID 8U
+#define STATX_GID 0x10U
+#define STATX_ATIME 0x20U
+#define STATX_MTIME 0x40U
+#define STATX_CTIME 0x80U
+#define STATX_INO 0x100U
+#define STATX_SIZE 0x200U
+#define STATX_BLOCKS 0x400U
+#define STATX_BASIC_STATS 0x7ffU
+#define STATX_BTIME 0x800U
+#define STATX_ALL 0xfffU
+
+struct statx_timestamp {
+	int64_t tv_sec;
+	uint32_t tv_nsec;
+	int32_t __pad;
+};
+
+struct statx {
+	uint32_t stx_mask;
+	uint32_t stx_blksize;
+	uint64_t stx_attributes;
+	uint32_t stx_nlink;
+	uint32_t stx_uid;
+	uint32_t stx_gid;
+	uint16_t stx_mode;
+	uint16_t __pad0[1];
+	uint64_t stx_ino;
+	uint64_t stx_size;
+	uint64_t stx_blocks;
+	uint64_t stx_attributes_mask;
+	struct statx_timestamp stx_atime;
+	struct statx_timestamp stx_btime;
+	struct statx_timestamp stx_ctime;
+	struct statx_timestamp stx_mtime;
+	uint32_t stx_rdev_major;
+	uint32_t stx_rdev_minor;
+	uint32_t stx_dev_major;
+	uint32_t stx_dev_minor;
+	uint64_t __pad1[14];
+};
+
+int statx(int, const char *__restrict, int, unsigned, struct statx *__restrict);
+#endif
 int stat(const char *__restrict, struct stat *__restrict);
 int fstat(int, struct stat *);
 int lstat(const char *__restrict, struct stat *__restrict);
diff --git a/src/stat/fstatat.c b/src/stat/fstatat.c
index 74c51cf5..5b2248a9 100644
--- a/src/stat/fstatat.c
+++ b/src/stat/fstatat.c
@@ -7,36 +7,11 @@
 #include <sys/sysmacros.h>
 #include "syscall.h"
 
-struct statx {
-	uint32_t stx_mask;
-	uint32_t stx_blksize;
-	uint64_t stx_attributes;
-	uint32_t stx_nlink;
-	uint32_t stx_uid;
-	uint32_t stx_gid;
-	uint16_t stx_mode;
-	uint16_t pad1;
-	uint64_t stx_ino;
-	uint64_t stx_size;
-	uint64_t stx_blocks;
-	uint64_t stx_attributes_mask;
-	struct {
-		int64_t tv_sec;
-		uint32_t tv_nsec;
-		int32_t pad;
-	} stx_atime, stx_btime, stx_ctime, stx_mtime;
-	uint32_t stx_rdev_major;
-	uint32_t stx_rdev_minor;
-	uint32_t stx_dev_major;
-	uint32_t stx_dev_minor;
-	uint64_t spare[14];
-};
-
 static int fstatat_statx(int fd, const char *restrict path, struct stat *restrict st, int flag)
 {
 	struct statx stx;
 
-	int ret = __syscall(SYS_statx, fd, path, flag, 0x7ff, &stx);
+	int ret = __syscall(SYS_statx, fd, path, flag, STATX_BASIC_STATS, &stx);
 	if (ret) return ret;
 
 	*st = (struct stat){
diff --git a/src/stat/statx.c b/src/stat/statx.c
new file mode 100644
index 00000000..ff49841b
--- /dev/null
+++ b/src/stat/statx.c
@@ -0,0 +1,35 @@
+#define _GNU_SOURCE
+#include <sys/stat.h>
+#include <syscall.h>
+#include <sys/sysmacros.h>
+#include <errno.h>
+
+int statx(int dirfd, const char *restrict path, int flags, unsigned mask, struct statx *restrict stx)
+{
+	int ret = syscall(SYS_statx, dirfd, path, flags, mask, stx);
+	if (ret == ENOSYS) {
+		struct stat st;
+		fstatat(dir_fd, path, &st, flags);
+		stx.stx_dev_major = major(st.st_dev);
+		stx.stx_dev_minor = minor(st.st_dev);
+		stx.stx_ino = st.st_ino;
+		stx.stx_mode = st.st_mode;
+		stx.stx_nlink = st.st_nlink;
+		stx.stx_uid = st.st_uid;
+		stx.stx_gid = st.st_gid;
+		stx.stx_size = st.st_size;
+		stx.stx_blksize = st.st_blksize;
+		stx.stx_blocks = st.st_blocks;
+		stx.stx_atime.tv_sec = st.st_atim.tv_sec;
+		stx.stx_atime.tv_nsec = st.st_atim.tv_nsec;
+		stx.stx_mtime.tv_sec = st.st_mtim.tv_sec;
+		stx.stx_mtime.tv_nsec = st.st_mtim.tv_nsec;
+		stx.stx_ctime.tv_sec = st.st_ctim.tv_sec;
+		stx.stx_ctime.tv_nsec = st.st_ctim.tv_nsec;
+		stx.stx_btime = 0;
+		stx.stx_mask = STATX_BASIC_STATS;
+		ret = EINVAL;
+	}
+
+	return ret;
+}
-- 
2.34.1

Re: [musl] [PATCH 1/1] resubmitting old statx patch with changes

[Rich Felker]

On Sat, Aug 27, 2022 at 03:57:52PM +0100, Duncan Bellamy wrote:
> ---
>  include/sys/stat.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/stat/fstatat.c | 27 +------------------------
>  src/stat/statx.c   | 35 +++++++++++++++++++++++++++++++++
>  3 files changed, 85 insertions(+), 26 deletions(-)
>  create mode 100644 src/stat/statx.c
> 
> diff --git a/include/sys/stat.h b/include/sys/stat.h
> index 10d446c4..81424462 100644
> --- a/include/sys/stat.h
> +++ b/include/sys/stat.h
> @@ -5,6 +5,7 @@ extern "C" {
>  #endif
>  
>  #include <features.h>
> +#include <stdint.h>

This can't be done unconditionally. Either the #include directive
needs to be within the preprocessor conditional where statx is, or the
individual __NEED_uint32_t etc need to be defined conditional on the
same condition before bits/alltypes.h is included. The latter is
probably better here.

>  #define __NEED_dev_t
>  #define __NEED_ino_t
> @@ -70,6 +71,54 @@ extern "C" {
>  #define UTIME_NOW  0x3fffffff
>  #define UTIME_OMIT 0x3ffffffe
>  
> +#if defined(_GNU_SOURCE)
> +#define STATX_TYPE 1U
> +#define STATX_MODE 2U
> +#define STATX_NLINK 4U
> +#define STATX_UID 8U
> +#define STATX_GID 0x10U
> +#define STATX_ATIME 0x20U
> +#define STATX_MTIME 0x40U
> +#define STATX_CTIME 0x80U
> +#define STATX_INO 0x100U
> +#define STATX_SIZE 0x200U
> +#define STATX_BLOCKS 0x400U
> +#define STATX_BASIC_STATS 0x7ffU
> +#define STATX_BTIME 0x800U
> +#define STATX_ALL 0xfffU
> +
> +struct statx_timestamp {
> +	int64_t tv_sec;
> +	uint32_t tv_nsec;
> +	int32_t __pad;
> +};

Minor nit but this could probably just be tv_nsec, __pad (both same
type). This also eliminates the gratuitous need to expose the signed
32-bit type which is not used elsewhere.

I was looking at whether *all* of the types here could be replaced
with equivalent ones that don't require exposing extra types, but the
ones documented as uint64_t probably can't. All the uint32_t in
principle could just be unsigned, and tv_sec time_t, but...

> +
> +struct statx {
> +	uint32_t stx_mask;
> +	uint32_t stx_blksize;
> +	uint64_t stx_attributes;
> +	uint32_t stx_nlink;
> +	uint32_t stx_uid;
> +	uint32_t stx_gid;
> +	uint16_t stx_mode;
> +	uint16_t __pad0[1];
> +	uint64_t stx_ino;
> +	uint64_t stx_size;
> +	uint64_t stx_blocks;
> +	uint64_t stx_attributes_mask;
> +	struct statx_timestamp stx_atime;
> +	struct statx_timestamp stx_btime;
> +	struct statx_timestamp stx_ctime;
> +	struct statx_timestamp stx_mtime;
> +	uint32_t stx_rdev_major;
> +	uint32_t stx_rdev_minor;
> +	uint32_t stx_dev_major;
> +	uint32_t stx_dev_minor;
> +	uint64_t __pad1[14];
> +};

stx_ino etc. should not be assuming ino_t etc. are defined the same as
uint64_t rather than something like unsigned long long. So we probably
just go with writing the types as documented...

> +
> +int statx(int, const char *__restrict, int, unsigned, struct statx *__restrict);
> +#endif
>  int stat(const char *__restrict, struct stat *__restrict);
>  int fstat(int, struct stat *);
>  int lstat(const char *__restrict, struct stat *__restrict);
> diff --git a/src/stat/fstatat.c b/src/stat/fstatat.c
> index 74c51cf5..5b2248a9 100644
> --- a/src/stat/fstatat.c
> +++ b/src/stat/fstatat.c
> @@ -7,36 +7,11 @@
>  #include <sys/sysmacros.h>
>  #include "syscall.h"
>  
> -struct statx {
> -	uint32_t stx_mask;
> -	uint32_t stx_blksize;
> -	uint64_t stx_attributes;
> -	uint32_t stx_nlink;
> -	uint32_t stx_uid;
> -	uint32_t stx_gid;
> -	uint16_t stx_mode;
> -	uint16_t pad1;
> -	uint64_t stx_ino;
> -	uint64_t stx_size;
> -	uint64_t stx_blocks;
> -	uint64_t stx_attributes_mask;
> -	struct {
> -		int64_t tv_sec;
> -		uint32_t tv_nsec;
> -		int32_t pad;
> -	} stx_atime, stx_btime, stx_ctime, stx_mtime;
> -	uint32_t stx_rdev_major;
> -	uint32_t stx_rdev_minor;
> -	uint32_t stx_dev_major;
> -	uint32_t stx_dev_minor;
> -	uint64_t spare[14];
> -};
> -
>  static int fstatat_statx(int fd, const char *restrict path, struct stat *restrict st, int flag)
>  {
>  	struct statx stx;
>  
> -	int ret = __syscall(SYS_statx, fd, path, flag, 0x7ff, &stx);
> +	int ret = __syscall(SYS_statx, fd, path, flag, STATX_BASIC_STATS, &stx);
>  	if (ret) return ret;
>  
>  	*st = (struct stat){

This can be a separate change from adding statx, but it's easy to
separate when merging.

> diff --git a/src/stat/statx.c b/src/stat/statx.c
> new file mode 100644
> index 00000000..ff49841b
> --- /dev/null
> +++ b/src/stat/statx.c
> @@ -0,0 +1,35 @@
> +#define _GNU_SOURCE
> +#include <sys/stat.h>
> +#include <syscall.h>
> +#include <sys/sysmacros.h>
> +#include <errno.h>
> +
> +int statx(int dirfd, const char *restrict path, int flags, unsigned mask, struct statx *restrict stx)
> +{
> +	int ret = syscall(SYS_statx, dirfd, path, flags, mask, stx);
> +	if (ret == ENOSYS) {
> +		struct stat st;
> +		fstatat(dir_fd, path, &st, flags);
> +		stx.stx_dev_major = major(st.st_dev);
> +		stx.stx_dev_minor = minor(st.st_dev);
> +		stx.stx_ino = st.st_ino;
> +		stx.stx_mode = st.st_mode;
> +		stx.stx_nlink = st.st_nlink;
> +		stx.stx_uid = st.st_uid;
> +		stx.stx_gid = st.st_gid;
> +		stx.stx_size = st.st_size;
> +		stx.stx_blksize = st.st_blksize;
> +		stx.stx_blocks = st.st_blocks;
> +		stx.stx_atime.tv_sec = st.st_atim.tv_sec;
> +		stx.stx_atime.tv_nsec = st.st_atim.tv_nsec;
> +		stx.stx_mtime.tv_sec = st.st_mtim.tv_sec;
> +		stx.stx_mtime.tv_nsec = st.st_mtim.tv_nsec;
> +		stx.stx_ctime.tv_sec = st.st_ctim.tv_sec;
> +		stx.stx_ctime.tv_nsec = st.st_ctim.tv_nsec;
> +		stx.stx_btime = 0;
> +		stx.stx_mask = STATX_BASIC_STATS;
> +		ret = EINVAL;
> +	}
> +
> +	return ret;
> +}
> -- 
> 2.34.1

I think this wasn't tested because it won't even compile (stx. instead
of stx->). It's also wrongly assuming syscall() returned a positive
error code rather than -1 on error and setting errno, but you should
be using the __syscall form that returns a negated error code, then
__syscall_ret at the end. I would write it as:

	int ret = __syscall(SYS_statx, dirfd, path, flags, mask, stx);
	if (ret != -ENOSYS) return __syscall_ret(ret);

then the fallback case outside a conditional. The fallback can't
assume fstatat succeeded like you're doing either. It needs to return
-1 immediately if fstatat fails.

All of this code needs to be conditional on SYS_fstatat existing, as
new archs don't have it and only have SYS_statx.

One annoying thing about this but I don't know a good fix; maybe you
have an idea: if SYS_statx fails with ENOSYS, the call to fstatat will
immediately perform SYS_statx again, only to have it fail, then
finally fall back to SYS_fstatat or other syscalls after two failures.
I'm not sure if it makes sense to expose __fstatat_kstat
libc-internally (hidden) to use here or do something else; that's
kinda getting into more complexity around this than I'd like for the
sake of optimizing old systems.

Rich

Re: [musl] [PATCH 1/1] resubmitting old statx patch with changes

[Dunk]

> On 27 Aug 2022, at 19:10, Rich Felker <dalias@libc.org> wrote:
> 
> On Sat, Aug 27, 2022 at 03:57:52PM +0100, Duncan Bellamy wrote:
>> ---
>> include/sys/stat.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>> src/stat/fstatat.c | 27 +------------------------
>> src/stat/statx.c   | 35 +++++++++++++++++++++++++++++++++
>> 3 files changed, 85 insertions(+), 26 deletions(-)
>> create mode 100644 src/stat/statx.c
>> 
>> diff --git a/include/sys/stat.h b/include/sys/stat.h
>> index 10d446c4..81424462 100644
>> --- a/include/sys/stat.h
>> +++ b/include/sys/stat.h
>> @@ -5,6 +5,7 @@ extern "C" {
>> #endif
>> 
>> #include <features.h>
>> +#include <stdint.h>
> 
> This can't be done unconditionally. Either the #include directive
> needs to be within the preprocessor conditional where statx is, or the
> individual __NEED_uint32_t etc need to be defined conditional on the
> same condition before bits/alltypes.h is included. The latter is
> probably better here.

I didn’t understand the later so did the former.

>> #define __NEED_dev_t
>> #define __NEED_ino_t
>> @@ -70,6 +71,54 @@ extern "C" {
>> #define UTIME_NOW  0x3fffffff
>> #define UTIME_OMIT 0x3ffffffe
>> 
>> +#if defined(_GNU_SOURCE)
>> +#define STATX_TYPE 1U
>> +#define STATX_MODE 2U
>> +#define STATX_NLINK 4U
>> +#define STATX_UID 8U
>> +#define STATX_GID 0x10U
>> +#define STATX_ATIME 0x20U
>> +#define STATX_MTIME 0x40U
>> +#define STATX_CTIME 0x80U
>> +#define STATX_INO 0x100U
>> +#define STATX_SIZE 0x200U
>> +#define STATX_BLOCKS 0x400U
>> +#define STATX_BASIC_STATS 0x7ffU
>> +#define STATX_BTIME 0x800U
>> +#define STATX_ALL 0xfffU
>> +
>> +struct statx_timestamp {
>> +    int64_t tv_sec;
>> +    uint32_t tv_nsec;
>> +    int32_t __pad;
>> +};
> 
> Minor nit but this could probably just be tv_nsec, __pad (both same
> type). This also eliminates the gratuitous need to expose the signed
> 32-bit type which is not used elsewhere.

changed

> I was looking at whether *all* of the types here could be replaced
> with equivalent ones that don't require exposing extra types, but the
> ones documented as uint64_t probably can't. All the uint32_t in
> principle could just be unsigned, and tv_sec time_t, but...
> 
>> +
>> +struct statx {
>> +    uint32_t stx_mask;
>> +    uint32_t stx_blksize;
>> +    uint64_t stx_attributes;
>> +    uint32_t stx_nlink;
>> +    uint32_t stx_uid;
>> +    uint32_t stx_gid;
>> +    uint16_t stx_mode;
>> +    uint16_t __pad0[1];
>> +    uint64_t stx_ino;
>> +    uint64_t stx_size;
>> +    uint64_t stx_blocks;
>> +    uint64_t stx_attributes_mask;
>> +    struct statx_timestamp stx_atime;
>> +    struct statx_timestamp stx_btime;
>> +    struct statx_timestamp stx_ctime;
>> +    struct statx_timestamp stx_mtime;
>> +    uint32_t stx_rdev_major;
>> +    uint32_t stx_rdev_minor;
>> +    uint32_t stx_dev_major;
>> +    uint32_t stx_dev_minor;
>> +    uint64_t __pad1[14];
>> +};
> 
> stx_ino etc. should not be assuming ino_t etc. are defined the same as
> uint64_t rather than something like unsigned long long. So we probably
> just go with writing the types as documented...
> 
>> +
>> +int statx(int, const char *__restrict, int, unsigned, struct statx *__restrict);
>> +#endif
>> int stat(const char *__restrict, struct stat *__restrict);
>> int fstat(int, struct stat *);
>> int lstat(const char *__restrict, struct stat *__restrict);
>> diff --git a/src/stat/fstatat.c b/src/stat/fstatat.c
>> index 74c51cf5..5b2248a9 100644
>> --- a/src/stat/fstatat.c
>> +++ b/src/stat/fstatat.c
>> @@ -7,36 +7,11 @@
>> #include <sys/sysmacros.h>
>> #include "syscall.h"
>> 
>> -struct statx {
>> -    uint32_t stx_mask;
>> -    uint32_t stx_blksize;
>> -    uint64_t stx_attributes;
>> -    uint32_t stx_nlink;
>> -    uint32_t stx_uid;
>> -    uint32_t stx_gid;
>> -    uint16_t stx_mode;
>> -    uint16_t pad1;
>> -    uint64_t stx_ino;
>> -    uint64_t stx_size;
>> -    uint64_t stx_blocks;
>> -    uint64_t stx_attributes_mask;
>> -    struct {
>> -        int64_t tv_sec;
>> -        uint32_t tv_nsec;
>> -        int32_t pad;
>> -    } stx_atime, stx_btime, stx_ctime, stx_mtime;
>> -    uint32_t stx_rdev_major;
>> -    uint32_t stx_rdev_minor;
>> -    uint32_t stx_dev_major;
>> -    uint32_t stx_dev_minor;
>> -    uint64_t spare[14];
>> -};
>> -
>> static int fstatat_statx(int fd, const char *restrict path, struct stat *restrict st, int flag)
>> {
>>    struct statx stx;
>> 
>> -    int ret = __syscall(SYS_statx, fd, path, flag, 0x7ff, &stx);
>> +    int ret = __syscall(SYS_statx, fd, path, flag, STATX_BASIC_STATS, &stx);
>>    if (ret) return ret;
>> 
>>    *st = (struct stat){
> 
> This can be a separate change from adding statx, but it's easy to
> separate when merging.

moved to separate commit

>> diff --git a/src/stat/statx.c b/src/stat/statx.c
>> new file mode 100644
>> index 00000000..ff49841b
>> --- /dev/null
>> +++ b/src/stat/statx.c
>> @@ -0,0 +1,35 @@
>> +#define _GNU_SOURCE
>> +#include <sys/stat.h>
>> +#include <syscall.h>
>> +#include <sys/sysmacros.h>
>> +#include <errno.h>
>> +
>> +int statx(int dirfd, const char *restrict path, int flags, unsigned mask, struct statx *restrict stx)
>> +{
>> +    int ret = syscall(SYS_statx, dirfd, path, flags, mask, stx);
>> +    if (ret == ENOSYS) {
>> +        struct stat st;
>> +        fstatat(dir_fd, path, &st, flags);
>> +        stx.stx_dev_major = major(st.st_dev);
>> +        stx.stx_dev_minor = minor(st.st_dev);
>> +        stx.stx_ino = st.st_ino;
>> +        stx.stx_mode = st.st_mode;
>> +        stx.stx_nlink = st.st_nlink;
>> +        stx.stx_uid = st.st_uid;
>> +        stx.stx_gid = st.st_gid;
>> +        stx.stx_size = st.st_size;
>> +        stx.stx_blksize = st.st_blksize;
>> +        stx.stx_blocks = st.st_blocks;
>> +        stx.stx_atime.tv_sec = st.st_atim.tv_sec;
>> +        stx.stx_atime.tv_nsec = st.st_atim.tv_nsec;
>> +        stx.stx_mtime.tv_sec = st.st_mtim.tv_sec;
>> +        stx.stx_mtime.tv_nsec = st.st_mtim.tv_nsec;
>> +        stx.stx_ctime.tv_sec = st.st_ctim.tv_sec;
>> +        stx.stx_ctime.tv_nsec = st.st_ctim.tv_nsec;
>> +        stx.stx_btime = 0;
>> +        stx.stx_mask = STATX_BASIC_STATS;
>> +        ret = EINVAL;
>> +    }
>> +
>> +    return ret;
>> +}
>> -- 
>> 2.34.1
> 
> I think this wasn't tested because it won't even compile (stx. instead
> of stx->). It's also wrongly assuming syscall() returned a positive
> error code rather than -1 on error and setting errno, but you should
> be using the __syscall form that returns a negated error code, then
> __syscall_ret at the end. I would write it as:
> 
>    int ret = __syscall(SYS_statx, dirfd, path, flags, mask, stx);
>    if (ret != -ENOSYS) return __syscall_ret(ret);
> 
> then the fallback case outside a conditional. The fallback can't
> assume fstatat succeeded like you're doing either. It needs to return
> -1 immediately if fstatat fails.

Yes it wasn’t compiled, I copied the old patch and code from `fstatat_statx` for copying the data.  Compiled it locally now.

> 
> All of this code needs to be conditional on SYS_fstatat existing, as
> new archs don't have it and only have SYS_statx.

Changed to this

> One annoying thing about this but I don't know a good fix; maybe you
> have an idea: if SYS_statx fails with ENOSYS, the call to fstatat will
> immediately perform SYS_statx again, only to have it fail, then
> finally fall back to SYS_fstatat or other syscalls after two failures.
> I'm not sure if it makes sense to expose __fstatat_kstat
> libc-internally (hidden) to use here or do something else; that's
> kinda getting into more complexity around this than I'd like for the
> sake of optimizing old systems.

I don’t really understand the fallback mechanism, or why statx would fail and fstatat work on some systems.  Maybe use defines at compile time so either one is used or the other? 

> Rich

Duncan

[musl] [PATCH 0/2] V2

[Duncan Bellamy]

Version 2

Duncan Bellamy (2):
  resubmitting old statx patch with changes
  src/stat/fstatat.c use new statx define

 include/sys/stat.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 src/stat/fstatat.c | 30 +++-------------------------
 src/stat/statx.c   | 39 ++++++++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+), 27 deletions(-)
 create mode 100644 src/stat/statx.c

-- 
2.37.1

[musl] [PATCH 1/2] V2 resubmitting old statx patch with changes

[Duncan Bellamy]

---
 include/sys/stat.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 src/stat/statx.c   | 39 ++++++++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+)
 create mode 100644 src/stat/statx.c

diff --git a/include/sys/stat.h b/include/sys/stat.h
index 10d446c4..21668a51 100644
--- a/include/sys/stat.h
+++ b/include/sys/stat.h
@@ -70,6 +70,55 @@ extern "C" {
 #define UTIME_NOW  0x3fffffff
 #define UTIME_OMIT 0x3ffffffe
 
+#if defined(_GNU_SOURCE)
+#include <stdint.h>
+
+#define STATX_TYPE 1U
+#define STATX_MODE 2U
+#define STATX_NLINK 4U
+#define STATX_UID 8U
+#define STATX_GID 0x10U
+#define STATX_ATIME 0x20U
+#define STATX_MTIME 0x40U
+#define STATX_CTIME 0x80U
+#define STATX_INO 0x100U
+#define STATX_SIZE 0x200U
+#define STATX_BLOCKS 0x400U
+#define STATX_BASIC_STATS 0x7ffU
+#define STATX_BTIME 0x800U
+#define STATX_ALL 0xfffU
+
+struct statx_timestamp {
+	int64_t tv_sec;
+	uint32_t tv_nsec, __pad;
+};
+
+struct statx {
+	uint32_t stx_mask;
+	uint32_t stx_blksize;
+	uint64_t stx_attributes;
+	uint32_t stx_nlink;
+	uint32_t stx_uid;
+	uint32_t stx_gid;
+	uint16_t stx_mode;
+	uint16_t __pad0[1];
+	uint64_t stx_ino;
+	uint64_t stx_size;
+	uint64_t stx_blocks;
+	uint64_t stx_attributes_mask;
+	struct statx_timestamp stx_atime;
+	struct statx_timestamp stx_btime;
+	struct statx_timestamp stx_ctime;
+	struct statx_timestamp stx_mtime;
+	uint32_t stx_rdev_major;
+	uint32_t stx_rdev_minor;
+	uint32_t stx_dev_major;
+	uint32_t stx_dev_minor;
+	uint64_t __pad1[14];
+};
+
+int statx(int, const char *__restrict, int, unsigned, struct statx *__restrict);
+#endif
 int stat(const char *__restrict, struct stat *__restrict);
 int fstat(int, struct stat *);
 int lstat(const char *__restrict, struct stat *__restrict);
diff --git a/src/stat/statx.c b/src/stat/statx.c
new file mode 100644
index 00000000..6788476a
--- /dev/null
+++ b/src/stat/statx.c
@@ -0,0 +1,39 @@
+#define _GNU_SOURCE
+#include <sys/stat.h>
+#include <syscall.h>
+#include <sys/sysmacros.h>
+#include <errno.h>
+
+int statx(int dirfd, const char *restrict path, int flags, unsigned mask, struct statx *restrict stx)
+{
+	int ret = __syscall(SYS_statx, dirfd, path, flags, mask, stx);
+	if (ret != -ENOSYS) return __syscall_ret(ret);
+
+	#if defined(SYS_fstatat)
+	struct stat st;
+	ret = fstatat(dirfd, path, &st, flags);
+	if (ret == -ENOSYS) return -1;
+
+	stx->stx_dev_major = major(st.st_dev);
+	stx->stx_dev_minor = minor(st.st_dev);
+	stx->stx_ino = st.st_ino;
+	stx->stx_mode = st.st_mode;
+	stx->stx_nlink = st.st_nlink;
+	stx->stx_uid = st.st_uid;
+	stx->stx_gid = st.st_gid;
+	stx->stx_size = st.st_size;
+	stx->stx_blksize = st.st_blksize;
+	stx->stx_blocks = st.st_blocks;
+	stx->stx_atime.tv_sec = st.st_atim.tv_sec;
+	stx->stx_atime.tv_nsec = st.st_atim.tv_nsec;
+	stx->stx_mtime.tv_sec = st.st_mtim.tv_sec;
+	stx->stx_mtime.tv_nsec = st.st_mtim.tv_nsec;
+	stx->stx_ctime.tv_sec = st.st_ctim.tv_sec;
+	stx->stx_ctime.tv_nsec = st.st_ctim.tv_nsec;
+	stx->stx_btime = (struct statx_timestamp){.tv_sec=0, .tv_nsec=0};
+	stx->stx_mask = STATX_BASIC_STATS;
+	ret = EINVAL;
+	#endif
+
+	return ret;
+}
-- 
2.37.1

[musl] Re: [PATCH 1/2] V2 resubmitting old statx patch with changes

[Dunk]

> On 28 Aug 2022, at 00:13, Duncan Bellamy <dunk@denkimushi.com> wrote:
> 
> ---
> include/sys/stat.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++
> src/stat/statx.c   | 39 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 88 insertions(+)
> create mode 100644 src/stat/statx.c
> 
> diff --git a/include/sys/stat.h b/include/sys/stat.h
> index 10d446c4..21668a51 100644
> --- a/include/sys/stat.h
> +++ b/include/sys/stat.h
> @@ -70,6 +70,55 @@ extern "C" {
> #define UTIME_NOW  0x3fffffff
> #define UTIME_OMIT 0x3ffffffe
> 
> +#if defined(_GNU_SOURCE)
> +#include <stdint.h>
> +
> +#define STATX_TYPE 1U
> +#define STATX_MODE 2U
> +#define STATX_NLINK 4U
> +#define STATX_UID 8U
> +#define STATX_GID 0x10U
> +#define STATX_ATIME 0x20U
> +#define STATX_MTIME 0x40U
> +#define STATX_CTIME 0x80U
> +#define STATX_INO 0x100U
> +#define STATX_SIZE 0x200U
> +#define STATX_BLOCKS 0x400U
> +#define STATX_BASIC_STATS 0x7ffU
> +#define STATX_BTIME 0x800U
> +#define STATX_ALL 0xfffU
> +
> +struct statx_timestamp {
> +    int64_t tv_sec;
> +    uint32_t tv_nsec, __pad;
> +};
> +
> +struct statx {
> +    uint32_t stx_mask;
> +    uint32_t stx_blksize;
> +    uint64_t stx_attributes;
> +    uint32_t stx_nlink;
> +    uint32_t stx_uid;
> +    uint32_t stx_gid;
> +    uint16_t stx_mode;
> +    uint16_t __pad0[1];
> +    uint64_t stx_ino;
> +    uint64_t stx_size;
> +    uint64_t stx_blocks;
> +    uint64_t stx_attributes_mask;
> +    struct statx_timestamp stx_atime;
> +    struct statx_timestamp stx_btime;
> +    struct statx_timestamp stx_ctime;
> +    struct statx_timestamp stx_mtime;
> +    uint32_t stx_rdev_major;
> +    uint32_t stx_rdev_minor;
> +    uint32_t stx_dev_major;
> +    uint32_t stx_dev_minor;
> +    uint64_t __pad1[14];
> +};
> +
> +int statx(int, const char *__restrict, int, unsigned, struct statx *__restrict);
> +#endif
> int stat(const char *__restrict, struct stat *__restrict);
> int fstat(int, struct stat *);
> int lstat(const char *__restrict, struct stat *__restrict);
> diff --git a/src/stat/statx.c b/src/stat/statx.c
> new file mode 100644
> index 00000000..6788476a
> --- /dev/null
> +++ b/src/stat/statx.c
> @@ -0,0 +1,39 @@
> +#define _GNU_SOURCE
> +#include <sys/stat.h>
> +#include <syscall.h>
> +#include <sys/sysmacros.h>
> +#include <errno.h>
> +
> +int statx(int dirfd, const char *restrict path, int flags, unsigned mask, struct statx *restrict stx)
> +{
> +    int ret = __syscall(SYS_statx, dirfd, path, flags, mask, stx);
> +    if (ret != -ENOSYS) return __syscall_ret(ret);
> +
> +    #if defined(SYS_fstatat)
> +    struct stat st;
> +    ret = fstatat(dirfd, path, &st, flags);
> +    if (ret == -ENOSYS) return -1;
> +
> +    stx->stx_dev_major = major(st.st_dev);
> +    stx->stx_dev_minor = minor(st.st_dev);
> +    stx->stx_ino = st.st_ino;
> +    stx->stx_mode = st.st_mode;
> +    stx->stx_nlink = st.st_nlink;
> +    stx->stx_uid = st.st_uid;
> +    stx->stx_gid = st.st_gid;
> +    stx->stx_size = st.st_size;
> +    stx->stx_blksize = st.st_blksize;
> +    stx->stx_blocks = st.st_blocks;
> +    stx->stx_atime.tv_sec = st.st_atim.tv_sec;
> +    stx->stx_atime.tv_nsec = st.st_atim.tv_nsec;
> +    stx->stx_mtime.tv_sec = st.st_mtim.tv_sec;
> +    stx->stx_mtime.tv_nsec = st.st_mtim.tv_nsec;
> +    stx->stx_ctime.tv_sec = st.st_ctim.tv_sec;
> +    stx->stx_ctime.tv_nsec = st.st_ctim.tv_nsec;
> +    stx->stx_btime = (struct statx_timestamp){.tv_sec=0, .tv_nsec=0};
> +    stx->stx_mask = STATX_BASIC_STATS;
> +    ret = EINVAL;
> +    #endif
> +
> +    return ret;
> +}
> -- 
> 2.37.1

I guess this is missing `return __syscall_ret(ret)` instead of just `return ret` at the end

[musl] [PATCH 2/2] V2 src/stat/fstatat.c use new statx define

[Duncan Bellamy]

---
 src/stat/fstatat.c | 30 +++---------------------------
 1 file changed, 3 insertions(+), 27 deletions(-)

diff --git a/src/stat/fstatat.c b/src/stat/fstatat.c
index 74c51cf5..6b44f48a 100644
--- a/src/stat/fstatat.c
+++ b/src/stat/fstatat.c
@@ -1,4 +1,5 @@
 #define _BSD_SOURCE
+#define _GNU_SOURCE
 #include <sys/stat.h>
 #include <string.h>
 #include <fcntl.h>
@@ -7,36 +8,11 @@
 #include <sys/sysmacros.h>
 #include "syscall.h"
 
-struct statx {
-	uint32_t stx_mask;
-	uint32_t stx_blksize;
-	uint64_t stx_attributes;
-	uint32_t stx_nlink;
-	uint32_t stx_uid;
-	uint32_t stx_gid;
-	uint16_t stx_mode;
-	uint16_t pad1;
-	uint64_t stx_ino;
-	uint64_t stx_size;
-	uint64_t stx_blocks;
-	uint64_t stx_attributes_mask;
-	struct {
-		int64_t tv_sec;
-		uint32_t tv_nsec;
-		int32_t pad;
-	} stx_atime, stx_btime, stx_ctime, stx_mtime;
-	uint32_t stx_rdev_major;
-	uint32_t stx_rdev_minor;
-	uint32_t stx_dev_major;
-	uint32_t stx_dev_minor;
-	uint64_t spare[14];
-};
-
 static int fstatat_statx(int fd, const char *restrict path, struct stat *restrict st, int flag)
 {
 	struct statx stx;
 
-	int ret = __syscall(SYS_statx, fd, path, flag, 0x7ff, &stx);
+	int ret = __syscall(SYS_statx, fd, path, flag, STATX_BASIC_STATS, &stx);
 	if (ret) return ret;
 
 	*st = (struct stat){
@@ -152,6 +128,6 @@ int __fstatat(int fd, const char *restrict path, struct stat *restrict st, int f
 
 weak_alias(__fstatat, fstatat);
 
-#if !_REDIR_TIME64
+#if !_REDIR_TIME64 && !defined(_GNU_SOURCE)
 weak_alias(fstatat, fstatat64);
 #endif
-- 
2.37.1

[musl] [PATCH 0/2] V3

[Duncan Bellamy]

Version 3

Duncan Bellamy (2):
  resubmitting old statx patch with changes
  src/stat/fstatat.c use new statx define

 include/sys/stat.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 src/stat/fstatat.c | 30 +++-------------------------
 src/stat/statx.c   | 40 +++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+), 27 deletions(-)
 create mode 100644 src/stat/statx.c

-- 
2.32.1 (Apple Git-133)

[musl] [PATCH 1/2] V3 resubmitting old statx patch with changes

[Duncan Bellamy]

---
 include/sys/stat.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 src/stat/statx.c   | 40 +++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)
 create mode 100644 src/stat/statx.c

diff --git a/include/sys/stat.h b/include/sys/stat.h
index 10d446c4..21668a51 100644
--- a/include/sys/stat.h
+++ b/include/sys/stat.h
@@ -70,6 +70,55 @@ extern "C" {
 #define UTIME_NOW  0x3fffffff
 #define UTIME_OMIT 0x3ffffffe
 
+#if defined(_GNU_SOURCE)
+#include <stdint.h>
+
+#define STATX_TYPE 1U
+#define STATX_MODE 2U
+#define STATX_NLINK 4U
+#define STATX_UID 8U
+#define STATX_GID 0x10U
+#define STATX_ATIME 0x20U
+#define STATX_MTIME 0x40U
+#define STATX_CTIME 0x80U
+#define STATX_INO 0x100U
+#define STATX_SIZE 0x200U
+#define STATX_BLOCKS 0x400U
+#define STATX_BASIC_STATS 0x7ffU
+#define STATX_BTIME 0x800U
+#define STATX_ALL 0xfffU
+
+struct statx_timestamp {
+	int64_t tv_sec;
+	uint32_t tv_nsec, __pad;
+};
+
+struct statx {
+	uint32_t stx_mask;
+	uint32_t stx_blksize;
+	uint64_t stx_attributes;
+	uint32_t stx_nlink;
+	uint32_t stx_uid;
+	uint32_t stx_gid;
+	uint16_t stx_mode;
+	uint16_t __pad0[1];
+	uint64_t stx_ino;
+	uint64_t stx_size;
+	uint64_t stx_blocks;
+	uint64_t stx_attributes_mask;
+	struct statx_timestamp stx_atime;
+	struct statx_timestamp stx_btime;
+	struct statx_timestamp stx_ctime;
+	struct statx_timestamp stx_mtime;
+	uint32_t stx_rdev_major;
+	uint32_t stx_rdev_minor;
+	uint32_t stx_dev_major;
+	uint32_t stx_dev_minor;
+	uint64_t __pad1[14];
+};
+
+int statx(int, const char *__restrict, int, unsigned, struct statx *__restrict);
+#endif
 int stat(const char *__restrict, struct stat *__restrict);
 int fstat(int, struct stat *);
 int lstat(const char *__restrict, struct stat *__restrict);
diff --git a/src/stat/statx.c b/src/stat/statx.c
new file mode 100644
index 00000000..4eae0d56
--- /dev/null
+++ b/src/stat/statx.c
@@ -0,0 +1,40 @@
+#define _GNU_SOURCE
+#include <sys/stat.h>
+#include <syscall.h>
+#include <sys/sysmacros.h>
+#include <errno.h>
+
+int statx(int dirfd, const char *restrict path, int flags, unsigned mask, struct statx *restrict stx)
+{
+	int ret = __syscall(SYS_statx, dirfd, path, flags, mask, stx);
+
+	#if defined(SYS_fstatat)
+	if (ret != -ENOSYS) return __syscall_ret(ret);
+	struct stat st;
+	ret = fstatat(dirfd, path, &st, flags);
+	if (ret == -ENOSYS) return -1;
+
+	stx->stx_dev_major = major(st.st_dev);
+	stx->stx_dev_minor = minor(st.st_dev);
+	stx->stx_ino = st.st_ino;
+	stx->stx_mode = st.st_mode;
+	stx->stx_nlink = st.st_nlink;
+	stx->stx_uid = st.st_uid;
+	stx->stx_gid = st.st_gid;
+	stx->stx_size = st.st_size;
+	stx->stx_blksize = st.st_blksize;
+	stx->stx_blocks = st.st_blocks;
+	stx->stx_atime.tv_sec = st.st_atim.tv_sec;
+	stx->stx_atime.tv_nsec = st.st_atim.tv_nsec;
+	stx->stx_mtime.tv_sec = st.st_mtim.tv_sec;
+	stx->stx_mtime.tv_nsec = st.st_mtim.tv_nsec;
+	stx->stx_ctime.tv_sec = st.st_ctim.tv_sec;
+	stx->stx_ctime.tv_nsec = st.st_ctim.tv_nsec;
+	stx->stx_btime = (struct statx_timestamp){.tv_sec=0, .tv_nsec=0};
+	stx->stx_mask = STATX_BASIC_STATS;
+	ret = EINVAL;
+	return ret;
+	#else
+	return __syscall_ret(ret);
+	#endif
+}
-- 
2.32.1 (Apple Git-133)

Re: [musl] [PATCH 1/2] V3 resubmitting old statx patch with changes

[Rich Felker]

On Wed, Aug 31, 2022 at 08:07:34PM +0100, Duncan Bellamy wrote:
> ---
>  include/sys/stat.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/stat/statx.c   | 40 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+)
>  create mode 100644 src/stat/statx.c

I'm picking back up review of this because it's been requested again.
Last time I dropped off because there continued to be unresolved
issues where it looked like this hadn't been tested.

I'll make the following changes and merge if there's no objection:

> diff --git a/include/sys/stat.h b/include/sys/stat.h
> index 10d446c4..21668a51 100644
> --- a/include/sys/stat.h
> +++ b/include/sys/stat.h
> @@ -70,6 +70,55 @@ extern "C" {
>  #define UTIME_NOW  0x3fffffff
>  #define UTIME_OMIT 0x3ffffffe
>  
> +#if defined(_GNU_SOURCE)
> +#include <stdint.h>

This should be moved above include of bits/alltypes.h as:

#ifdef _GNU_SOURCE
#define #define __NEED_int64_t
#define #define __NEED_uint62_t
#define #define __NEED_uint32_t
#endif

> +#define STATX_TYPE 1U
> +#define STATX_MODE 2U
> +#define STATX_NLINK 4U
> +#define STATX_UID 8U
> +#define STATX_GID 0x10U
> +#define STATX_ATIME 0x20U
> +#define STATX_MTIME 0x40U
> +#define STATX_CTIME 0x80U
> +#define STATX_INO 0x100U
> +#define STATX_SIZE 0x200U
> +#define STATX_BLOCKS 0x400U
> +#define STATX_BASIC_STATS 0x7ffU
> +#define STATX_BTIME 0x800U
> +#define STATX_ALL 0xfffU
> +
> +struct statx_timestamp {
> +	int64_t tv_sec;
> +	uint32_t tv_nsec, __pad;
> +};
> +
> +struct statx {
> +	uint32_t stx_mask;
> +	uint32_t stx_blksize;
> +	uint64_t stx_attributes;
> +	uint32_t stx_nlink;
> +	uint32_t stx_uid;
> +	uint32_t stx_gid;
> +	uint16_t stx_mode;
> +	uint16_t __pad0[1];
> +	uint64_t stx_ino;
> +	uint64_t stx_size;
> +	uint64_t stx_blocks;
> +	uint64_t stx_attributes_mask;
> +	struct statx_timestamp stx_atime;
> +	struct statx_timestamp stx_btime;
> +	struct statx_timestamp stx_ctime;
> +	struct statx_timestamp stx_mtime;
> +	uint32_t stx_rdev_major;
> +	uint32_t stx_rdev_minor;
> +	uint32_t stx_dev_major;
> +	uint32_t stx_dev_minor;
> +	uint64_t __pad1[14];
> +};
> +
> +int statx(int, const char *__restrict, int, unsigned, struct statx *__restrict);
> +#endif

This should probably be reordered to go with the other extensions, but
it's not really a big deal, that's just usually how the headers are
organized.

>  int stat(const char *__restrict, struct stat *__restrict);
>  int fstat(int, struct stat *);
>  int lstat(const char *__restrict, struct stat *__restrict);
> diff --git a/src/stat/statx.c b/src/stat/statx.c
> new file mode 100644
> index 00000000..4eae0d56
> --- /dev/null
> +++ b/src/stat/statx.c
> @@ -0,0 +1,40 @@
> +#define _GNU_SOURCE
> +#include <sys/stat.h>
> +#include <syscall.h>
> +#include <sys/sysmacros.h>
> +#include <errno.h>
> +
> +int statx(int dirfd, const char *restrict path, int flags, unsigned mask, struct statx *restrict stx)
> +{
> +	int ret = __syscall(SYS_statx, dirfd, path, flags, mask, stx);
> +
> +	#if defined(SYS_fstatat)
> +	if (ret != -ENOSYS) return __syscall_ret(ret);
> +	struct stat st;
> +	ret = fstatat(dirfd, path, &st, flags);
> +	if (ret == -ENOSYS) return -1;

fstatat returns -1 on error and sets errno. The condition also makes
no sense. The intent here is to return without writing to *stx when
fstatat failed. This is not specific to ENOSYS (which can't happen, at
least not without bogus seccomp meddling) but could happen for lots of
reasons. I think it should just be:

	if (ret) return ret;

> +	stx->stx_dev_major = major(st.st_dev);
> +	stx->stx_dev_minor = minor(st.st_dev);
> +	stx->stx_ino = st.st_ino;
> +	stx->stx_mode = st.st_mode;
> +	stx->stx_nlink = st.st_nlink;
> +	stx->stx_uid = st.st_uid;
> +	stx->stx_gid = st.st_gid;
> +	stx->stx_size = st.st_size;
> +	stx->stx_blksize = st.st_blksize;
> +	stx->stx_blocks = st.st_blocks;
> +	stx->stx_atime.tv_sec = st.st_atim.tv_sec;
> +	stx->stx_atime.tv_nsec = st.st_atim.tv_nsec;
> +	stx->stx_mtime.tv_sec = st.st_mtim.tv_sec;
> +	stx->stx_mtime.tv_nsec = st.st_mtim.tv_nsec;
> +	stx->stx_ctime.tv_sec = st.st_ctim.tv_sec;
> +	stx->stx_ctime.tv_nsec = st.st_ctim.tv_nsec;
> +	stx->stx_btime = (struct statx_timestamp){.tv_sec=0, .tv_nsec=0};
> +	stx->stx_mask = STATX_BASIC_STATS;

This is probably all fine. Rather than explicitly filling btime with
zeros, it might make sense to memset the whole struct zero first so
there's no stack data leak in unsupported extension fields.

> +	ret = EINVAL;
> +	return ret;

This is definitely wrong. statx returns 0 on success or -1 on failure,
never a positive value like EINVAL. I'm not even sure what the intent
was here..?

> +	#else
> +	return __syscall_ret(ret);
> +	#endif
> +}

This looks ok, but it would probably make sense to reverse the #ifdef
to #ifndef and flip the order in the file, so the return code using
negated error number like this is up with the __syscall it goes with
rather than separated. As written it's hard to see that the right
thing is being done in the path that uses errno vs the one using
negated error codes. (Admittedly, this being mishandled and needing to
review it in more detail is probably what made me think of this.)

Rich

Re: [musl] [PATCH 1/2] V3 resubmitting old statx patch with changes

[Rich Felker]

On Sat, Feb 24, 2024 at 11:56:32AM -0500, Rich Felker wrote:
> On Wed, Aug 31, 2022 at 08:07:34PM +0100, Duncan Bellamy wrote:
> > ---
> >  include/sys/stat.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++
> >  src/stat/statx.c   | 40 +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 89 insertions(+)
> >  create mode 100644 src/stat/statx.c
> 
> I'm picking back up review of this because it's been requested again.
> Last time I dropped off because there continued to be unresolved
> issues where it looked like this hadn't been tested.
> 
> I'll make the following changes and merge if there's no objection:
> 
> > diff --git a/include/sys/stat.h b/include/sys/stat.h
> > index 10d446c4..21668a51 100644
> > --- a/include/sys/stat.h
> > +++ b/include/sys/stat.h
> > @@ -70,6 +70,55 @@ extern "C" {
> >  #define UTIME_NOW  0x3fffffff
> >  #define UTIME_OMIT 0x3ffffffe
> >  
> > +#if defined(_GNU_SOURCE)
> > +#include <stdint.h>
> 
> This should be moved above include of bits/alltypes.h as:
> 
> #ifdef _GNU_SOURCE
> #define #define __NEED_int64_t
> #define #define __NEED_uint62_t
> #define #define __NEED_uint32_t
> #endif
> 
> > +#define STATX_TYPE 1U
> > +#define STATX_MODE 2U
> > +#define STATX_NLINK 4U
> > +#define STATX_UID 8U
> > +#define STATX_GID 0x10U
> > +#define STATX_ATIME 0x20U
> > +#define STATX_MTIME 0x40U
> > +#define STATX_CTIME 0x80U
> > +#define STATX_INO 0x100U
> > +#define STATX_SIZE 0x200U
> > +#define STATX_BLOCKS 0x400U
> > +#define STATX_BASIC_STATS 0x7ffU
> > +#define STATX_BTIME 0x800U
> > +#define STATX_ALL 0xfffU
> > +
> > +struct statx_timestamp {
> > +	int64_t tv_sec;
> > +	uint32_t tv_nsec, __pad;
> > +};
> > +
> > +struct statx {
> > +	uint32_t stx_mask;
> > +	uint32_t stx_blksize;
> > +	uint64_t stx_attributes;
> > +	uint32_t stx_nlink;
> > +	uint32_t stx_uid;
> > +	uint32_t stx_gid;
> > +	uint16_t stx_mode;
> > +	uint16_t __pad0[1];
> > +	uint64_t stx_ino;
> > +	uint64_t stx_size;
> > +	uint64_t stx_blocks;
> > +	uint64_t stx_attributes_mask;
> > +	struct statx_timestamp stx_atime;
> > +	struct statx_timestamp stx_btime;
> > +	struct statx_timestamp stx_ctime;
> > +	struct statx_timestamp stx_mtime;
> > +	uint32_t stx_rdev_major;
> > +	uint32_t stx_rdev_minor;
> > +	uint32_t stx_dev_major;
> > +	uint32_t stx_dev_minor;
> > +	uint64_t __pad1[14];
> > +};
> > +
> > +int statx(int, const char *__restrict, int, unsigned, struct statx *__restrict);
> > +#endif
> 
> This should probably be reordered to go with the other extensions, but
> it's not really a big deal, that's just usually how the headers are
> organized.
> 
> >  int stat(const char *__restrict, struct stat *__restrict);
> >  int fstat(int, struct stat *);
> >  int lstat(const char *__restrict, struct stat *__restrict);
> > diff --git a/src/stat/statx.c b/src/stat/statx.c
> > new file mode 100644
> > index 00000000..4eae0d56
> > --- /dev/null
> > +++ b/src/stat/statx.c
> > @@ -0,0 +1,40 @@
> > +#define _GNU_SOURCE
> > +#include <sys/stat.h>
> > +#include <syscall.h>
> > +#include <sys/sysmacros.h>
> > +#include <errno.h>
> > +
> > +int statx(int dirfd, const char *restrict path, int flags, unsigned mask, struct statx *restrict stx)
> > +{
> > +	int ret = __syscall(SYS_statx, dirfd, path, flags, mask, stx);
> > +
> > +	#if defined(SYS_fstatat)
> > +	if (ret != -ENOSYS) return __syscall_ret(ret);
> > +	struct stat st;
> > +	ret = fstatat(dirfd, path, &st, flags);
> > +	if (ret == -ENOSYS) return -1;
> 
> fstatat returns -1 on error and sets errno. The condition also makes
> no sense. The intent here is to return without writing to *stx when
> fstatat failed. This is not specific to ENOSYS (which can't happen, at
> least not without bogus seccomp meddling) but could happen for lots of
> reasons. I think it should just be:
> 
> 	if (ret) return ret;
> 
> > +	stx->stx_dev_major = major(st.st_dev);
> > +	stx->stx_dev_minor = minor(st.st_dev);
> > +	stx->stx_ino = st.st_ino;
> > +	stx->stx_mode = st.st_mode;
> > +	stx->stx_nlink = st.st_nlink;
> > +	stx->stx_uid = st.st_uid;
> > +	stx->stx_gid = st.st_gid;
> > +	stx->stx_size = st.st_size;
> > +	stx->stx_blksize = st.st_blksize;
> > +	stx->stx_blocks = st.st_blocks;
> > +	stx->stx_atime.tv_sec = st.st_atim.tv_sec;
> > +	stx->stx_atime.tv_nsec = st.st_atim.tv_nsec;
> > +	stx->stx_mtime.tv_sec = st.st_mtim.tv_sec;
> > +	stx->stx_mtime.tv_nsec = st.st_mtim.tv_nsec;
> > +	stx->stx_ctime.tv_sec = st.st_ctim.tv_sec;
> > +	stx->stx_ctime.tv_nsec = st.st_ctim.tv_nsec;
> > +	stx->stx_btime = (struct statx_timestamp){.tv_sec=0, .tv_nsec=0};
> > +	stx->stx_mask = STATX_BASIC_STATS;
> 
> This is probably all fine. Rather than explicitly filling btime with
> zeros, it might make sense to memset the whole struct zero first so
> there's no stack data leak in unsupported extension fields.
> 
> > +	ret = EINVAL;
> > +	return ret;
> 
> This is definitely wrong. statx returns 0 on success or -1 on failure,
> never a positive value like EINVAL. I'm not even sure what the intent
> was here..?
> 
> > +	#else
> > +	return __syscall_ret(ret);
> > +	#endif
> > +}
> 
> This looks ok, but it would probably make sense to reverse the #ifdef
> to #ifndef and flip the order in the file, so the return code using
> negated error number like this is up with the __syscall it goes with
> rather than separated. As written it's hard to see that the right
> thing is being done in the path that uses errno vs the one using
> negated error codes. (Admittedly, this being mishandled and needing to
> review it in more detail is probably what made me think of this.)

It looks like this failed to fill the stx_attributes[_mask] fields,
leaving them uninitialized, in the fallback for pre-statx kernels.
This needs to be fixed. What should be done with them?

Rich

Re: [musl] [PATCH 1/2] V3 resubmitting old statx patch with changes

[lolzery wowzery]

Hi all! I'm new to git over email, new to musl, and I want to really
get into contributing to musl. So, I'd really appreciate any
tips/comments/info for a newbie like me!

Please do not merge these changes yet. There's numerous problems with
this code above and beyond the issues you pointed out, and each of
those issues led me down successive rabbit holes all over the musl
source code and I've ended up opening a sizable can of worms,
including io race condition bugs in fallback conditions, symbol
weakness problems, header/struct misorganization, and bugs in syscall
support detection making musl silently/unexpectedly fail under certain
seccomp configurations.

Also, let me cite the earlier patch from tony.ambardar@gmail.com about
renameat2. My patch will include a full proper renameat2
implementation with validation and fallback and will compile on
systems where SYS_renameat2 is undefined (using exclusively the
fallback.)

I recognize I probably sound like an over-eager-beaver fussing over
fools-gold as I'm new and none of you know me. (That's the assumption
I myself would make if a new guy showed up claiming to have lots of
bug fixes and stuff.) So, please suspend your disbelief for a short
while until I can give my full report and all fixes tomorrow for
everyone to review. Give me a chance to prove my commitment to musl
and watch as I uphold my pledge to maintain my areas of the code in
musl and keep them ship-shape.

Have a great day everyone and really looking forwards to sharing my
full report tomorrow!,
Jack


On Wed, Apr 24, 2024 at 3:30 PM Rich Felker <dalias@libc.org> wrote:
>
> On Sat, Feb 24, 2024 at 11:56:32AM -0500, Rich Felker wrote:
> > On Wed, Aug 31, 2022 at 08:07:34PM +0100, Duncan Bellamy wrote:
> > > ---
> > >  include/sys/stat.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  src/stat/statx.c   | 40 +++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 89 insertions(+)
> > >  create mode 100644 src/stat/statx.c
> >
> > I'm picking back up review of this because it's been requested again.
> > Last time I dropped off because there continued to be unresolved
> > issues where it looked like this hadn't been tested.
> >
> > I'll make the following changes and merge if there's no objection:
> >
> > > diff --git a/include/sys/stat.h b/include/sys/stat.h
> > > index 10d446c4..21668a51 100644
> > > --- a/include/sys/stat.h
> > > +++ b/include/sys/stat.h
> > > @@ -70,6 +70,55 @@ extern "C" {
> > >  #define UTIME_NOW  0x3fffffff
> > >  #define UTIME_OMIT 0x3ffffffe
> > >
> > > +#if defined(_GNU_SOURCE)
> > > +#include <stdint.h>
> >
> > This should be moved above include of bits/alltypes.h as:
> >
> > #ifdef _GNU_SOURCE
> > #define #define __NEED_int64_t
> > #define #define __NEED_uint62_t
> > #define #define __NEED_uint32_t
> > #endif
> >
> > > +#define STATX_TYPE 1U
> > > +#define STATX_MODE 2U
> > > +#define STATX_NLINK 4U
> > > +#define STATX_UID 8U
> > > +#define STATX_GID 0x10U
> > > +#define STATX_ATIME 0x20U
> > > +#define STATX_MTIME 0x40U
> > > +#define STATX_CTIME 0x80U
> > > +#define STATX_INO 0x100U
> > > +#define STATX_SIZE 0x200U
> > > +#define STATX_BLOCKS 0x400U
> > > +#define STATX_BASIC_STATS 0x7ffU
> > > +#define STATX_BTIME 0x800U
> > > +#define STATX_ALL 0xfffU
> > > +
> > > +struct statx_timestamp {
> > > +   int64_t tv_sec;
> > > +   uint32_t tv_nsec, __pad;
> > > +};
> > > +
> > > +struct statx {
> > > +   uint32_t stx_mask;
> > > +   uint32_t stx_blksize;
> > > +   uint64_t stx_attributes;
> > > +   uint32_t stx_nlink;
> > > +   uint32_t stx_uid;
> > > +   uint32_t stx_gid;
> > > +   uint16_t stx_mode;
> > > +   uint16_t __pad0[1];
> > > +   uint64_t stx_ino;
> > > +   uint64_t stx_size;
> > > +   uint64_t stx_blocks;
> > > +   uint64_t stx_attributes_mask;
> > > +   struct statx_timestamp stx_atime;
> > > +   struct statx_timestamp stx_btime;
> > > +   struct statx_timestamp stx_ctime;
> > > +   struct statx_timestamp stx_mtime;
> > > +   uint32_t stx_rdev_major;
> > > +   uint32_t stx_rdev_minor;
> > > +   uint32_t stx_dev_major;
> > > +   uint32_t stx_dev_minor;
> > > +   uint64_t __pad1[14];
> > > +};
> > > +
> > > +int statx(int, const char *__restrict, int, unsigned, struct statx *__restrict);
> > > +#endif
> >
> > This should probably be reordered to go with the other extensions, but
> > it's not really a big deal, that's just usually how the headers are
> > organized.
> >
> > >  int stat(const char *__restrict, struct stat *__restrict);
> > >  int fstat(int, struct stat *);
> > >  int lstat(const char *__restrict, struct stat *__restrict);
> > > diff --git a/src/stat/statx.c b/src/stat/statx.c
> > > new file mode 100644
> > > index 00000000..4eae0d56
> > > --- /dev/null
> > > +++ b/src/stat/statx.c
> > > @@ -0,0 +1,40 @@
> > > +#define _GNU_SOURCE
> > > +#include <sys/stat.h>
> > > +#include <syscall.h>
> > > +#include <sys/sysmacros.h>
> > > +#include <errno.h>
> > > +
> > > +int statx(int dirfd, const char *restrict path, int flags, unsigned mask, struct statx *restrict stx)
> > > +{
> > > +   int ret = __syscall(SYS_statx, dirfd, path, flags, mask, stx);
> > > +
> > > +   #if defined(SYS_fstatat)
> > > +   if (ret != -ENOSYS) return __syscall_ret(ret);
> > > +   struct stat st;
> > > +   ret = fstatat(dirfd, path, &st, flags);
> > > +   if (ret == -ENOSYS) return -1;
> >
> > fstatat returns -1 on error and sets errno. The condition also makes
> > no sense. The intent here is to return without writing to *stx when
> > fstatat failed. This is not specific to ENOSYS (which can't happen, at
> > least not without bogus seccomp meddling) but could happen for lots of
> > reasons. I think it should just be:
> >
> >       if (ret) return ret;
> >
> > > +   stx->stx_dev_major = major(st.st_dev);
> > > +   stx->stx_dev_minor = minor(st.st_dev);
> > > +   stx->stx_ino = st.st_ino;
> > > +   stx->stx_mode = st.st_mode;
> > > +   stx->stx_nlink = st.st_nlink;
> > > +   stx->stx_uid = st.st_uid;
> > > +   stx->stx_gid = st.st_gid;
> > > +   stx->stx_size = st.st_size;
> > > +   stx->stx_blksize = st.st_blksize;
> > > +   stx->stx_blocks = st.st_blocks;
> > > +   stx->stx_atime.tv_sec = st.st_atim.tv_sec;
> > > +   stx->stx_atime.tv_nsec = st.st_atim.tv_nsec;
> > > +   stx->stx_mtime.tv_sec = st.st_mtim.tv_sec;
> > > +   stx->stx_mtime.tv_nsec = st.st_mtim.tv_nsec;
> > > +   stx->stx_ctime.tv_sec = st.st_ctim.tv_sec;
> > > +   stx->stx_ctime.tv_nsec = st.st_ctim.tv_nsec;
> > > +   stx->stx_btime = (struct statx_timestamp){.tv_sec=0, .tv_nsec=0};
> > > +   stx->stx_mask = STATX_BASIC_STATS;
> >
> > This is probably all fine. Rather than explicitly filling btime with
> > zeros, it might make sense to memset the whole struct zero first so
> > there's no stack data leak in unsupported extension fields.
> >
> > > +   ret = EINVAL;
> > > +   return ret;
> >
> > This is definitely wrong. statx returns 0 on success or -1 on failure,
> > never a positive value like EINVAL. I'm not even sure what the intent
> > was here..?
> >
> > > +   #else
> > > +   return __syscall_ret(ret);
> > > +   #endif
> > > +}
> >
> > This looks ok, but it would probably make sense to reverse the #ifdef
> > to #ifndef and flip the order in the file, so the return code using
> > negated error number like this is up with the __syscall it goes with
> > rather than separated. As written it's hard to see that the right
> > thing is being done in the path that uses errno vs the one using
> > negated error codes. (Admittedly, this being mishandled and needing to
> > review it in more detail is probably what made me think of this.)
>
> It looks like this failed to fill the stx_attributes[_mask] fields,
> leaving them uninitialized, in the fallback for pre-statx kernels.
> This needs to be fixed. What should be done with them?
>
> Rich

Re: [musl] [PATCH 1/2] V3 resubmitting old statx patch with changes

[Markus Wichmann]

Am Wed, Apr 24, 2024 at 07:55:32PM -0400 schrieb lolzery wowzery:
> Hi all! I'm new to git over email, new to musl, and I want to really
> get into contributing to musl. So, I'd really appreciate any
> tips/comments/info for a newbie like me!
>

Here's one: Please don't top-post. Another one: Are you the same as the
OP in this thread? Because I don't seem to have that message, but Rich
was quoting someone named "Duncan Bellamy", and that is not the name you
put on this message.

> Please do not merge these changes yet. There's numerous problems with
> this code above and beyond the issues you pointed out, and each of
> those issues led me down successive rabbit holes all over the musl
> source code and I've ended up opening a sizable can of worms,
> including io race condition bugs in fallback conditions, symbol
> weakness problems, header/struct misorganization, and bugs in syscall
> support detection making musl silently/unexpectedly fail under certain
> seccomp configurations.
>

This is called scope creep, and you should not do it. Fix one issue at a
time. If there are problems, report them or send patches one at a time.
Especially when you don't know what to do, you might find that the issue
you are worried about is not a problem at all when you just speak to
people about it.

> Also, let me cite the earlier patch from tony.ambardar@gmail.com about
> renameat2. My patch will include a full proper renameat2
> implementation with validation and fallback and will compile on
> systems where SYS_renameat2 is undefined (using exclusively the
> fallback.)
>

SYS_renameat2 is defined in every architecture musl supports. You have
created code that will not be used. I don't think this is a good thing.

Generally, new system calls are added to all architectures, though it
may take time. Therefore fallbacks for them not being defined are
typically useless. And renameat2 is older than seccomp, so not even
particularly new.

Ciao,
Markus

Re: [musl] [PATCH 1/2] V3 resubmitting old statx patch with changes

[Rich Felker]

On Wed, Apr 24, 2024 at 07:55:32PM -0400, lolzery wowzery wrote:
> Hi all! I'm new to git over email, new to musl, and I want to really
> get into contributing to musl. So, I'd really appreciate any
> tips/comments/info for a newbie like me!
> 
> Please do not merge these changes yet. There's numerous problems with
> this code above and beyond the issues you pointed out,

statx is already in musl 1.2.5 (commit b817541f1cfd). If there are
other problems than the ones I fixed when merging it and the
stx_attributes field, please report them here rather than spending
time trying to make a comprehensive changeset for a lot of things that
might or might not actually be wrong.

> and each of
> those issues led me down successive rabbit holes all over the musl
> source code and I've ended up opening a sizable can of worms,
> including io race condition bugs in fallback conditions,

Can you clarify what you mean? There are some places where correctly
atomic fallback is impossible and the fallback is best-effort only.
This is generally only for missing O_CLOEXEC type functionality. If
there are others, please report.

> symbol
> weakness problems,

This sounds unlikely. It's more likely that you misunderstand how/why
they're used. But I'm happy to look at your findings.

> header/struct misorganization, and bugs in syscall
> support detection making musl silently/unexpectedly fail under certain
> seccomp configurations.

It's expected to fail catastrophically if seccomp filters modify
syscall behavior in ways that make forward progress impossible (e.g.
blocking close or something) or that lie about the cause of failure
(the classic but badly wrong practice of using EPERM where ENOSYS was
the correct code for syscalls not available in the sandbox). This is
inherently not a supportable configuration.

Generally musl always uses the oldest-possible syscall that can fully
meet the requirements. If it has to try a new syscall first (like
statx on 32-bit, since the legacy syscalls don't support full-range
time_t and we don't know until after stat whether the times fit in 32
bits), it falls back on ENOSYS. So as long as seccomp filters are
using semantically correct error codes, things should generally work.
But it's the responsibility of the seccomp filter authors to ensure
they're doing this right.

> Also, let me cite the earlier patch from tony.ambardar@gmail.com about
> renameat2. My patch will include a full proper renameat2
> implementation with validation and fallback and will compile on
> systems where SYS_renameat2 is undefined (using exclusively the
> fallback.)

It's never undefined. That's not how this works.

If you're making out-of-tree bare-metal ports and don't want the
overhead of having to add new syscalls like this, you can do something
like define the SYS_* macros for them such that syscall_arch.h can
statically catch that they're nonexistant (e.g. by given them values
in some high range) and directly return -ENOSYS; then the code would
collapse down as you want with the ENOSYS path being always-taken.

Note that there is no way to emulate the nonzero flags for renameat2
without race conditions. Since this is not standard/mandatory
functionality, the right thing to do is just return an error for
"unsupported flags", not try to emulate them.

> I recognize I probably sound like an over-eager-beaver fussing over
> fools-gold as I'm new and none of you know me. (That's the assumption
> I myself would make if a new guy showed up claiming to have lots of
> bug fixes and stuff.) So, please suspend your disbelief for a short
> while until I can give my full report and all fixes tomorrow for
> everyone to review. Give me a chance to prove my commitment to musl
> and watch as I uphold my pledge to maintain my areas of the code in
> musl and keep them ship-shape.
> 
> Have a great day everyone and really looking forwards to sharing my
> full report tomorrow!,

OK, please send.

Rich

Re: [musl] [PATCH 1/2] V3 resubmitting old statx patch with changes

[lolzery wowzery]

Hi,

Update: you've given me a lot to think about in my suggestions and
proposals to musl and, because you took time to respond to me and
explain things clearly, I feel much more compelled to put good effort
into making sure my changes are solid and indisputably beneficial (not
as in more=better kind of way but less=more but keeping in mind your
responses about musl's design.)

> statx is already in musl 1.2.5 (commit b817541f1cfd). If there are
> other problems than the ones I fixed when merging it and the
> stx_attributes field, please report them here rather than spending
> time trying to make a comprehensive changeset for a lot of things that
> might or might not actually be wrong.

I must have dyslexia or something because it turned out my efforts were a
wild goose chase for xstat, not statx! I'm so embarrassed, ha ha.

> > symbol
> > weakness problems,
>
> This sounds unlikely. It's more likely that you misunderstand how/why
> they're used. But I'm happy to look at your findings.

There are some symbol weakness inconsistencies with glibc I found in musl
but trying to track them all down by hand would be insane. I will get together
a tool to difference musl's and glibc symbols and list all changes to you one
of these days.

For reference, symbol weakness only affects what happens when linking two
libraries with same symbol names, which is used to override libc methods
for various necessary purposes. Glibc is the golden standard software is
built to link to, so, if anything, this will only help some software
work in musl.

> Can you clarify what you mean? There are some places where correctly
> atomic fallback is impossible and the fallback is best-effort only.
> This is generally only for missing O_CLOEXEC type functionality. If
> there are others, please report.

My original thinking was that my proposed solution of trying both the full
syscall and the fallback and seeing which work to handle seccomp would
introduce a race condition where the file is created between the two calls.

That thought is invalid now that I know we are not in the business of
supporting bad seccomp configurations.

> It's expected to fail catastrophically if seccomp filters modify
> syscall behavior in ways that make forward progress impossible (e.g.
> blocking close or something) or that lie about the cause of failure
> (the classic but badly wrong practice of using EPERM where ENOSYS was
> the correct code for syscalls not available in the sandbox). This is
> inherently not a supportable configuration.
>
> Generally musl always uses the oldest-possible syscall that can fully
> meet the requirements. If it has to try a new syscall first (like
> statx on 32-bit, since the legacy syscalls don't support full-range
> time_t and we don't know until after stat whether the times fit in 32
> bits), it falls back on ENOSYS. So as long as seccomp filters are
> using semantically correct error codes, things should generally work.
> But it's the responsibility of the seccomp filter authors to ensure
> they're doing this right.

Got it and thank you for the explanation. I now understand.

> It's never undefined. That's not how this works.
>
> If you're making out-of-tree bare-metal ports and don't want the
> overhead of having to add new syscalls like this, you can do something
> like define the SYS_* macros for them such that syscall_arch.h can
> statically catch that they're nonexistant (e.g. by given them values
> in some high range) and directly return -ENOSYS; then the code would
> collapse down as you want with the ENOSYS path being always-taken.
>
> Note that there is no way to emulate the nonzero flags for renameat2
> without race conditions. Since this is not standard/mandatory
> functionality, the right thing to do is just return an error for
> "unsupported flags", not try to emulate them.

Got it and thanks for explaining. Now that I've fixed my eyes and am
looking at statx, I see four main things to be fixed:
1. Remove the `#ifndef SYS_fstatat` because it's nonsensible
2. Add comments to explain things
3. Correctly validate the flags and EINVAL if unsupported by fallback
4. Zero all extraneous fields like __pad1 for future proofing.
5. The stx_rdev_major and stx_rdev_minor fields were not correctly filled in

Please do not make these 5 changes yourself yet as I might find more and
I have some great comments I want to add to explain why things are.

I also discovered and would like to do a very minor cleanup on
src/stat/fstatat.c, which has a duplicate copy of the struct statx for
no reason.

Additionally, I am working on a proper fallback and implementation for fstatx
and lstatx, which are the open-fd and symlink equivalents of statx. It will
properly use ioctl_iflags to polyfill the attributes field for fstatx.
This cannot be
done for lstatx, statx, or statatx as they accept a file name and no
equivalent to
ioctl_iflags exists which accepts a file name, so it requires an open file. RTM:

> These functions return information about a file, in the buffer
> pointed to by statbuf.  No permissions are required on the file
> itself, but—in the case of stat(), fstatat(), and lstat()—execute
> (search) permission is required on all of the directories in
>  pathname that lead to the file.

I swear I've spent the last 5 hours digging into the nitty gritty
depths of these
little-documented methods to ensure musl will have proper fallbacks.

QUESTION: The glibc wrapper for statx explicitly sets the errno to ENOSYS
upon successful execution of its fallback to fstat64 (yes you read that right.)
I swore I misread something or this was a bug until quadruple checking
that this is works-as-intended over the next 2 hours. Will check around
glibc more tomorrow but what should musl's policy be about this (and
perhaps other works-as-intended POSIX violations around glibc?) I'm
personally strongly leaning towards do-what-glibc-does for compatibility

> OK, please send.

Working hard to keep my changes small, localized, and important.

Jack









On Thu, Apr 25, 2024 at 8:25 AM Rich Felker <dalias@libc.org> wrote:
>
> On Wed, Apr 24, 2024 at 07:55:32PM -0400, lolzery wowzery wrote:
> > Hi all! I'm new to git over email, new to musl, and I want to really
> > get into contributing to musl. So, I'd really appreciate any
> > tips/comments/info for a newbie like me!
> >
> > Please do not merge these changes yet. There's numerous problems with
> > this code above and beyond the issues you pointed out,
>
> statx is already in musl 1.2.5 (commit b817541f1cfd). If there are
> other problems than the ones I fixed when merging it and the
> stx_attributes field, please report them here rather than spending
> time trying to make a comprehensive changeset for a lot of things that
> might or might not actually be wrong.
>
> > and each of
> > those issues led me down successive rabbit holes all over the musl
> > source code and I've ended up opening a sizable can of worms,
> > including io race condition bugs in fallback conditions,
>
> Can you clarify what you mean? There are some places where correctly
> atomic fallback is impossible and the fallback is best-effort only.
> This is generally only for missing O_CLOEXEC type functionality. If
> there are others, please report.
>
> > symbol
> > weakness problems,
>
> This sounds unlikely. It's more likely that you misunderstand how/why
> they're used. But I'm happy to look at your findings.
>
> > header/struct misorganization, and bugs in syscall
> > support detection making musl silently/unexpectedly fail under certain
> > seccomp configurations.
>
> It's expected to fail catastrophically if seccomp filters modify
> syscall behavior in ways that make forward progress impossible (e.g.
> blocking close or something) or that lie about the cause of failure
> (the classic but badly wrong practice of using EPERM where ENOSYS was
> the correct code for syscalls not available in the sandbox). This is
> inherently not a supportable configuration.
>
> Generally musl always uses the oldest-possible syscall that can fully
> meet the requirements. If it has to try a new syscall first (like
> statx on 32-bit, since the legacy syscalls don't support full-range
> time_t and we don't know until after stat whether the times fit in 32
> bits), it falls back on ENOSYS. So as long as seccomp filters are
> using semantically correct error codes, things should generally work.
> But it's the responsibility of the seccomp filter authors to ensure
> they're doing this right.
>
> > Also, let me cite the earlier patch from tony.ambardar@gmail.com about
> > renameat2. My patch will include a full proper renameat2
> > implementation with validation and fallback and will compile on
> > systems where SYS_renameat2 is undefined (using exclusively the
> > fallback.)
>
> It's never undefined. That's not how this works.
>
> If you're making out-of-tree bare-metal ports and don't want the
> overhead of having to add new syscalls like this, you can do something
> like define the SYS_* macros for them such that syscall_arch.h can
> statically catch that they're nonexistant (e.g. by given them values
> in some high range) and directly return -ENOSYS; then the code would
> collapse down as you want with the ENOSYS path being always-taken.
>
> Note that there is no way to emulate the nonzero flags for renameat2
> without race conditions. Since this is not standard/mandatory
> functionality, the right thing to do is just return an error for
> "unsupported flags", not try to emulate them.
>
> > I recognize I probably sound like an over-eager-beaver fussing over
> > fools-gold as I'm new and none of you know me. (That's the assumption
> > I myself would make if a new guy showed up claiming to have lots of
> > bug fixes and stuff.) So, please suspend your disbelief for a short
> > while until I can give my full report and all fixes tomorrow for
> > everyone to review. Give me a chance to prove my commitment to musl
> > and watch as I uphold my pledge to maintain my areas of the code in
> > musl and keep them ship-shape.
> >
> > Have a great day everyone and really looking forwards to sharing my
> > full report tomorrow!,
>
> OK, please send.
>
> Rich

Re: [musl] [PATCH 1/2] V3 resubmitting old statx patch with changes

[Rich Felker]

On Sat, Apr 27, 2024 at 10:29:35PM -0400, lolzery wowzery wrote:
> Hi,
> 
> Update: you've given me a lot to think about in my suggestions and
> proposals to musl and, because you took time to respond to me and
> explain things clearly, I feel much more compelled to put good effort
> into making sure my changes are solid and indisputably beneficial (not
> as in more=better kind of way but less=more but keeping in mind your
> responses about musl's design.)
> 
> > statx is already in musl 1.2.5 (commit b817541f1cfd). If there are
> > other problems than the ones I fixed when merging it and the
> > stx_attributes field, please report them here rather than spending
> > time trying to make a comprehensive changeset for a lot of things that
> > might or might not actually be wrong.
> 
> I must have dyslexia or something because it turned out my efforts were a
> wild goose chase for xstat, not statx! I'm so embarrassed, ha ha.
> 
> > > symbol
> > > weakness problems,
> >
> > This sounds unlikely. It's more likely that you misunderstand how/why
> > they're used. But I'm happy to look at your findings.
> 
> There are some symbol weakness inconsistencies with glibc I found in musl

There is nothing about weakness that is a public interface. It does
nothing at all in dynamic linking, and in static linking, it does not
declare an intent that the application can override/redefine the
function, only that libc not conflict with the application's namespace
*if the libc-provided function is not being used at all* because the
application is using a different namespace profile, like plain C
instead of POSIX or POSIX instead of POSIX+extensions. If you review
this I think you'll find they're all correct.

The other place they're used is for controlling link dependencies in
static linking and avoiding pulling in code that's not used/needed
because other functionality was not linked.

> but trying to track them all down by hand would be insane. I will get together
> a tool to difference musl's and glibc symbols and list all changes to you one
> of these days.
> 
> For reference, symbol weakness only affects what happens when linking two
> libraries with same symbol names, which is used to override libc methods
> for various necessary purposes.

> Glibc is the golden standard software is
> built to link to,

No, just no. This is not a premise that is acceptable here. musl is
not a glibc clone/drop-in replacement. musl and glibc are *different
systems*. musl implements certain standards (C, POSIX, IEEE754) and
selected nonstandard extensions based on certain criteria. We do not
do anything "because glibc does it and glibc is the 'golden
standard'".

> so, if anything, this will only help some software
> work in musl.

Facilitating hacks that involve UB and poking at implementation
internals is generally not a goal of musl.

> > Can you clarify what you mean? There are some places where correctly
> > atomic fallback is impossible and the fallback is best-effort only.
> > This is generally only for missing O_CLOEXEC type functionality. If
> > there are others, please report.
> 
> My original thinking was that my proposed solution of trying both the full
> syscall and the fallback and seeing which work to handle seccomp would
> introduce a race condition where the file is created between the two calls.

I don't see how that matters. If the fallback is correct, either is
correct for at least one moment in time and there is no distinction
except an ordering that's not synchronized and thereby arbitrary.


> > It's never undefined. That's not how this works.
> >
> > If you're making out-of-tree bare-metal ports and don't want the
> > overhead of having to add new syscalls like this, you can do something
> > like define the SYS_* macros for them such that syscall_arch.h can
> > statically catch that they're nonexistant (e.g. by given them values
> > in some high range) and directly return -ENOSYS; then the code would
> > collapse down as you want with the ENOSYS path being always-taken.
> >
> > Note that there is no way to emulate the nonzero flags for renameat2
> > without race conditions. Since this is not standard/mandatory
> > functionality, the right thing to do is just return an error for
> > "unsupported flags", not try to emulate them.
> 
> Got it and thanks for explaining. Now that I've fixed my eyes and am
> looking at statx, I see four main things to be fixed:
> 1. Remove the `#ifndef SYS_fstatat` because it's nonsensible

No, riscv32 does not have (and no new 32-bit archs will have)
SYS_fstatat because they require SYS_statx for time64 support (they
don't have a 32-bit native kernel stat structure).

The conditional is not necessary but it just optimizes out dead code.
On such archs, it's known that if SYS_statx fails, fstatat() will also
fail for the same reason, because it makes the same syscall.

> 2. Add comments to explain things

This is possibly okay, if they're explaining reasons for doing things
and not just translating C into a natural-language description of what
it's doing.

> 3. Correctly validate the flags and EINVAL if unsupported by fallback

fstatat() does that already. There isn't really any way to do the
validation in userspace without blocking access to newly-added kernel
functionality.

> 4. Zero all extraneous fields like __pad1 for future proofing.

This is probably a good idea, but it should be done via
zero-initializing the whole structure before filling it, not referring
to those fields by name. The names are not a public or even
libc-internal-private interface, but placeholders, and shouldn't be
used.

> 5. The stx_rdev_major and stx_rdev_minor fields were not correctly filled in

Another thing I missed on the initial review. Thanks for catching it.

> Please do not make these 5 changes yourself yet as I might find more and
> I have some great comments I want to add to explain why things are.

If you'd like to submit the fixes, please do them as individual
changes with commit messages that explain what was wrong and what
specifically is being fixed, not a big combined "fix statx fallback"
patch.

> I also discovered and would like to do a very minor cleanup on
> src/stat/fstatat.c, which has a duplicate copy of the struct statx for
> no reason.

That's because the patch 2 in this series as submitted was wrong and
never fixed, so I refrained from applying it, with the intent to
revisit after release. So that would be fine to do now.

> Additionally, I am working on a proper fallback and implementation for fstatx
> and lstatx, which are the open-fd and symlink equivalents of statx. It will

statx already supports those usages with proper flags
(AT_SYMLINK_NOFOLLOW, AT_EMPTY_PATH). Unless there's precedent for
functions by those names that wrap it with the necessary flags, I
don't see a motivation for adding them rather than just writing
application code to use the flags with statx().

> > These functions return information about a file, in the buffer
> > pointed to by statbuf.  No permissions are required on the file
> > itself, but—in the case of stat(), fstatat(), and lstat()—execute
> > (search) permission is required on all of the directories in
> >  pathname that lead to the file.
> 
> I swear I've spent the last 5 hours digging into the nitty gritty
> depths of these
> little-documented methods to ensure musl will have proper fallbacks.
> 
> QUESTION: The glibc wrapper for statx explicitly sets the errno to ENOSYS
> upon successful execution of its fallback to fstat64 (yes you read that right.)
> I swore I misread something or this was a bug until quadruple checking
> that this is works-as-intended over the next 2 hours. Will check around
> glibc more tomorrow but what should musl's policy be about this (and
> perhaps other works-as-intended POSIX violations around glibc?) I'm
> personally strongly leaning towards do-what-glibc-does for compatibility

The value of errno on success is not meaningful; it's valid for it to
take on any nonzero value as a consequence of a function call that
succeeds.

Our policy in general if offering interfaces modelled off glibc
(normally only happens when these are Linux-specific interfaces) is
that only properties which an application can reasonably expect to
rely on need to be matched. Things like the value of errno after
success would not fall under that. Even though POSIX does not govern
nonstandard interfaces like this, the principle that errno is not
meaningful in this case still applies.

Rich

Re: [musl] [PATCH 1/2] V3 resubmitting old statx patch with changes

[Rich Felker]

On Sun, Apr 28, 2024 at 12:13:56PM -0400, Rich Felker wrote:
> On Sat, Apr 27, 2024 at 10:29:35PM -0400, lolzery wowzery wrote:
> > 4. Zero all extraneous fields like __pad1 for future proofing.
> 
> This is probably a good idea, but it should be done via
> zero-initializing the whole structure before filling it, not referring
> to those fields by name. The names are not a public or even
> libc-internal-private interface, but placeholders, and shouldn't be
> used.
> 
> > 5. The stx_rdev_major and stx_rdev_minor fields were not correctly filled in
> 
> Another thing I missed on the initial review. Thanks for catching it.
> 
> > Please do not make these 5 changes yourself yet as I might find more and
> > I have some great comments I want to add to explain why things are.
> 
> If you'd like to submit the fixes, please do them as individual
> changes with commit messages that explain what was wrong and what
> specifically is being fixed, not a big combined "fix statx fallback"
> patch.

Ping. I'd like to get the uninitialized-field bugs fixed and ping
distros to backport those fixes quickly so that folks don't continue
generating binaries that would malfunction on older kernels.

I don't really care if these are separate fixes for missing rdev,
attrs, and padding, or a combined "fix uninitialized output fields in
statx fallback" patch.

The other things in your mails are separate and can be discussed (if
there is anything wrong to begin with) without holding these
known-needed fixes up.

Rich

Re: [musl] [PATCH 1/2] V3 resubmitting old statx patch with changes

[Rich Felker]

On Wed, Apr 24, 2024 at 07:55:32PM -0400, lolzery wowzery wrote:
> Hi all! I'm new to git over email, new to musl, and I want to really
> get into contributing to musl. So, I'd really appreciate any
> tips/comments/info for a newbie like me!
> 
> Please do not merge these changes yet. There's numerous problems with
> this code above and beyond the issues you pointed out, and each of
> those issues led me down successive rabbit holes all over the musl
> source code and I've ended up opening a sizable can of worms,
> including io race condition bugs in fallback conditions, symbol
> weakness problems, header/struct misorganization, and bugs in syscall
> support detection making musl silently/unexpectedly fail under certain
> seccomp configurations.
> 
> Also, let me cite the earlier patch from tony.ambardar@gmail.com about
> renameat2. My patch will include a full proper renameat2
> implementation with validation and fallback and will compile on
> systems where SYS_renameat2 is undefined (using exclusively the
> fallback.)
> 
> I recognize I probably sound like an over-eager-beaver fussing over
> fools-gold as I'm new and none of you know me. (That's the assumption
> I myself would make if a new guy showed up claiming to have lots of
> bug fixes and stuff.) So, please suspend your disbelief for a short
> while until I can give my full report and all fixes tomorrow for
> everyone to review. Give me a chance to prove my commitment to musl
> and watch as I uphold my pledge to maintain my areas of the code in
> musl and keep them ship-shape.
> 
> Have a great day everyone and really looking forwards to sharing my
> full report tomorrow!,

Ping. Any updates?

[musl] [PATCH 2/2] V3 src/stat/fstatat.c use new statx define

[Duncan Bellamy]

---
 src/stat/fstatat.c | 30 +++---------------------------
 1 file changed, 3 insertions(+), 27 deletions(-)

diff --git a/src/stat/fstatat.c b/src/stat/fstatat.c
index 74c51cf5..6b44f48a 100644
--- a/src/stat/fstatat.c
+++ b/src/stat/fstatat.c
@@ -1,4 +1,5 @@
 #define _BSD_SOURCE
+#define _GNU_SOURCE
 #include <sys/stat.h>
 #include <string.h>
 #include <fcntl.h>
@@ -7,36 +8,11 @@
 #include <sys/sysmacros.h>
 #include "syscall.h"
 
-struct statx {
-	uint32_t stx_mask;
-	uint32_t stx_blksize;
-	uint64_t stx_attributes;
-	uint32_t stx_nlink;
-	uint32_t stx_uid;
-	uint32_t stx_gid;
-	uint16_t stx_mode;
-	uint16_t pad1;
-	uint64_t stx_ino;
-	uint64_t stx_size;
-	uint64_t stx_blocks;
-	uint64_t stx_attributes_mask;
-	struct {
-		int64_t tv_sec;
-		uint32_t tv_nsec;
-		int32_t pad;
-	} stx_atime, stx_btime, stx_ctime, stx_mtime;
-	uint32_t stx_rdev_major;
-	uint32_t stx_rdev_minor;
-	uint32_t stx_dev_major;
-	uint32_t stx_dev_minor;
-	uint64_t spare[14];
-};
-
 static int fstatat_statx(int fd, const char *restrict path, struct stat *restrict st, int flag)
 {
 	struct statx stx;
 
-	int ret = __syscall(SYS_statx, fd, path, flag, 0x7ff, &stx);
+	int ret = __syscall(SYS_statx, fd, path, flag, STATX_BASIC_STATS, &stx);
 	if (ret) return ret;
 
 	*st = (struct stat){
@@ -152,6 +128,6 @@ int __fstatat(int fd, const char *restrict path, struct stat *restrict st, int f
 
 weak_alias(__fstatat, fstatat);
 
-#if !_REDIR_TIME64
+#if !_REDIR_TIME64 && !defined(_GNU_SOURCE)
 weak_alias(fstatat, fstatat64);
 #endif
-- 
2.32.1 (Apple Git-133)

Re: [musl] [PATCH 2/2] V3 src/stat/fstatat.c use new statx define

[Rich Felker]

On Wed, Aug 31, 2022 at 08:07:35PM +0100, Duncan Bellamy wrote:
> ---
>  src/stat/fstatat.c | 30 +++---------------------------
>  1 file changed, 3 insertions(+), 27 deletions(-)
> 
> diff --git a/src/stat/fstatat.c b/src/stat/fstatat.c
> index 74c51cf5..6b44f48a 100644
> --- a/src/stat/fstatat.c
> +++ b/src/stat/fstatat.c
> @@ -1,4 +1,5 @@
>  #define _BSD_SOURCE
> +#define _GNU_SOURCE
>  #include <sys/stat.h>
>  #include <string.h>
>  #include <fcntl.h>
> @@ -7,36 +8,11 @@
>  #include <sys/sysmacros.h>
>  #include "syscall.h"
>  
> -struct statx {
> -	uint32_t stx_mask;
> -	uint32_t stx_blksize;
> -	uint64_t stx_attributes;
> -	uint32_t stx_nlink;
> -	uint32_t stx_uid;
> -	uint32_t stx_gid;
> -	uint16_t stx_mode;
> -	uint16_t pad1;
> -	uint64_t stx_ino;
> -	uint64_t stx_size;
> -	uint64_t stx_blocks;
> -	uint64_t stx_attributes_mask;
> -	struct {
> -		int64_t tv_sec;
> -		uint32_t tv_nsec;
> -		int32_t pad;
> -	} stx_atime, stx_btime, stx_ctime, stx_mtime;
> -	uint32_t stx_rdev_major;
> -	uint32_t stx_rdev_minor;
> -	uint32_t stx_dev_major;
> -	uint32_t stx_dev_minor;
> -	uint64_t spare[14];
> -};
> -
>  static int fstatat_statx(int fd, const char *restrict path, struct stat *restrict st, int flag)
>  {
>  	struct statx stx;
>  
> -	int ret = __syscall(SYS_statx, fd, path, flag, 0x7ff, &stx);
> +	int ret = __syscall(SYS_statx, fd, path, flag, STATX_BASIC_STATS, &stx);
>  	if (ret) return ret;
>  
>  	*st = (struct stat){
> @@ -152,6 +128,6 @@ int __fstatat(int fd, const char *restrict path, struct stat *restrict st, int f
>  
>  weak_alias(__fstatat, fstatat);
>  
> -#if !_REDIR_TIME64
> +#if !_REDIR_TIME64 && !defined(_GNU_SOURCE)
>  weak_alias(fstatat, fstatat64);
>  #endif
> -- 
> 2.32.1 (Apple Git-133)

I don't understand why the condition for the glibc-compat alias was
changed here. It seems like an unrelated and unmotivated change that
slipped in...

Rich