On Fri, Apr 25, 2025 at 3:33 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Fri 25-04-25 10:45:22, Christian Brauner wrote:
> > On Thu, Apr 24, 2025 at 05:45:17PM +0200, Mateusz Guzik wrote:
> > > On Thu, Apr 24, 2025 at 03:22:47PM +0200, Jan Kara wrote:
> > > > Currently, setxattrat(2) and getxattrat(2) are wrongly handling the
> > > > calls of the from setxattrat(AF_FDCWD, NULL, AT_EMPTY_PATH, ...) and
> > > > fail with -EBADF error instead of operating on CWD. Fix it.
> > > >
> > > > Fixes: 6140be90ec70 ("fs/xattr: add *at family syscalls")
> > > > Signed-off-by: Jan Kara <jack@xxxxxxx>
> > > > ---
> > > > fs/xattr.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/xattr.c b/fs/xattr.c
> > > > index 02bee149ad96..fabb2a04501e 100644
> > > > --- a/fs/xattr.c
> > > > +++ b/fs/xattr.c
> > > > @@ -703,7 +703,7 @@ static int path_setxattrat(int dfd, const char __user *pathname,
> > > > return error;
> > > >
> > > > filename = getname_maybe_null(pathname, at_flags);
> > > > - if (!filename) {
> > > > + if (!filename && dfd >= 0) {
> > > > CLASS(fd, f)(dfd);
> > > > if (fd_empty(f))
> > > > error = -EBADF;
> > > > @@ -847,7 +847,7 @@ static ssize_t path_getxattrat(int dfd, const char __user *pathname,
> > > > return error;
> > > >
> > > > filename = getname_maybe_null(pathname, at_flags);
> > > > - if (!filename) {
> > > > + if (!filename && dfd >= 0) {
> > > > CLASS(fd, f)(dfd);
> > > > if (fd_empty(f))
> > > > return -EBADF;
> > >
> > > Is there any code which legitimately does not follow this pattern?
> > >
> > > With some refactoring getname_maybe_null() could handle the fd thing,
> > > notably return the NULL pointer if the name is empty. This could bring
> > > back the invariant that the path argument is not NULL.
> > >
> > > Something like this:
> > > static inline struct filename *getname_maybe_null(int fd, const char __user *name, int flags)
> > > {
> > > if (!(flags & AT_EMPTY_PATH))
> > > return getname(name);
> > >
> > > if (!name && fd >= 0)
> > > return NULL;
> > > return __getname_maybe_null(fd, name);
> > > }
> > >
> > > struct filename *__getname_maybe_null(int fd, const char __user *pathname)
> > > {
> > > char c;
> > >
> > > if (fd >= 0) {
> > > /* try to save on allocations; loss on um, though */
> > > if (get_user(c, pathname))
> > > return ERR_PTR(-EFAULT);
> > > if (!c)
> > > return NULL;
> > > }
> > >
> > > /* we alloc suffer the allocation of the buffer. worst case, if
> > > * the name turned empty in the meantime, we return it and
> > > * handle it the old-fashioned way.
> > > /
> > > return getname_flags(pathname, LOOKUP_EMPTY);
> > > }
> > >
> > > Then callers would look like this:
> > > filename = getname_maybe_null(dfd, pathname, at_flags);
> > > if (!filename) {
> > > /* fd handling goes here */
> > > CLASS(fd, f)(dfd);
> > > ....
> > >
> > > } else {
> > > /* regular path handling goes here */
> > > }
> > >
> > >
> > > set_nameidata() would lose this branch:
> > > p->pathname = likely(name) ? name->name : "";
> > >
> > > and putname would convert IS_ERR_OR_NULL (which is 2 branches) into one,
> > > maybe like so:
> > > - if (IS_ERR_OR_NULL(name))
> > > + VFS_BUG_ON(!name);
> > > +
> > > + if (IS_ERR(name))
> > > return;
> > >
> > > i think this would be an ok cleanup
> >
> > Not opposed, but please for -next and Jan's thing as a backportable fix,
> > please. Thanks!
>
> Exactly, I agree the code is pretty subtle and ugly. It shouldn't take
> several engineers to properly call a function to lookup a file :) So
> some cleanup and refactoring is definitely long overdue but for now I
> wanted some minimal fix which is easy to backport to stable.
>
> When we speak about refactoring: Is there a reason why user_path_at()
> actually doesn't handle NULL 'name' as empty like we do it in *xattrat()
> syscalls? I understand this will make all _at() syscalls accept NULL name
> with AT_EMPTY_PATH but is that a problem?
Is there a benefit for doing it though?
I think the entire AT_EMPTY_PATH and NULL thing is trainwreck which
needs to be reasonably contained instead. In particular the flag has
most regrettable semantics of requiring an actual path (the NULL thing
is a Linux extension) and being a nop if the path is not empty.
The entire thing is a kludge for syscalls which don't have an fd-only
variant and imo was the wrong way to approach this (provide fd-only
variants instead), but it's too late now.
user_path_at() always returns a path (go figure). Suppose it got
extended with the fuckery and some userspace started to rely on it.
Part of the benefit of having a fd-based op and knowing it is fd-based
is that you know the inode itself is secured by liveness of the file
object. If the calling thread is a part of a single-threaded process,
then there is the extra benefit of eliding atomics on the file thing
(reducing single-threaded cost). If the thing is multi-threaded,
atomics are only done on the file (not the inode), which scales better
if other procs use a different file obj for the same inode.
Or to put it differently, if user_path_at() keeps returning a path
like it does now *and* is relied on for AT_EMPTY_PATH fuckery, it is
going to impose extra overhead on its consumers.
Suppose one will decide to combat it. Then the routine will have to
copy path from the file without refing it and return an indicator
what's needed -- path_put for a real path handling, fput for fd-only
in a multithreaded proc [but then also it will need to return the
found file obj] and nothing for a fd-only in a single-threaded proc.
I think that's ugly af and completely unnecessary.
--
Mateusz Guzik <mjguzik gmail.com>
