[PATCH 0/2] vfs: fstatat, statx: Consistently accept AT_EMPTY_PATH and NULL path

[PATCH 0/2] vfs: fstatat, statx: Consistently accept AT_EMPTY_PATH and NULL path

[Xi Ruoyao]

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

-- 
2.46.2

[PATCH 1/2] vfs: support fstatat(..., NULL, AT_EMPTY_PATH | AT_NO_AUTOMOUNT, ...)

[Xi Ruoyao]

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

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

[Xi Ruoyao]

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

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

[Mateusz Guzik]

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("");
> +       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.

-- 
Mateusz Guzik <mjguzik gmail.com>

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

[Al Viro]

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("");
> > +       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.

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

[Al Viro]

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

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

[Al Viro]

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

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

[Xi Ruoyao]

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

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