Since Linux 6.11 we support AT_EMPTY_PATH and NULL path for fstatat and statx in "some circumstances" mostly for performance and allowing seccomp audition. But to make the API easier to be documented and used, we should just treat AT_EMPTY_PATH and NULL as is AT_EMPTY_PATH and empty string even if there are no performance or seccomp benefits.
Cc: Miao Wang shankerwangmiao@gmail.com Cc: linux-fsdevel@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org
Xi Ruoyao (2): vfs: support fstatat(..., NULL, AT_EMPTY_PATH | AT_NO_AUTOMOUNT, ...) vfs: Make sure {statx,fstatat}(..., AT_EMPTY_PATH | ..., NULL, ...) behave as (..., AT_EMPTY_PATH | ..., "", ...)
fs/stat.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
Since Linux 4.11 AT_NO_AUTOMOUNT is implied for fstatat. So we should support this like fstatat(..., NULL, AT_EMPTY_PATH, ...) for consistency. Also note that statx(..., NULL, AT_EMPTY_PATH | AT_NO_AUTOMOUNT) is already supported.
Fixes: 27a2d0cb2f38 ("stat: use vfs_empty_path() helper") Cc: stable@vger.kernel.org Signed-off-by: Xi Ruoyao xry111@xry111.site --- fs/stat.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/fs/stat.c b/fs/stat.c index 41e598376d7e..ed9d4fd8ba2c 100644 --- a/fs/stat.c +++ b/fs/stat.c @@ -334,6 +334,7 @@ int vfs_fstatat(int dfd, const char __user *filename, * If AT_EMPTY_PATH is set, we expect the common case to be that * empty path, and avoid doing all the extra pathname work. */ + flags &= ~AT_NO_AUTOMOUNT; if (flags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename)) return vfs_fstat(dfd, stat);
We've supported {statx,fstatat}(real_fd, NULL, AT_EMPTY_PATH, ...) since Linux 6.11 for better performance. However there are other cases, for example using AT_FDCWD as the fd or having AT_SYMLINK_NOFOLLOW in flags, not covered by the fast path. While it may be impossible, too difficult, or not very beneficial to optimize these cases, we should still turn NULL into "" for them in the slow path to make the API easier to be documented and used.
Fixes: 0ef625bba6fb ("vfs: support statx(..., NULL, AT_EMPTY_PATH, ...)") Cc: stable@vger.kernel.org Signed-off-by: Xi Ruoyao xry111@xry111.site --- fs/stat.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/fs/stat.c b/fs/stat.c index ed9d4fd8ba2c..5d1b51c23c62 100644 --- a/fs/stat.c +++ b/fs/stat.c @@ -337,8 +337,11 @@ int vfs_fstatat(int dfd, const char __user *filename, flags &= ~AT_NO_AUTOMOUNT; if (flags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename)) return vfs_fstat(dfd, stat); + else if ((flags & AT_EMPTY_PATH) && !filename) + name = getname_kernel(""); + else + name = getname_flags(filename, getname_statx_lookup_flags(statx_flags));
- name = getname_flags(filename, getname_statx_lookup_flags(statx_flags)); ret = vfs_statx(dfd, name, statx_flags, stat, STATX_BASIC_STATS); putname(name);
@@ -791,8 +794,11 @@ SYSCALL_DEFINE5(statx, lflags = flags & ~(AT_NO_AUTOMOUNT | AT_STATX_SYNC_TYPE); if (lflags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename)) return do_statx_fd(dfd, flags & ~AT_NO_AUTOMOUNT, mask, buffer); + else if ((lflags & AT_EMPTY_PATH) && !filename) + 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);
On Mon, Oct 7, 2024 at 3:08 PM Xi Ruoyao xry111@xry111.site wrote:
We've supported {statx,fstatat}(real_fd, NULL, AT_EMPTY_PATH, ...) since Linux 6.11 for better performance. However there are other cases, for example using AT_FDCWD as the fd or having AT_SYMLINK_NOFOLLOW in flags, not covered by the fast path. While it may be impossible, too difficult, or not very beneficial to optimize these cases, we should still turn NULL into "" for them in the slow path to make the API easier to be documented and used.
Fixes: 0ef625bba6fb ("vfs: support statx(..., NULL, AT_EMPTY_PATH, ...)") Cc: stable@vger.kernel.org Signed-off-by: Xi Ruoyao xry111@xry111.site
fs/stat.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/fs/stat.c b/fs/stat.c index ed9d4fd8ba2c..5d1b51c23c62 100644 --- a/fs/stat.c +++ b/fs/stat.c @@ -337,8 +337,11 @@ int vfs_fstatat(int dfd, const char __user *filename, flags &= ~AT_NO_AUTOMOUNT; if (flags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename)) return vfs_fstat(dfd, stat);
else if ((flags & AT_EMPTY_PATH) && !filename)name = getname_kernel("");elsename = getname_flags(filename, getname_statx_lookup_flags(statx_flags));
name = getname_flags(filename, getname_statx_lookup_flags(statx_flags)); ret = vfs_statx(dfd, name, statx_flags, stat, STATX_BASIC_STATS); putname(name);@@ -791,8 +794,11 @@ SYSCALL_DEFINE5(statx, lflags = flags & ~(AT_NO_AUTOMOUNT | AT_STATX_SYNC_TYPE); if (lflags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename)) return do_statx_fd(dfd, flags & ~AT_NO_AUTOMOUNT, mask, buffer);
else if ((lflags & AT_EMPTY_PATH) && !filename)name = getname_kernel("");elsename = 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);
I thought you are going to patch up the 2 callsites of vfs_empty_path() or add the flags argument to said routine so that it can do the branching internally.
Either way I don't think implementing AT_FDCWD + NULL + AT_EMPTY_PATH with getname_kernel("") is necessary.
On Tue, Oct 08, 2024 at 05:57:00AM +0200, Mateusz Guzik wrote:
On Mon, Oct 7, 2024 at 3:08 PM Xi Ruoyao xry111@xry111.site wrote:
We've supported {statx,fstatat}(real_fd, NULL, AT_EMPTY_PATH, ...) since Linux 6.11 for better performance. However there are other cases, for example using AT_FDCWD as the fd or having AT_SYMLINK_NOFOLLOW in flags, not covered by the fast path. While it may be impossible, too difficult, or not very beneficial to optimize these cases, we should still turn NULL into "" for them in the slow path to make the API easier to be documented and used.
Fixes: 0ef625bba6fb ("vfs: support statx(..., NULL, AT_EMPTY_PATH, ...)") Cc: stable@vger.kernel.org Signed-off-by: Xi Ruoyao xry111@xry111.site
fs/stat.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/fs/stat.c b/fs/stat.c index ed9d4fd8ba2c..5d1b51c23c62 100644 --- a/fs/stat.c +++ b/fs/stat.c @@ -337,8 +337,11 @@ int vfs_fstatat(int dfd, const char __user *filename, flags &= ~AT_NO_AUTOMOUNT; if (flags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename)) return vfs_fstat(dfd, stat);
else if ((flags & AT_EMPTY_PATH) && !filename)name = getname_kernel("");elsename = getname_flags(filename, getname_statx_lookup_flags(statx_flags));
name = getname_flags(filename, getname_statx_lookup_flags(statx_flags)); ret = vfs_statx(dfd, name, statx_flags, stat, STATX_BASIC_STATS); putname(name);@@ -791,8 +794,11 @@ SYSCALL_DEFINE5(statx, lflags = flags & ~(AT_NO_AUTOMOUNT | AT_STATX_SYNC_TYPE); if (lflags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename)) return do_statx_fd(dfd, flags & ~AT_NO_AUTOMOUNT, mask, buffer);
else if ((lflags & AT_EMPTY_PATH) && !filename)name = getname_kernel("");elsename = 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);I thought you are going to patch up the 2 callsites of vfs_empty_path() or add the flags argument to said routine so that it can do the branching internally.
Either way I don't think implementing AT_FDCWD + NULL + AT_EMPTY_PATH with getname_kernel("") is necessary.
Folks, please don't go there. Really. IMO vfs_empty_path() is a wrong API in the first place. Too low-level and racy as well.
See the approach in #work.xattr; I'm going to lift that into fs/namei.c (well, the slow path - everything after "if path is NULL, we are done") and yes, fs/stat.c users get handled better that way.
On Tue, Oct 08, 2024 at 05:16:21AM +0100, Al Viro wrote:
Folks, please don't go there. Really. IMO vfs_empty_path() is a wrong API in the first place. Too low-level and racy as well.
See the approach in #work.xattr; I'm going to lift that into fs/namei.c (well, the slow path - everything after "if path is NULL, we are done") and yes, fs/stat.c users get handled better that way.
FWIW, the intermediate (just after that commit) state of those functions is
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_maybe_null(filename, flags);
if (!name) return vfs_fstat(dfd, stat);
ret = vfs_statx(dfd, name, statx_flags, stat, STATX_BASIC_STATS); putname(name);
return ret; }
and
SYSCALL_DEFINE5(statx, int, dfd, const char __user *, filename, unsigned, flags, unsigned int, mask, struct statx __user *, buffer) { int ret; unsigned lflags; struct filename *name = getname_maybe_null(filename, flags);
/* * Short-circuit handling of NULL and "" paths. * * For a NULL path we require and accept only the AT_EMPTY_PATH flag * (possibly |'d with AT_STATX flags). * * However, glibc on 32-bit architectures implements fstatat as statx * with the "" pathname and AT_NO_AUTOMOUNT | AT_EMPTY_PATH flags. * Supporting this results in the uglification below. */ lflags = flags & ~(AT_NO_AUTOMOUNT | AT_STATX_SYNC_TYPE); if (!name) return do_statx_fd(dfd, flags & ~AT_NO_AUTOMOUNT, mask, buffer);
ret = do_statx(dfd, name, flags, mask, buffer); putname(name);
return ret; }
static inline struct filename *getname_maybe_null(const char __user *name, int flags) { if (!(flags & AT_EMPTY_PATH)) return getname(name);
if (!name) return NULL; return __getname_maybe_null(name); }
struct filename *__getname_maybe_null(const char __user *pathname) { struct filename *name; char c;
/* try to save on allocations; loss on um, though */ if (get_user(c, pathname)) return ERR_PTR(-EFAULT); if (!c) return NULL;
name = getname_flags(pathname, LOOKUP_EMPTY); if (!IS_ERR(name) && !(name->name[0])) { putname(name); name = NULL; } return name; }
On Tue, Oct 08, 2024 at 05:27:51AM +0100, Al Viro wrote:
On Tue, Oct 08, 2024 at 05:16:21AM +0100, Al Viro wrote:
Folks, please don't go there. Really. IMO vfs_empty_path() is a wrong API in the first place. Too low-level and racy as well.
See the approach in #work.xattr; I'm going to lift that into fs/namei.c (well, the slow path - everything after "if path is NULL, we are done") and yes, fs/stat.c users get handled better that way.
FWIW, the intermediate (just after that commit) state of those functions is
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_maybe_null(filename, flags);
if (!name) return vfs_fstat(dfd, stat); ret = vfs_statx(dfd, name, statx_flags, stat, STATX_BASIC_STATS); putname(name); return ret;}
and
SYSCALL_DEFINE5(statx, int, dfd, const char __user *, filename, unsigned, flags, unsigned int, mask, struct statx __user *, buffer) { int ret; unsigned lflags; struct filename *name = getname_maybe_null(filename, flags);
/* * Short-circuit handling of NULL and "" paths. * * For a NULL path we require and accept only the AT_EMPTY_PATH flag * (possibly |'d with AT_STATX flags). * * However, glibc on 32-bit architectures implements fstatat as statx * with the "" pathname and AT_NO_AUTOMOUNT | AT_EMPTY_PATH flags. * Supporting this results in the uglification below. */ lflags = flags & ~(AT_NO_AUTOMOUNT | AT_STATX_SYNC_TYPE); if (!name) return do_statx_fd(dfd, flags & ~AT_NO_AUTOMOUNT, mask, buffer); ret = do_statx(dfd, name, flags, mask, buffer); putname(name); return ret;}
static inline struct filename *getname_maybe_null(const char __user *name, int flags) { if (!(flags & AT_EMPTY_PATH)) return getname(name);
if (!name) return NULL; return __getname_maybe_null(name);}
struct filename *__getname_maybe_null(const char __user *pathname) { struct filename *name; char c;
/* try to save on allocations; loss on um, though */ if (get_user(c, pathname)) return ERR_PTR(-EFAULT); if (!c) return NULL; name = getname_flags(pathname, LOOKUP_EMPTY); if (!IS_ERR(name) && !(name->name[0])) { putname(name); name = NULL; } return name;}
Incidentally, the name 'getname_statx_lookup_flags' is an atrocity: * getname and its variants do not give a fuck for the state of any flags besides AT_EMPTY_PATH * lookups, OTOH, ignore LOOKUP_EMPTY (which shouldn't have been in the LOOKUP_... namespace to start with).
Another fun question: why do we play with setting ->mnt_id, etc. in vfs_statx_path() if vfs_getattr() returns non-zero? Or when we hit it via vfs_statx() from vfs_fstatat()...
On Tue, 2024-10-08 at 05:27 +0100, Al Viro wrote:
On Tue, Oct 08, 2024 at 05:16:21AM +0100, Al Viro wrote:
Folks, please don't go there. Really. IMO vfs_empty_path() is a wrong API in the first place. Too low-level and racy as well.
See the approach in #work.xattr; I'm going to lift that into fs/namei.c (well, the slow path - everything after "if path is NULL, we are done") and yes, fs/stat.c users get handled better that way.
So IIUC I just need to sit down here and wait for your branch to be merged?
FWIW, the intermediate (just after that commit) state of those functions is
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_maybe_null(filename, flags);
if (!name) return vfs_fstat(dfd, stat);
ret = vfs_statx(dfd, name, statx_flags, stat, STATX_BASIC_STATS); putname(name);
return ret; }
and
SYSCALL_DEFINE5(statx, int, dfd, const char __user *, filename, unsigned, flags, unsigned int, mask, struct statx __user *, buffer) { int ret; unsigned lflags; struct filename *name = getname_maybe_null(filename, flags);
/* * Short-circuit handling of NULL and "" paths. * * For a NULL path we require and accept only the AT_EMPTY_PATH flag * (possibly |'d with AT_STATX flags). * * However, glibc on 32-bit architectures implements fstatat as statx * with the "" pathname and AT_NO_AUTOMOUNT | AT_EMPTY_PATH flags. * Supporting this results in the uglification below. */ lflags = flags & ~(AT_NO_AUTOMOUNT | AT_STATX_SYNC_TYPE); if (!name) return do_statx_fd(dfd, flags & ~AT_NO_AUTOMOUNT, mask, buffer);
ret = do_statx(dfd, name, flags, mask, buffer); putname(name);
return ret; }
static inline struct filename *getname_maybe_null(const char __user *name, int flags) { if (!(flags & AT_EMPTY_PATH)) return getname(name);
if (!name) return NULL; return __getname_maybe_null(name); }
struct filename *__getname_maybe_null(const char __user *pathname) { struct filename *name; char c;
/* try to save on allocations; loss on um, though */ if (get_user(c, pathname)) return ERR_PTR(-EFAULT); if (!c) return NULL;
name = getname_flags(pathname, LOOKUP_EMPTY); if (!IS_ERR(name) && !(name->name[0])) { putname(name); name = NULL; } return name; }
linux-stable-mirror@lists.linaro.org
-
Al Viro -
Mateusz Guzik -
Xi Ruoyao