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!
