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

[Christian Brauner <brauner@kernel.org>]

On Fri, Oct 18, 2024 at 12:54:59AM +0100, Al Viro wrote:
> 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.

I know, that's what I suggested to Linus initially but he NAKed it
because he didn't want the extra cycles.

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