[PATCH v2] vfs: Shortcut AT_EMPTY_PATH early for statx, and add AT_NO_PATH for statx and fstatat

[PATCH v2] vfs: Shortcut AT_EMPTY_PATH early for statx, and add AT_NO_PATH for statx and fstatat

[Xi Ruoyao]

It's cheap to check if the path is empty in the userspace, but expensive
to check if a userspace string is empty from the kernel.  So it seems a
waste to delay the check into the kernel, and using statx(AT_EMPTY_PATH)
to implement fstat is slower than a "native" fstat call.

In the past there was a similar performance issue with several Glibc
releases using fstatat(AT_EMPTY_PATH) for fstat.  That issue was fixed
by Glibc with reverting back to plain fstat, and worked around by the
kernel with a special fast code path for fstatat(AT_EMPTY_PATH) at
commit 9013c51c630a ("vfs: mostly undo glibc turning 'fstat()' into
'fstatat(AT_EMPTY_PATH)'").

But for arch/loongarch fstat does not exist, so we have to use statx.
And on all 32-bit architectures we must use statx for fstat after 2037
since the plain fstat call uses 32-bit timestamp.  Thus Glibc uses statx
for fstat on LoongArch and all 32-bit platforms, and these platforms
still suffer the performance issue.

So port the fstatat(AT_EMPTY_PATH) fast path to statx(AT_EMPTY_PATH) as
well, and add AT_NO_PATH (the name is suggested by Mateusz) which makes
statx and fstatat completely skip the path check and assume the path is
empty.

Benchmark on LoongArch Loongson 3A6000:

1. Unpatched kernel, Glibc fstat, actually statx(AT_EMPTY_PATH):
   2575328 ops/s
2. Patched kernel, Glibc fstat, actually statx(AT_EMPTY_PATH):
   5434782 ops/s, +111% from 1
3. Patched kernel, statx(AT_NO_PATH): 5773672 ops/s, +124% from 1,
   +6.2% from 2.

Seccomp sandboxes can also green light fstatat/statx(AT_NO_PATH) easier
than fstatat/statx(AT_EMPTY_PATH) for which the audition needs to check
5434782543478254347825434782the path but seccomp BPF program cannot do that now.

Link: https://sourceware.org/pipermail/libc-alpha/2023-September/151364.html
Link: https://sourceware.org/git/?p=glibc.git;a=commit;h=e6547d635b99
Link: https://sourceware.org/git/?p=glibc.git;a=commit;h=551101e8240b
Link: https://lore.kernel.org/loongarch/599df4a3-47a4-49be-9c81-8e21ea1f988a@xen0n.name/
Cc: Christian Brauner <brauner@kernel.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Jan Kara <jack@suse.cz>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Mateusz Guzik <mjguzik@gmail.com>
Cc: Alejandro Colomar <alx@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Huacai Chen <chenhuacai@loongson.cn>
Cc: Xuerui Wang <kernel@xen0n.name>
Cc: Jiaxun Yang <jiaxun.yang@flygoat.com>
Cc: Icenowy Zheng <uwu@icenowy.me>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: loongarch@lists.linux.dev
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Xi Ruoyao <xry111@xry111.site>
---

Superseds
https://lore.kernel.org/all/20240622105621.7922-1-xry111@xry111.site/.

 fs/stat.c                  | 103 +++++++++++++++++++++++++------------
 include/uapi/linux/fcntl.h |   3 ++
 2 files changed, 74 insertions(+), 32 deletions(-)

diff --git a/fs/stat.c b/fs/stat.c
index 70bd3e888cfa..2b7d4a22f971 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -208,7 +208,7 @@ int getname_statx_lookup_flags(int flags)
 		lookup_flags |= LOOKUP_FOLLOW;
 	if (!(flags & AT_NO_AUTOMOUNT))
 		lookup_flags |= LOOKUP_AUTOMOUNT;
-	if (flags & AT_EMPTY_PATH)
+	if (flags & (AT_EMPTY_PATH | AT_NO_PATH))
 		lookup_flags |= LOOKUP_EMPTY;
 
 	return lookup_flags;
@@ -217,7 +217,8 @@ int getname_statx_lookup_flags(int flags)
 /**
  * vfs_statx - Get basic and extra attributes by filename
  * @dfd: A file descriptor representing the base dir for a relative filename
- * @filename: The name of the file of interest
+ * @filename: The name of the file of interest, or NULL if the file of
+	      interest is dfd itself and dfd isn't AT_FDCWD
  * @flags: Flags to control the query
  * @stat: The result structure to fill in.
  * @request_mask: STATX_xxx flags indicating what the caller wants
@@ -232,42 +233,56 @@ int getname_statx_lookup_flags(int flags)
 static int vfs_statx(int dfd, struct filename *filename, int flags,
 	      struct kstat *stat, u32 request_mask)
 {
-	struct path path;
+	struct path path, *p;
+	struct fd f;
 	unsigned int lookup_flags = getname_statx_lookup_flags(flags);
 	int error;
 
 	if (flags & ~(AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT | AT_EMPTY_PATH |
-		      AT_STATX_SYNC_TYPE))
+		      AT_STATX_SYNC_TYPE | AT_NO_PATH))
 		return -EINVAL;
 
 retry:
-	error = filename_lookup(dfd, filename, lookup_flags, &path, NULL);
-	if (error)
-		goto out;
+	if (filename) {
+		p = &path;
+		error = filename_lookup(dfd, filename, lookup_flags, p,
+					NULL);
+		if (error)
+			goto out;
+	} else {
+		f = fdget_raw(dfd);
+		if (!f.file)
+			return -EBADF;
+		p = &f.file->f_path;
+	}
 
-	error = vfs_getattr(&path, stat, request_mask, flags);
+	error = vfs_getattr(p, stat, request_mask, flags);
 
 	if (request_mask & STATX_MNT_ID_UNIQUE) {
-		stat->mnt_id = real_mount(path.mnt)->mnt_id_unique;
+		stat->mnt_id = real_mount(p->mnt)->mnt_id_unique;
 		stat->result_mask |= STATX_MNT_ID_UNIQUE;
 	} else {
-		stat->mnt_id = real_mount(path.mnt)->mnt_id;
+		stat->mnt_id = real_mount(p->mnt)->mnt_id;
 		stat->result_mask |= STATX_MNT_ID;
 	}
 
-	if (path.mnt->mnt_root == path.dentry)
+	if (p->mnt->mnt_root == p->dentry)
 		stat->attributes |= STATX_ATTR_MOUNT_ROOT;
 	stat->attributes_mask |= STATX_ATTR_MOUNT_ROOT;
 
 	/* Handle STATX_DIOALIGN for block devices. */
 	if (request_mask & STATX_DIOALIGN) {
-		struct inode *inode = d_backing_inode(path.dentry);
+		struct inode *inode = d_backing_inode(p->dentry);
 
 		if (S_ISBLK(inode->i_mode))
 			bdev_statx_dioalign(inode, stat);
 	}
 
-	path_put(&path);
+	if (filename)
+		path_put(p);
+	else
+		fdput(f);
+
 	if (retry_estale(error, lookup_flags)) {
 		lookup_flags |= LOOKUP_REVAL;
 		goto retry;
@@ -276,33 +291,53 @@ static int vfs_statx(int dfd, struct filename *filename, int flags,
 	return error;
 }
 
-int vfs_fstatat(int dfd, const char __user *filename,
-			      struct kstat *stat, int flags)
+static struct filename *getname_statx(int dfd, const char __user *filename,
+				      int flags)
 {
-	int ret;
-	int statx_flags = flags | AT_NO_AUTOMOUNT;
-	struct filename *name;
+	int r;
+	char c;
+	bool no_path = false;
+
+	if (dfd < 0 && dfd != AT_FDCWD)
+		return ERR_PTR(-EBADF);
 
 	/*
-	 * Work around glibc turning fstat() into fstatat(AT_EMPTY_PATH)
+	 * Work around glibc turning fstat() into fstatat(AT_EMPTY_PATH) or
+	 * statx(AT_EMPTY_PATH)
 	 *
 	 * If AT_EMPTY_PATH is set, we expect the common case to be that
 	 * empty path, and avoid doing all the extra pathname work.
 	 */
-	if (dfd >= 0 && flags == AT_EMPTY_PATH) {
-		char c;
+	if (flags & AT_NO_PATH)
+		no_path = true;
+	else if (flags & AT_EMPTY_PATH) {
+		r = get_user(c, filename);
+		if (unlikely(r))
+			return ERR_PTR(r);
+		no_path = likely(!c);
+	}
 
-		ret = get_user(c, filename);
-		if (unlikely(ret))
-			return ret;
+	if (no_path)
+		return dfd == AT_FDCWD ? getname_kernel("") : NULL;
 
-		if (likely(!c))
-			return vfs_fstat(dfd, stat);
-	}
+	return getname_flags(filename, getname_statx_lookup_flags(flags),
+			     NULL);
+}
+
+int vfs_fstatat(int dfd, const char __user *filename,
+			      struct kstat *stat, int flags)
+{
+	int ret;
+	int statx_flags = flags | AT_NO_AUTOMOUNT;
+	struct filename *name = getname_statx(dfd, filename, flags);
+
+	if (IS_ERR(name))
+		return PTR_ERR(name);
 
-	name = getname_flags(filename, getname_statx_lookup_flags(statx_flags), NULL);
 	ret = vfs_statx(dfd, name, statx_flags, stat, STATX_BASIC_STATS);
-	putname(name);
+
+	if (name)
+		putname(name);
 
 	return ret;
 }
@@ -703,11 +738,15 @@ SYSCALL_DEFINE5(statx,
 		struct statx __user *, buffer)
 {
 	int ret;
-	struct filename *name;
+	struct filename *name = getname_statx(dfd, filename, flags);
+
+	if (IS_ERR(name))
+		return PTR_ERR(name);
 
-	name = getname_flags(filename, getname_statx_lookup_flags(flags), NULL);
 	ret = do_statx(dfd, name, flags, mask, buffer);
-	putname(name);
+
+	if (name)
+		putname(name);
 
 	return ret;
 }
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index c0bcc185fa48..470b5c77000c 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -113,6 +113,9 @@
 #define AT_STATX_DONT_SYNC	0x4000	/* - Don't sync attributes with the server */
 
 #define AT_RECURSIVE		0x8000	/* Apply to the entire subtree */
+#define AT_NO_PATH		0x10000	/* Ignore relative pathname,
+					   behave as if AT_EMPTY_PATH and
+					   the relative pathname is empty */
 
 /* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
 #define AT_HANDLE_FID		AT_REMOVEDIR	/* file handle is needed to
-- 
2.45.2

Re: [PATCH v2] vfs: Shortcut AT_EMPTY_PATH early for statx, and add AT_NO_PATH for statx and fstatat

[Xi Ruoyao]

On Mon, 2024-06-24 at 16:50 +0800, Xi Ruoyao wrote:
> Seccomp sandboxes can also green light fstatat/statx(AT_NO_PATH) easier
> than fstatat/statx(AT_EMPTY_PATH) for which the audition needs to check
> 5434782543478254347825434782the path but seccomp BPF program cannot do that now.

Oops "5434782543478254347825434782" is meaningless.  Not sure how it
ended up in my commit message.  I'll remove it in v3 but now just
waiting for comments.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH v2] vfs: Shortcut AT_EMPTY_PATH early for statx, and add AT_NO_PATH for statx and fstatat

[Mateusz Guzik]

On Mon, Jun 24, 2024 at 04:50:26PM +0800, Xi Ruoyao wrote:
> It's cheap to check if the path is empty in the userspace, but expensive
> to check if a userspace string is empty from the kernel.  So it seems a
> waste to delay the check into the kernel, and using statx(AT_EMPTY_PATH)
> to implement fstat is slower than a "native" fstat call.
> 
> In the past there was a similar performance issue with several Glibc
> releases using fstatat(AT_EMPTY_PATH) for fstat.  That issue was fixed
> by Glibc with reverting back to plain fstat, and worked around by the
> kernel with a special fast code path for fstatat(AT_EMPTY_PATH) at
> commit 9013c51c630a ("vfs: mostly undo glibc turning 'fstat()' into
> 'fstatat(AT_EMPTY_PATH)'").
> 
> But for arch/loongarch fstat does not exist, so we have to use statx.
> And on all 32-bit architectures we must use statx for fstat after 2037
> since the plain fstat call uses 32-bit timestamp.  Thus Glibc uses statx
> for fstat on LoongArch and all 32-bit platforms, and these platforms
> still suffer the performance issue.
> 
> So port the fstatat(AT_EMPTY_PATH) fast path to statx(AT_EMPTY_PATH) as
> well, and add AT_NO_PATH (the name is suggested by Mateusz) which makes
> statx and fstatat completely skip the path check and assume the path is
> empty.
> 
> Benchmark on LoongArch Loongson 3A6000:
> 
> 1. Unpatched kernel, Glibc fstat, actually statx(AT_EMPTY_PATH):
>    2575328 ops/s
> 2. Patched kernel, Glibc fstat, actually statx(AT_EMPTY_PATH):
>    5434782 ops/s, +111% from 1
> 3. Patched kernel, statx(AT_NO_PATH): 5773672 ops/s, +124% from 1,
>    +6.2% from 2.
> 

Nice win.

> Seccomp sandboxes can also green light fstatat/statx(AT_NO_PATH) easier
> than fstatat/statx(AT_EMPTY_PATH) for which the audition needs to check
> 5434782543478254347825434782the path but seccomp BPF program cannot do that now.
> 
> Link: https://sourceware.org/pipermail/libc-alpha/2023-September/151364.html
> Link: https://sourceware.org/git/?p=glibc.git;a=commit;h=e6547d635b99
> Link: https://sourceware.org/git/?p=glibc.git;a=commit;h=551101e8240b
> Link: https://lore.kernel.org/loongarch/599df4a3-47a4-49be-9c81-8e21ea1f988a@xen0n.name/
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Mateusz Guzik <mjguzik@gmail.com>
> Cc: Alejandro Colomar <alx@kernel.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Huacai Chen <chenhuacai@loongson.cn>
> Cc: Xuerui Wang <kernel@xen0n.name>
> Cc: Jiaxun Yang <jiaxun.yang@flygoat.com>
> Cc: Icenowy Zheng <uwu@icenowy.me>
> Cc: linux-fsdevel@vger.kernel.org
> Cc: linux-arch@vger.kernel.org
> Cc: loongarch@lists.linux.dev
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Xi Ruoyao <xry111@xry111.site>
> ---
> 
> Superseds
> https://lore.kernel.org/all/20240622105621.7922-1-xry111@xry111.site/.
> 
>  fs/stat.c                  | 103 +++++++++++++++++++++++++------------
>  include/uapi/linux/fcntl.h |   3 ++
>  2 files changed, 74 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/stat.c b/fs/stat.c
> index 70bd3e888cfa..2b7d4a22f971 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -208,7 +208,7 @@ int getname_statx_lookup_flags(int flags)
>  		lookup_flags |= LOOKUP_FOLLOW;
>  	if (!(flags & AT_NO_AUTOMOUNT))
>  		lookup_flags |= LOOKUP_AUTOMOUNT;
> -	if (flags & AT_EMPTY_PATH)
> +	if (flags & (AT_EMPTY_PATH | AT_NO_PATH))
>  		lookup_flags |= LOOKUP_EMPTY;
>  
>  	return lookup_flags;
> @@ -217,7 +217,8 @@ int getname_statx_lookup_flags(int flags)
>  /**
>   * vfs_statx - Get basic and extra attributes by filename
>   * @dfd: A file descriptor representing the base dir for a relative filename
> - * @filename: The name of the file of interest
> + * @filename: The name of the file of interest, or NULL if the file of
> +	      interest is dfd itself and dfd isn't AT_FDCWD
>   * @flags: Flags to control the query
>   * @stat: The result structure to fill in.
>   * @request_mask: STATX_xxx flags indicating what the caller wants
> @@ -232,42 +233,56 @@ int getname_statx_lookup_flags(int flags)
>  static int vfs_statx(int dfd, struct filename *filename, int flags,
>  	      struct kstat *stat, u32 request_mask)
>  {
> -	struct path path;
> +	struct path path, *p;
> +	struct fd f;
>  	unsigned int lookup_flags = getname_statx_lookup_flags(flags);
>  	int error;
>  
>  	if (flags & ~(AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT | AT_EMPTY_PATH |
> -		      AT_STATX_SYNC_TYPE))
> +		      AT_STATX_SYNC_TYPE | AT_NO_PATH))
>  		return -EINVAL;
>  
>  retry:
> -	error = filename_lookup(dfd, filename, lookup_flags, &path, NULL);
> -	if (error)
> -		goto out;
> +	if (filename) {
> +		p = &path;
> +		error = filename_lookup(dfd, filename, lookup_flags, p,
> +					NULL);
> +		if (error)
> +			goto out;
> +	} else {
> +		f = fdget_raw(dfd);
> +		if (!f.file)
> +			return -EBADF;
> +		p = &f.file->f_path;
> +	}
>  

I don't think this is the right approach. Note this also did not cover
io_uring.

I was thinking factoring code out to vfs_statx_path and adding
vfs_statx_fd.

Below is a diff which compiles but is untested. It adds AT_EMPTY_PATH +
NULL as suggsted by Linus, but it can be adjusted for AT_NO_PATH (which
would be my preffered option, or better yet not do that and add fstatx).

It does not do the hack to 0-check if a pointer was passed along with
AT_EMPTY_PATH but that again is an easy addition.

Feel free to take without attribution:

diff --git a/fs/internal.h b/fs/internal.h
index 84f371193f74..1d820018e6dc 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -247,6 +247,8 @@ extern const struct dentry_operations ns_dentry_operations;
 int getname_statx_lookup_flags(int flags);
 int do_statx(int dfd, struct filename *filename, unsigned int flags,
 	     unsigned int mask, struct statx __user *buffer);
+int do_statx_fd(int fd, unsigned int flags, unsigned int mask,
+		struct statx __user *buffer);
 
 /*
  * fs/splice.c:
diff --git a/fs/stat.c b/fs/stat.c
index 16aa1f5ceec4..b0a4db7b90df 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -214,6 +214,48 @@ int getname_statx_lookup_flags(int flags)
 	return lookup_flags;
 }
 
+static int vfs_statx_path(struct path *path, int flags, struct kstat *stat,
+			  u32 request_mask)
+{
+	int error = vfs_getattr(path, stat, request_mask, flags);
+
+	if (request_mask & STATX_MNT_ID_UNIQUE) {
+		stat->mnt_id = real_mount(path->mnt)->mnt_id_unique;
+		stat->result_mask |= STATX_MNT_ID_UNIQUE;
+	} else {
+		stat->mnt_id = real_mount(path->mnt)->mnt_id;
+		stat->result_mask |= STATX_MNT_ID;
+	}
+
+	if (path->mnt->mnt_root == path->dentry)
+		stat->attributes |= STATX_ATTR_MOUNT_ROOT;
+	stat->attributes_mask |= STATX_ATTR_MOUNT_ROOT;
+
+	/* Handle STATX_DIOALIGN for block devices. */
+	if (request_mask & STATX_DIOALIGN) {
+		struct inode *inode = d_backing_inode(path->dentry);
+
+		if (S_ISBLK(inode->i_mode))
+			bdev_statx_dioalign(inode, stat);
+	}
+
+	return error;
+}
+
+static int vfs_statx_fd(int fd, int flags, struct kstat *stat,
+			  u32 request_mask)
+{
+	struct fd f;
+	int error;
+
+	f = fdget_raw(fd);
+	if (!f.file)
+		return -EBADF;
+	error = vfs_statx_path(&f.file->f_path, flags, stat, request_mask);
+	fdput(f);
+	return error;
+}
+
 /**
  * vfs_statx - Get basic and extra attributes by filename
  * @dfd: A file descriptor representing the base dir for a relative filename
@@ -243,36 +285,13 @@ static int vfs_statx(int dfd, struct filename *filename, int flags,
 retry:
 	error = filename_lookup(dfd, filename, lookup_flags, &path, NULL);
 	if (error)
-		goto out;
-
-	error = vfs_getattr(&path, stat, request_mask, flags);
-
-	if (request_mask & STATX_MNT_ID_UNIQUE) {
-		stat->mnt_id = real_mount(path.mnt)->mnt_id_unique;
-		stat->result_mask |= STATX_MNT_ID_UNIQUE;
-	} else {
-		stat->mnt_id = real_mount(path.mnt)->mnt_id;
-		stat->result_mask |= STATX_MNT_ID;
-	}
-
-	if (path.mnt->mnt_root == path.dentry)
-		stat->attributes |= STATX_ATTR_MOUNT_ROOT;
-	stat->attributes_mask |= STATX_ATTR_MOUNT_ROOT;
-
-	/* Handle STATX_DIOALIGN for block devices. */
-	if (request_mask & STATX_DIOALIGN) {
-		struct inode *inode = d_backing_inode(path.dentry);
-
-		if (S_ISBLK(inode->i_mode))
-			bdev_statx_dioalign(inode, stat);
-	}
-
+		return error;
+	error = vfs_statx_path(&path, flags, stat, request_mask);
 	path_put(&path);
 	if (retry_estale(error, lookup_flags)) {
 		lookup_flags |= LOOKUP_REVAL;
 		goto retry;
 	}
-out:
 	return error;
 }
 
@@ -691,6 +710,29 @@ int do_statx(int dfd, struct filename *filename, unsigned int flags,
 	return cp_statx(&stat, buffer);
 }
 
+int do_statx_fd(int fd, unsigned int flags, unsigned int mask,
+	     struct statx __user *buffer)
+{
+	struct kstat stat;
+	int error;
+
+	if (mask & STATX__RESERVED)
+		return -EINVAL;
+	if ((flags & AT_STATX_SYNC_TYPE) == AT_STATX_SYNC_TYPE)
+		return -EINVAL;
+
+	/* STATX_CHANGE_COOKIE is kernel-only for now. Ignore requests
+	 * from userland.
+	 */
+	mask &= ~STATX_CHANGE_COOKIE;
+
+	error = vfs_statx_fd(fd, flags, &stat, mask);
+	if (error)
+		return error;
+
+	return cp_statx(&stat, buffer);
+}
+
 /**
  * sys_statx - System call to get enhanced stats
  * @dfd: Base directory to pathwalk from *or* fd to stat.
@@ -710,6 +752,9 @@ SYSCALL_DEFINE5(statx,
 	int ret;
 	struct filename *name;
 
+	if (filename == NULL && (flags & AT_EMPTY_PATH))
+		return do_statx_fd(dfd, flags, mask, buffer);
+
 	name = getname_flags(filename, getname_statx_lookup_flags(flags));
 	ret = do_statx(dfd, name, flags, mask, buffer);
 	putname(name);
diff --git a/io_uring/statx.c b/io_uring/statx.c
index f7f9b202eec0..21c97aff270d 100644
--- a/io_uring/statx.c
+++ b/io_uring/statx.c
@@ -23,6 +23,7 @@ struct io_statx {
 int io_statx_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
 	struct io_statx *sx = io_kiocb_to_cmd(req, struct io_statx);
+	struct filename *filename;
 	const char __user *path;
 
 	if (sqe->buf_index || sqe->splice_fd_in)
@@ -36,14 +37,13 @@ int io_statx_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	sx->buffer = u64_to_user_ptr(READ_ONCE(sqe->addr2));
 	sx->flags = READ_ONCE(sqe->statx_flags);
 
-	sx->filename = getname_flags(path,
-				     getname_statx_lookup_flags(sx->flags));
-
-	if (IS_ERR(sx->filename)) {
-		int ret = PTR_ERR(sx->filename);
-
-		sx->filename = NULL;
-		return ret;
+	sx->filename = NULL;
+	if (!(path == NULL && (sx->flags & AT_EMPTY_PATH))) {
+		filename = getname_flags(path,
+					 getname_statx_lookup_flags(sx->flags));
+		if (IS_ERR(filename))
+			return PTR_ERR(filename);
+		sx->filename = filename;
 	}
 
 	req->flags |= REQ_F_NEED_CLEANUP;
@@ -58,7 +58,10 @@ int io_statx(struct io_kiocb *req, unsigned int issue_flags)
 
 	WARN_ON_ONCE(issue_flags & IO_URING_F_NONBLOCK);
 
-	ret = do_statx(sx->dfd, sx->filename, sx->flags, sx->mask, sx->buffer);
+	if (sx->filename == NULL && (sx->flags & AT_EMPTY_PATH))
+		ret = do_statx_fd(sx->dfd, sx->flags, sx->mask, sx->buffer);
+	else
+		ret = do_statx(sx->dfd, sx->filename, sx->flags, sx->mask, sx->buffer);
 	io_req_set_res(req, ret, 0);
 	return IOU_OK;
 }
-- 
2.43.0

Re: [PATCH v2] vfs: Shortcut AT_EMPTY_PATH early for statx, and add AT_NO_PATH for statx and fstatat

[Xi Ruoyao]

On Mon, 2024-06-24 at 11:04 +0200, Mateusz Guzik wrote:
> Below is a diff which compiles but is untested. It adds AT_EMPTY_PATH +
> NULL as suggsted by Linus, but it can be adjusted for AT_NO_PATH (which
> would be my preffered option, or better yet not do that and add fstatx).
> 
> It does not do the hack to 0-check if a pointer was passed along with
> AT_EMPTY_PATH but that again is an easy addition.
> 
> Feel free to take without attribution:

I'd still like to make it Co-developed-by: or just From: you.  Could you
give a S-o-b?

And with this change AT_FDCWD with AT_EMPTY_PATH and NULL path does not
work.  For consistency it'd be better to make it work too:

diff --git a/fs/stat.c b/fs/stat.c
index b0a4db7b90df..d04f7ba46645 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -752,10 +752,14 @@ SYSCALL_DEFINE5(statx,
 	int ret;
 	struct filename *name;
 
-	if (filename == NULL && (flags & AT_EMPTY_PATH))
-		return do_statx_fd(dfd, flags, mask, buffer);
+	if (filename == NULL && (flags & AT_EMPTY_PATH)) {
+		if (dfd >= 0)
+			return do_statx_fd(dfd, flags, mask, buffer);
+		else
+			name = getname_kernel("");
+	} else
+		name = getname_flags(filename, getname_statx_lookup_flags(flags));
 
-	name = getname_flags(filename, getname_statx_lookup_flags(flags));
 	ret = do_statx(dfd, name, flags, mask, buffer);
 	putname(name);
 
And should we do it for fstatat() as well?

Let's wait for comment from Linus anyway.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH v2] vfs: Shortcut AT_EMPTY_PATH early for statx, and add AT_NO_PATH for statx and fstatat

[Mateusz Guzik]

On Mon, Jun 24, 2024 at 2:19 PM Xi Ruoyao <xry111@xry111.site> wrote:
>
> On Mon, 2024-06-24 at 11:04 +0200, Mateusz Guzik wrote:
> > Below is a diff which compiles but is untested. It adds AT_EMPTY_PATH +
> > NULL as suggsted by Linus, but it can be adjusted for AT_NO_PATH (which
> > would be my preffered option, or better yet not do that and add fstatx).
> >
> > It does not do the hack to 0-check if a pointer was passed along with
> > AT_EMPTY_PATH but that again is an easy addition.
> >
> > Feel free to take without attribution:
>
> I'd still like to make it Co-developed-by: or just From: you.  Could you
> give a S-o-b?
>

I just trivially shuffled some things around and did not even test, so
I'm not signing off on squat here. :)

However, if you insist you can add something like "Written after
picking up an initial patch written by Mateusz Guzik, see [link goes
here]" or similar.

> And with this change AT_FDCWD with AT_EMPTY_PATH and NULL path does not
> work.  For consistency it'd be better to make it work too:
>

Good catch, I have no opinion which way to fix it.

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH v2] vfs: Shortcut AT_EMPTY_PATH early for statx, and add AT_NO_PATH for statx and fstatat

[Christian Brauner]

On Mon, Jun 24, 2024 at 11:04:03AM GMT, Mateusz Guzik wrote:
> On Mon, Jun 24, 2024 at 04:50:26PM +0800, Xi Ruoyao wrote:
> > It's cheap to check if the path is empty in the userspace, but expensive
> > to check if a userspace string is empty from the kernel.  So it seems a
> > waste to delay the check into the kernel, and using statx(AT_EMPTY_PATH)
> > to implement fstat is slower than a "native" fstat call.
> > 
> > In the past there was a similar performance issue with several Glibc
> > releases using fstatat(AT_EMPTY_PATH) for fstat.  That issue was fixed
> > by Glibc with reverting back to plain fstat, and worked around by the
> > kernel with a special fast code path for fstatat(AT_EMPTY_PATH) at
> > commit 9013c51c630a ("vfs: mostly undo glibc turning 'fstat()' into
> > 'fstatat(AT_EMPTY_PATH)'").
> > 
> > But for arch/loongarch fstat does not exist, so we have to use statx.
> > And on all 32-bit architectures we must use statx for fstat after 2037
> > since the plain fstat call uses 32-bit timestamp.  Thus Glibc uses statx
> > for fstat on LoongArch and all 32-bit platforms, and these platforms
> > still suffer the performance issue.
> > 
> > So port the fstatat(AT_EMPTY_PATH) fast path to statx(AT_EMPTY_PATH) as
> > well, and add AT_NO_PATH (the name is suggested by Mateusz) which makes
> > statx and fstatat completely skip the path check and assume the path is
> > empty.
> > 
> > Benchmark on LoongArch Loongson 3A6000:
> > 
> > 1. Unpatched kernel, Glibc fstat, actually statx(AT_EMPTY_PATH):
> >    2575328 ops/s
> > 2. Patched kernel, Glibc fstat, actually statx(AT_EMPTY_PATH):
> >    5434782 ops/s, +111% from 1
> > 3. Patched kernel, statx(AT_NO_PATH): 5773672 ops/s, +124% from 1,
> >    +6.2% from 2.
> > 
> 
> Nice win.
> 
> > Seccomp sandboxes can also green light fstatat/statx(AT_NO_PATH) easier
> > than fstatat/statx(AT_EMPTY_PATH) for which the audition needs to check
> > 5434782543478254347825434782the path but seccomp BPF program cannot do that now.
> > 
> > Link: https://sourceware.org/pipermail/libc-alpha/2023-September/151364.html
> > Link: https://sourceware.org/git/?p=glibc.git;a=commit;h=e6547d635b99
> > Link: https://sourceware.org/git/?p=glibc.git;a=commit;h=551101e8240b
> > Link: https://lore.kernel.org/loongarch/599df4a3-47a4-49be-9c81-8e21ea1f988a@xen0n.name/
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: Jan Kara <jack@suse.cz>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Mateusz Guzik <mjguzik@gmail.com>
> > Cc: Alejandro Colomar <alx@kernel.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Huacai Chen <chenhuacai@loongson.cn>
> > Cc: Xuerui Wang <kernel@xen0n.name>
> > Cc: Jiaxun Yang <jiaxun.yang@flygoat.com>
> > Cc: Icenowy Zheng <uwu@icenowy.me>
> > Cc: linux-fsdevel@vger.kernel.org
> > Cc: linux-arch@vger.kernel.org
> > Cc: loongarch@lists.linux.dev
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Xi Ruoyao <xry111@xry111.site>
> > ---
> > 
> > Superseds
> > https://lore.kernel.org/all/20240622105621.7922-1-xry111@xry111.site/.
> > 
> >  fs/stat.c                  | 103 +++++++++++++++++++++++++------------
> >  include/uapi/linux/fcntl.h |   3 ++
> >  2 files changed, 74 insertions(+), 32 deletions(-)
> > 
> > diff --git a/fs/stat.c b/fs/stat.c
> > index 70bd3e888cfa..2b7d4a22f971 100644
> > --- a/fs/stat.c
> > +++ b/fs/stat.c
> > @@ -208,7 +208,7 @@ int getname_statx_lookup_flags(int flags)
> >  		lookup_flags |= LOOKUP_FOLLOW;
> >  	if (!(flags & AT_NO_AUTOMOUNT))
> >  		lookup_flags |= LOOKUP_AUTOMOUNT;
> > -	if (flags & AT_EMPTY_PATH)
> > +	if (flags & (AT_EMPTY_PATH | AT_NO_PATH))
> >  		lookup_flags |= LOOKUP_EMPTY;
> >  
> >  	return lookup_flags;
> > @@ -217,7 +217,8 @@ int getname_statx_lookup_flags(int flags)
> >  /**
> >   * vfs_statx - Get basic and extra attributes by filename
> >   * @dfd: A file descriptor representing the base dir for a relative filename
> > - * @filename: The name of the file of interest
> > + * @filename: The name of the file of interest, or NULL if the file of
> > +	      interest is dfd itself and dfd isn't AT_FDCWD
> >   * @flags: Flags to control the query
> >   * @stat: The result structure to fill in.
> >   * @request_mask: STATX_xxx flags indicating what the caller wants
> > @@ -232,42 +233,56 @@ int getname_statx_lookup_flags(int flags)
> >  static int vfs_statx(int dfd, struct filename *filename, int flags,
> >  	      struct kstat *stat, u32 request_mask)
> >  {
> > -	struct path path;
> > +	struct path path, *p;
> > +	struct fd f;
> >  	unsigned int lookup_flags = getname_statx_lookup_flags(flags);
> >  	int error;
> >  
> >  	if (flags & ~(AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT | AT_EMPTY_PATH |
> > -		      AT_STATX_SYNC_TYPE))
> > +		      AT_STATX_SYNC_TYPE | AT_NO_PATH))
> >  		return -EINVAL;
> >  
> >  retry:
> > -	error = filename_lookup(dfd, filename, lookup_flags, &path, NULL);
> > -	if (error)
> > -		goto out;
> > +	if (filename) {
> > +		p = &path;
> > +		error = filename_lookup(dfd, filename, lookup_flags, p,
> > +					NULL);
> > +		if (error)
> > +			goto out;
> > +	} else {
> > +		f = fdget_raw(dfd);
> > +		if (!f.file)
> > +			return -EBADF;
> > +		p = &f.file->f_path;
> > +	}
> >  
> 
> I don't think this is the right approach. Note this also did not cover
> io_uring.
> 
> I was thinking factoring code out to vfs_statx_path and adding
> vfs_statx_fd.
> 
> Below is a diff which compiles but is untested. It adds AT_EMPTY_PATH +
> NULL as suggsted by Linus, but it can be adjusted for AT_NO_PATH (which

No, let's not waste AT_* flag space in fear of some hypothetical
breakage. Let's try it cleanly first and make AT_EMPTY_PATH work with
NULL paths.

Note, I started working on this (checks ...) 30th April but then some
other work came up and I never got back to it (Sorry, Linus). I pushed
the branch to #vfs.empty.path now. The top three commits was what I had
started doing.

It was based on a new vfs_empty_path() helper so we could reuse it for
other system calls as well.

> would be my preffered option, or better yet not do that and add fstatx).
> 
> It does not do the hack to 0-check if a pointer was passed along with
> AT_EMPTY_PATH but that again is an easy addition.
> 
> Feel free to take without attribution:
> 
> diff --git a/fs/internal.h b/fs/internal.h
> index 84f371193f74..1d820018e6dc 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -247,6 +247,8 @@ extern const struct dentry_operations ns_dentry_operations;
>  int getname_statx_lookup_flags(int flags);
>  int do_statx(int dfd, struct filename *filename, unsigned int flags,
>  	     unsigned int mask, struct statx __user *buffer);
> +int do_statx_fd(int fd, unsigned int flags, unsigned int mask,
> +		struct statx __user *buffer);
>  
>  /*
>   * fs/splice.c:
> diff --git a/fs/stat.c b/fs/stat.c
> index 16aa1f5ceec4..b0a4db7b90df 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -214,6 +214,48 @@ int getname_statx_lookup_flags(int flags)
>  	return lookup_flags;
>  }
>  
> +static int vfs_statx_path(struct path *path, int flags, struct kstat *stat,
> +			  u32 request_mask)
> +{
> +	int error = vfs_getattr(path, stat, request_mask, flags);
> +
> +	if (request_mask & STATX_MNT_ID_UNIQUE) {
> +		stat->mnt_id = real_mount(path->mnt)->mnt_id_unique;
> +		stat->result_mask |= STATX_MNT_ID_UNIQUE;
> +	} else {
> +		stat->mnt_id = real_mount(path->mnt)->mnt_id;
> +		stat->result_mask |= STATX_MNT_ID;
> +	}
> +
> +	if (path->mnt->mnt_root == path->dentry)
> +		stat->attributes |= STATX_ATTR_MOUNT_ROOT;
> +	stat->attributes_mask |= STATX_ATTR_MOUNT_ROOT;
> +
> +	/* Handle STATX_DIOALIGN for block devices. */
> +	if (request_mask & STATX_DIOALIGN) {
> +		struct inode *inode = d_backing_inode(path->dentry);
> +
> +		if (S_ISBLK(inode->i_mode))
> +			bdev_statx_dioalign(inode, stat);
> +	}
> +
> +	return error;
> +}
> +
> +static int vfs_statx_fd(int fd, int flags, struct kstat *stat,
> +			  u32 request_mask)
> +{
> +	struct fd f;
> +	int error;
> +
> +	f = fdget_raw(fd);
> +	if (!f.file)
> +		return -EBADF;

CLASS(fd_raw, f)(fd), please.

> +	error = vfs_statx_path(&f.file->f_path, flags, stat, request_mask);
> +	fdput(f);
> +	return error;
> +}
> +
>  /**
>   * vfs_statx - Get basic and extra attributes by filename
>   * @dfd: A file descriptor representing the base dir for a relative filename
> @@ -243,36 +285,13 @@ static int vfs_statx(int dfd, struct filename *filename, int flags,
>  retry:
>  	error = filename_lookup(dfd, filename, lookup_flags, &path, NULL);
>  	if (error)
> -		goto out;
> -
> -	error = vfs_getattr(&path, stat, request_mask, flags);
> -
> -	if (request_mask & STATX_MNT_ID_UNIQUE) {
> -		stat->mnt_id = real_mount(path.mnt)->mnt_id_unique;
> -		stat->result_mask |= STATX_MNT_ID_UNIQUE;
> -	} else {
> -		stat->mnt_id = real_mount(path.mnt)->mnt_id;
> -		stat->result_mask |= STATX_MNT_ID;
> -	}
> -
> -	if (path.mnt->mnt_root == path.dentry)
> -		stat->attributes |= STATX_ATTR_MOUNT_ROOT;
> -	stat->attributes_mask |= STATX_ATTR_MOUNT_ROOT;
> -
> -	/* Handle STATX_DIOALIGN for block devices. */
> -	if (request_mask & STATX_DIOALIGN) {
> -		struct inode *inode = d_backing_inode(path.dentry);
> -
> -		if (S_ISBLK(inode->i_mode))
> -			bdev_statx_dioalign(inode, stat);
> -	}
> -
> +		return error;
> +	error = vfs_statx_path(&path, flags, stat, request_mask);
>  	path_put(&path);
>  	if (retry_estale(error, lookup_flags)) {
>  		lookup_flags |= LOOKUP_REVAL;
>  		goto retry;
>  	}
> -out:
>  	return error;
>  }
>  
> @@ -691,6 +710,29 @@ int do_statx(int dfd, struct filename *filename, unsigned int flags,
>  	return cp_statx(&stat, buffer);
>  }
>  
> +int do_statx_fd(int fd, unsigned int flags, unsigned int mask,
> +	     struct statx __user *buffer)
> +{
> +	struct kstat stat;
> +	int error;
> +
> +	if (mask & STATX__RESERVED)
> +		return -EINVAL;
> +	if ((flags & AT_STATX_SYNC_TYPE) == AT_STATX_SYNC_TYPE)
> +		return -EINVAL;
> +
> +	/* STATX_CHANGE_COOKIE is kernel-only for now. Ignore requests
> +	 * from userland.
> +	 */
> +	mask &= ~STATX_CHANGE_COOKIE;
> +
> +	error = vfs_statx_fd(fd, flags, &stat, mask);
> +	if (error)
> +		return error;
> +
> +	return cp_statx(&stat, buffer);
> +}
> +
>  /**
>   * sys_statx - System call to get enhanced stats
>   * @dfd: Base directory to pathwalk from *or* fd to stat.
> @@ -710,6 +752,9 @@ SYSCALL_DEFINE5(statx,
>  	int ret;
>  	struct filename *name;
>  
> +	if (filename == NULL && (flags & AT_EMPTY_PATH))
> +		return do_statx_fd(dfd, flags, mask, buffer);

Maybe use the helper I added

static inline bool vfs_empty_path(int dfd, const char __user *path)
{
       char c;

       if (dfd < 0)
               return false;

       /* We now allow NULL to be used for empty path. */
       if (!path)
               return true;

       if (unlikely(get_user(c, path)))
               return false;

       return !c;
}

> +
>  	name = getname_flags(filename, getname_statx_lookup_flags(flags));
>  	ret = do_statx(dfd, name, flags, mask, buffer);
>  	putname(name);
> diff --git a/io_uring/statx.c b/io_uring/statx.c
> index f7f9b202eec0..21c97aff270d 100644
> --- a/io_uring/statx.c
> +++ b/io_uring/statx.c
> @@ -23,6 +23,7 @@ struct io_statx {
>  int io_statx_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  {
>  	struct io_statx *sx = io_kiocb_to_cmd(req, struct io_statx);
> +	struct filename *filename;
>  	const char __user *path;
>  
>  	if (sqe->buf_index || sqe->splice_fd_in)
> @@ -36,14 +37,13 @@ int io_statx_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  	sx->buffer = u64_to_user_ptr(READ_ONCE(sqe->addr2));
>  	sx->flags = READ_ONCE(sqe->statx_flags);
>  
> -	sx->filename = getname_flags(path,
> -				     getname_statx_lookup_flags(sx->flags));
> -
> -	if (IS_ERR(sx->filename)) {
> -		int ret = PTR_ERR(sx->filename);
> -
> -		sx->filename = NULL;
> -		return ret;
> +	sx->filename = NULL;
> +	if (!(path == NULL && (sx->flags & AT_EMPTY_PATH))) {
> +		filename = getname_flags(path,
> +					 getname_statx_lookup_flags(sx->flags));
> +		if (IS_ERR(filename))
> +			return PTR_ERR(filename);
> +		sx->filename = filename;
>  	}
>  
>  	req->flags |= REQ_F_NEED_CLEANUP;
> @@ -58,7 +58,10 @@ int io_statx(struct io_kiocb *req, unsigned int issue_flags)
>  
>  	WARN_ON_ONCE(issue_flags & IO_URING_F_NONBLOCK);
>  
> -	ret = do_statx(sx->dfd, sx->filename, sx->flags, sx->mask, sx->buffer);
> +	if (sx->filename == NULL && (sx->flags & AT_EMPTY_PATH))
> +		ret = do_statx_fd(sx->dfd, sx->flags, sx->mask, sx->buffer);
> +	else
> +		ret = do_statx(sx->dfd, sx->filename, sx->flags, sx->mask, sx->buffer);
>  	io_req_set_res(req, ret, 0);
>  	return IOU_OK;
>  }
> -- 
> 2.43.0
> 

Re: [PATCH v2] vfs: Shortcut AT_EMPTY_PATH early for statx, and add AT_NO_PATH for statx and fstatat

[Xi Ruoyao]

On Tue, 2024-06-25 at 10:50 +0200, Christian Brauner wrote:
> No, let's not waste AT_* flag space in fear of some hypothetical
> breakage. Let's try it cleanly first and make AT_EMPTY_PATH work with
> NULL paths.
> 
> Note, I started working on this (checks ...) 30th April but then some
> other work came up and I never got back to it (Sorry, Linus). I pushed
> the branch to #vfs.empty.path now. The top three commits was what I had
> started doing.
> 
> It was based on a new vfs_empty_path() helper so we could reuse it for
> other system calls as well.

If I understand correctly, I should submit a patch against
vfs.empty.path making statx(fd, NULL, AT_EMPTY_PATH) work then?

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH v2] vfs: Shortcut AT_EMPTY_PATH early for statx, and add AT_NO_PATH for statx and fstatat

[Mateusz Guzik]

On Tue, Jun 25, 2024 at 12:51 PM Xi Ruoyao <xry111@xry111.site> wrote:
>
> On Tue, 2024-06-25 at 10:50 +0200, Christian Brauner wrote:
> > No, let's not waste AT_* flag space in fear of some hypothetical
> > breakage. Let's try it cleanly first and make AT_EMPTY_PATH work with
> > NULL paths.
> >
> > Note, I started working on this (checks ...) 30th April but then some
> > other work came up and I never got back to it (Sorry, Linus). I pushed
> > the branch to #vfs.empty.path now. The top three commits was what I had
> > started doing.
> >
> > It was based on a new vfs_empty_path() helper so we could reuse it for
> > other system calls as well.
>
> If I understand correctly, I should submit a patch against
> vfs.empty.path making statx(fd, NULL, AT_EMPTY_PATH) work then?
>

I just sorted this out, will be sending a patchset shortly.


-- 
Mateusz Guzik <mjguzik gmail.com>