[2/2] vfs: Make sure {statx,fstatat}(..., AT_EMPTY_PATH | ..., NULL, ...) behave as (..., AT_EMPTY_PATH | ..., "", ...)

Commit Message

Xi Ruoyao Oct. 7, 2024, 1:08 p.m. UTC

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: [e-mail]
Signed-off-by: Xi Ruoyao <[e-mail]>
---
 fs/stat.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Mateusz Guzik Oct. 8, 2024, 3:57 a.m. UTC | #1

On Mon, Oct 7, 2024 at 3:08 PM Xi Ruoyao <[e-mail]> 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: [e-mail]
> Signed-off-by: Xi Ruoyao <[e-mail]>
> ---
>  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);
>

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.

Al Viro Oct. 8, 2024, 4:16 a.m. UTC | #2

On Tue, Oct 08, 2024 at 05:57:00AM +0200, Mateusz Guzik wrote:
> On Mon, Oct 7, 2024 at 3:08 PM Xi Ruoyao <[e-mail]> 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: [e-mail]
> > Signed-off-by: Xi Ruoyao <[e-mail]>
> > ---
> >  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);
> >
> 
> 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.

Al Viro Oct. 8, 2024, 4:27 a.m. UTC | #3

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;   
}

Al Viro Oct. 8, 2024, 4:52 a.m. UTC | #4

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()...

Xi Ruoyao Oct. 19, 2024, 9:31 a.m. UTC | #5

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;   
> }

Patch

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);