diff mbox series

[RFC] getname_maybe_null() - the third variant of pathname copy-in

Message ID 20241009040316.GY4017910@ZenIV (mailing list archive)
State New
Headers show
Series [RFC] getname_maybe_null() - the third variant of pathname copy-in | expand

Commit Message

Al Viro Oct. 9, 2024, 4:03 a.m. UTC 
[
in #work.getname; if nobody objects, I'm going to make #work.xattr pull that.
IMO it's saner than hacks around vfs_empty_path() and it does very similar
logics - with simpler handling on the caller side.
]

Semantics used by statx(2) (and later *xattrat(2)): without AT_EMPTY_PATH
it's standard getname() (i.e. ERR_PTR(-ENOENT) on empty string,
ERR_PTR(-EFAULT) on NULL), with AT_EMPTY_PATH both empty string and
NULL are accepted.
    
Calling conventions: getname_maybe_null(user_pointer, flags) returns
	* pointer to struct filename when non-empty string had been
successfully read
	* ERR_PTR(...) on error
	* NULL if an empty string or NULL pointer had been given
with AT_EMPTY_FLAGS in the flags argument.

It tries to avoid allocation in the last case; it's not always
able to do so, in which case the temporary struct filename instance
is freed and NULL returned anyway.

Fast path is inlined.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
--- 
diff --git a/fs/namei.c b/fs/namei.c
index 4a4a22a08ac2..27eb0a81d9b8 100644

Comments

Christian Brauner Oct. 15, 2024, 2:05 p.m. UTC | #1 
On Wed, Oct 09, 2024 at 05:03:16AM +0100, Al Viro wrote:
> [
> in #work.getname; if nobody objects, I'm going to make #work.xattr pull that.
> IMO it's saner than hacks around vfs_empty_path() and it does very similar
> logics - with simpler handling on the caller side.
> ]
> 
> Semantics used by statx(2) (and later *xattrat(2)): without AT_EMPTY_PATH
> it's standard getname() (i.e. ERR_PTR(-ENOENT) on empty string,
> ERR_PTR(-EFAULT) on NULL), with AT_EMPTY_PATH both empty string and
> NULL are accepted.
>     
> Calling conventions: getname_maybe_null(user_pointer, flags) returns
> 	* pointer to struct filename when non-empty string had been
> successfully read
> 	* ERR_PTR(...) on error
> 	* NULL if an empty string or NULL pointer had been given
> with AT_EMPTY_FLAGS in the flags argument.
> 
> It tries to avoid allocation in the last case; it's not always
> able to do so, in which case the temporary struct filename instance
> is freed and NULL returned anyway.
> 
> Fast path is inlined.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> --- 

Looks good.

Fyi, I'm using your #base.getname as a base for some other work that I'm
currently doing. So please don't rebase #base.getname anymore. ;)

Since you have your #work.xattr and #work.stat using it as base it seems
pretty unlikely anyway but I just thought I mention explicitly that I'm
relying on that #base.getname branch.
Al Viro Oct. 16, 2024, 5:09 a.m. UTC | #2 
On Tue, Oct 15, 2024 at 04:05:15PM +0200, Christian Brauner wrote:
> Looks good.
> 
> Fyi, I'm using your #base.getname as a base for some other work that I'm
> currently doing. So please don't rebase #base.getname anymore. ;)
> 
> Since you have your #work.xattr and #work.stat using it as base it seems
> pretty unlikely anyway but I just thought I mention explicitly that I'm
> relying on that #base.getname branch.

FWIW, I see a problem with that sucker.  The trouble is in the
combination AT_FDCWD, "", AT_EMPTY_PATH.  vfs_empty_path() returns
false on that, so fstatat() in mainline falls back to vfs_statx() and
does the right thing.  This variant does _not_.

Note that your variant of xattr series also ended up failing on e.g.
getxattrat() with such combination:
        if (at_flags & AT_EMPTY_PATH && vfs_empty_path(dfd, pathname)) {
                CLASS(fd, f)(dfd);
                if (!f.file)
                        return -EBADF;
                audit_file(f.file);
                return getxattr(file_mnt_idmap(f.file), file_dentry(f.file),
                                name, value, size);
        }

        lookup_flags = (at_flags & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW;

retry:
        error = user_path_at(dfd, pathname, lookup_flags, &path);

ended up calling user_path_at() with empty pathname and nothing like LOOKUP_EMPTY
in lookup_flags.  Which bails out with -ENOENT, since getname() in there does
so.  My variant bails out with -EBADF and I'd argue that neither is correct.

Not sure what's the sane solution here, need to think for a while...
Christian Brauner Oct. 16, 2024, 8:32 a.m. UTC | #3 
On Wed, Oct 16, 2024 at 06:09:08AM +0100, Al Viro wrote:
> On Tue, Oct 15, 2024 at 04:05:15PM +0200, Christian Brauner wrote:
> > Looks good.
> > 
> > Fyi, I'm using your #base.getname as a base for some other work that I'm
> > currently doing. So please don't rebase #base.getname anymore. ;)
> > 
> > Since you have your #work.xattr and #work.stat using it as base it seems
> > pretty unlikely anyway but I just thought I mention explicitly that I'm
> > relying on that #base.getname branch.
> 
> FWIW, I see a problem with that sucker.  The trouble is in the
> combination AT_FDCWD, "", AT_EMPTY_PATH.  vfs_empty_path() returns

Yeah, we're aware.

> false on that, so fstatat() in mainline falls back to vfs_statx() and
> does the right thing.  This variant does _not_.
> 
> Note that your variant of xattr series also ended up failing on e.g.
> getxattrat() with such combination:
>         if (at_flags & AT_EMPTY_PATH && vfs_empty_path(dfd, pathname)) {
>                 CLASS(fd, f)(dfd);
>                 if (!f.file)
>                         return -EBADF;
>                 audit_file(f.file);
>                 return getxattr(file_mnt_idmap(f.file), file_dentry(f.file),
>                                 name, value, size);
>         }
> 
>         lookup_flags = (at_flags & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW;
> 
> retry:
>         error = user_path_at(dfd, pathname, lookup_flags, &path);
> 
> ended up calling user_path_at() with empty pathname and nothing like LOOKUP_EMPTY
> in lookup_flags.  Which bails out with -ENOENT, since getname() in there does
> so.  My variant bails out with -EBADF and I'd argue that neither is correct.
> 
> Not sure what's the sane solution here, need to think for a while...

Fwiw, in the other thread we concluded to just not care about AT_FDCWD with "".
And so far I agree with that.
Al Viro Oct. 16, 2024, 2 p.m. UTC | #4 
On Wed, Oct 16, 2024 at 10:32:16AM +0200, Christian Brauner wrote:

> > ended up calling user_path_at() with empty pathname and nothing like LOOKUP_EMPTY
> > in lookup_flags.  Which bails out with -ENOENT, since getname() in there does
> > so.  My variant bails out with -EBADF and I'd argue that neither is correct.
> > 
> > Not sure what's the sane solution here, need to think for a while...
> 
> Fwiw, in the other thread we concluded to just not care about AT_FDCWD with "".
> And so far I agree with that.

Subject:?
Christian Brauner Oct. 16, 2024, 2:49 p.m. UTC | #5 
On Wed, Oct 16, 2024 at 03:00:50PM +0100, Al Viro wrote:
> On Wed, Oct 16, 2024 at 10:32:16AM +0200, Christian Brauner wrote:
> 
> > > ended up calling user_path_at() with empty pathname and nothing like LOOKUP_EMPTY
> > > in lookup_flags.  Which bails out with -ENOENT, since getname() in there does
> > > so.  My variant bails out with -EBADF and I'd argue that neither is correct.
> > > 
> > > Not sure what's the sane solution here, need to think for a while...
> > 
> > Fwiw, in the other thread we concluded to just not care about AT_FDCWD with "".
> > And so far I agree with that.
> 
> Subject:?

https://lore.kernel.org/r/CAGudoHHdccL5Lh8zAO-0swqqRCW4GXMSXhq4jQGoVj=UdBK-Lg@mail.gmail.com

Hm, this only speaks about the NULL case.


I just looked through codesearch on github and on debian and the only
example I found was
https://sources.debian.org/src/snapd/2.65.3-1/tests/main/seccomp-statx/test-snapd-statx/bin/statx.py/?hl=71#L71

So really, just special-case it for statx() imho instead of spreading
that ugliness everywhere?
Al Viro Oct. 17, 2024, 11:54 p.m. UTC | #6 
On Wed, Oct 16, 2024 at 04:49:48PM +0200, Christian Brauner wrote:
> On Wed, Oct 16, 2024 at 03:00:50PM +0100, Al Viro wrote:
> > On Wed, Oct 16, 2024 at 10:32:16AM +0200, Christian Brauner wrote:
> > 
> > > > ended up calling user_path_at() with empty pathname and nothing like LOOKUP_EMPTY
> > > > in lookup_flags.  Which bails out with -ENOENT, since getname() in there does
> > > > so.  My variant bails out with -EBADF and I'd argue that neither is correct.
> > > > 
> > > > Not sure what's the sane solution here, need to think for a while...
> > > 
> > > Fwiw, in the other thread we concluded to just not care about AT_FDCWD with "".
> > > And so far I agree with that.
> > 
> > Subject:?
> 
> https://lore.kernel.org/r/CAGudoHHdccL5Lh8zAO-0swqqRCW4GXMSXhq4jQGoVj=UdBK-Lg@mail.gmail.com
> 
> Hm, this only speaks about the NULL case.
> 
> 
> I just looked through codesearch on github and on debian and the only
> example I found was
> https://sources.debian.org/src/snapd/2.65.3-1/tests/main/seccomp-statx/test-snapd-statx/bin/statx.py/?hl=71#L71
> 
> So really, just special-case it for statx() imho instead of spreading
> that ugliness everywhere?

Not sure, TBH.  I wonder if it would be simpler to have filename_lookup()
accept NULL for pathname and have it treat that as "".

Look: all it takes is the following trick
	* add const char *pathname to struct nameidata
	* in __set_nameidata() add
        p->pathname = likely(name) ? name->name : "";
	* in path_init() replace
	const char *s = nd->name->name;
with
	const char *s = nd->pathname;
and we are done.  Oh, and teach putname() to treat NULL as no-op.

With such changes in fs/namei.c we could do
        struct filename *name = getname_maybe_null(filename, flags);

	if (!name && dfd != AT_FDCWD)
		return do_statx_fd(dfd, flags & ~AT_NO_AUTOMOUNT, mask, buffer);
	ret = vfs_statx(dfd, name, statx_flags, stat, STATX_BASIC_STATS);
	putname(name);
	return ret;
in statx(2) and similar in vfs_fstatat().

With that I'm not even sure we want to bother with separate
vfs_statx_fd() - the overhead might very well turn out to
be tolerable.  It is non-zero, but that's a fairly straight
trip through filename_lookup() machinery.  Would require some
profiling, though...
diff mbox series

Patch

--- a/fs/namei.c
+++ b/fs/namei.c
@@ -211,22 +211,38 @@  getname_flags(const char __user *filename, int flags)
 	return result;
 }
 
-struct filename *
-getname_uflags(const char __user *filename, int uflags)
+struct filename *getname_uflags(const char __user *filename, int uflags)
 {
 	int flags = (uflags & AT_EMPTY_PATH) ? LOOKUP_EMPTY : 0;
 
 	return getname_flags(filename, flags);
 }
 
-struct filename *
-getname(const char __user * filename)
+struct filename *getname(const char __user * filename)
 {
 	return getname_flags(filename, 0);
 }
 
-struct filename *
-getname_kernel(const char * filename)
+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;
+}
+
+struct filename *getname_kernel(const char * filename)
 {
 	struct filename *result;
 	int len = strlen(filename) + 1;
diff --git a/fs/stat.c b/fs/stat.c
index 41e598376d7e..aa5bfc41a669 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -326,18 +326,11 @@  int vfs_fstatat(int dfd, const char __user *filename,
 {
 	int ret;
 	int statx_flags = flags | AT_NO_AUTOMOUNT;
-	struct filename *name;
+	struct filename *name = getname_maybe_null(filename, flags);
 
-	/*
-	 * Work around glibc turning fstat() into fstatat(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 (flags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename))
+	if (!name)
 		return vfs_fstat(dfd, stat);
 
-	name = getname_flags(filename, getname_statx_lookup_flags(statx_flags));
 	ret = vfs_statx(dfd, name, statx_flags, stat, STATX_BASIC_STATS);
 	putname(name);
 
@@ -775,7 +768,7 @@  SYSCALL_DEFINE5(statx,
 {
 	int ret;
 	unsigned lflags;
-	struct filename *name;
+	struct filename *name = getname_maybe_null(filename, flags);
 
 	/*
 	 * Short-circuit handling of NULL and "" paths.
@@ -788,10 +781,9 @@  SYSCALL_DEFINE5(statx,
 	 * Supporting this results in the uglification below.
 	 */
 	lflags = flags & ~(AT_NO_AUTOMOUNT | AT_STATX_SYNC_TYPE);
-	if (lflags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename))
+	if (!name)
 		return do_statx_fd(dfd, flags & ~AT_NO_AUTOMOUNT, mask, buffer);
 
-	name = getname_flags(filename, getname_statx_lookup_flags(flags));
 	ret = do_statx(dfd, name, flags, mask, buffer);
 	putname(name);
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e3c603d01337..403258ac2ea2 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2766,6 +2766,16 @@  extern struct filename *getname_flags(const char __user *, int);
 extern struct filename *getname_uflags(const char __user *, int);
 extern struct filename *getname(const char __user *);
 extern struct filename *getname_kernel(const char *);
+extern struct filename *__getname_maybe_null(const char __user *);
+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);
+}
 extern void putname(struct filename *name);
 
 extern int finish_open(struct file *file, struct dentry *dentry,