On Wed, Dec 29, 2021 at 12:30:02PM -0800, Stefan Roesch wrote:
> +static int io_getxattr(struct io_kiocb *req, unsigned int issue_flags)
> +{
> + struct io_xattr *ix = &req->xattr;
> + unsigned int lookup_flags = LOOKUP_FOLLOW;
> + struct path path;
> + int ret;
> +
> + if (issue_flags & IO_URING_F_NONBLOCK)
> + return -EAGAIN;
> +
> +retry:
> + ret = do_user_path_at_empty(AT_FDCWD, ix->filename, lookup_flags, &path);
> + if (!ret) {
> + ret = do_getxattr(mnt_user_ns(path.mnt),
> + path.dentry,
> + ix->ctx.kname->name,
> + (void __user *)ix->ctx.value,
> + ix->ctx.size);
> +
> + path_put(&path);
> + if (retry_estale(ret, lookup_flags)) {
> + lookup_flags |= LOOKUP_REVAL;
> + goto retry;
> + }
> + }
> + putname(ix->filename);
> +
> + __io_getxattr_finish(req, ret);
> + return 0;
> +}
Looking at that one... Is there any reason to have that loop (from retry: to
putname() call) outside of fs/xattr.c? Come to think of that, why bother
polluting your struct io_xattr with ->filename?
Note, BTW, that we already have this:
static ssize_t path_getxattr(const char __user *pathname,
const char __user *name, void __user *value,
size_t size, unsigned int lookup_flags)
{
struct path path;
ssize_t error;
retry:
error = user_path_at(AT_FDCWD, pathname, lookup_flags, &path);
if (error)
return error;
error = getxattr(mnt_user_ns(path.mnt), path.dentry, name, value, size);
path_put(&path);
if (retry_estale(error, lookup_flags)) {
lookup_flags |= LOOKUP_REVAL;
goto retry;
}
return error;
}
in there. The only potential benefit here would be to avoid repeated getname
in case of having hit -ESTALE and going to repeat the entire fucking pathwalk
with maximal paranoia, asking the server(s) involved to revalidate on every
step, etc.
If we end up going there, who the hell *cares* about the costs of less than
a page worth of copy_from_user()? We are already on a very slow path as it
is, so what's the point?
