| Message ID | 20210730150134.216126-4-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtiofsd: Allow using file handles instead of O_PATH FDs | expand |
On Fri, Jul 30, 2021 at 05:01:27PM +0200, Max Reitz wrote: > The xattr functions want a non-O_PATH FD, so they reopen the lo_inode.fd > with the flags they need through /proc/self/fd. > > Similarly, lo_opendir() needs an O_RDONLY FD. Instead of the > /proc/self/fd trick, it just uses openat(fd, "."), because the FD is > guaranteed to be a directory, so this works. Ok, given now lo_opendir() will use lo_inode_open(), it will switch to using proc O_PATH fd trick. I guess that should be fine. Vivek > > All cases have one problem in common, though: In the future, when we may > have a file handle in the lo_inode instead of an FD, querying an > lo_inode FD may incur an open_by_handle_at() call. It does not make > sense to then reopen that FD with custom flags, those should have been > passed to open_by_handle_at() instead. > > Use lo_inode_open() instead of openat(). As part of the file handle > change, lo_inode_open() will be made to invoke openat() only if > lo_inode.fd is valid. Otherwise, it will invoke open_by_handle_at() > with the right flags from the start. > > Consequently, after this patch, lo_inode_open() is the only place to > invoke openat() to reopen an existing FD with different flags. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tools/virtiofsd/passthrough_ll.c | 43 ++++++++++++++++++++------------ > 1 file changed, 27 insertions(+), 16 deletions(-) > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > index fb5e073e6a..a444c3a7e2 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -1729,18 +1729,26 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, > { > int error = ENOMEM; > struct lo_data *lo = lo_data(req); > - struct lo_dirp *d; > + struct lo_inode *inode; > + struct lo_dirp *d = NULL; > int fd; > ssize_t fh; > > + inode = lo_inode(req, ino); > + if (!inode) { > + error = EBADF; > + goto out_err; > + } > + > d = calloc(1, sizeof(struct lo_dirp)); > if (d == NULL) { > goto out_err; > } > > - fd = openat(lo_fd(req, ino), ".", O_RDONLY); > - if (fd == -1) { > - goto out_errno; > + fd = lo_inode_open(lo, inode, O_RDONLY); > + if (fd < 0) { > + error = -fd; > + goto out_err; > } > > d->dp = fdopendir(fd); > @@ -1769,6 +1777,7 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, > out_errno: > error = errno; > out_err: > + lo_inode_put(lo, &inode); > if (d) { > if (d->dp) { > closedir(d->dp); > @@ -2973,7 +2982,6 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, > } > } > > - sprintf(procname, "%i", inode->fd); > /* > * It is not safe to open() non-regular/non-dir files in file server > * unless O_PATH is used, so use that method for regular files/dir > @@ -2981,13 +2989,15 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, > * Otherwise, call fchdir() to avoid open(). > */ > if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) { > - fd = openat(lo->proc_self_fd, procname, O_RDONLY); > + fd = lo_inode_open(lo, inode, O_RDONLY); > if (fd < 0) { > - goto out_err; > + saverr = -fd; > + goto out; > } > ret = fgetxattr(fd, name, value, size); > saverr = ret == -1 ? errno : 0; > } else { > + sprintf(procname, "%i", inode->fd); > /* fchdir should not fail here */ > FCHDIR_NOFAIL(lo->proc_self_fd); > ret = getxattr(procname, name, value, size); > @@ -3054,15 +3064,16 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size) > } > } > > - sprintf(procname, "%i", inode->fd); > if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) { > - fd = openat(lo->proc_self_fd, procname, O_RDONLY); > + fd = lo_inode_open(lo, inode, O_RDONLY); > if (fd < 0) { > - goto out_err; > + saverr = -fd; > + goto out; > } > ret = flistxattr(fd, value, size); > saverr = ret == -1 ? errno : 0; > } else { > + sprintf(procname, "%i", inode->fd); > /* fchdir should not fail here */ > FCHDIR_NOFAIL(lo->proc_self_fd); > ret = listxattr(procname, value, size); > @@ -3211,14 +3222,14 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, > * setxattr() on the link's filename there. > */ > open_inode = S_ISREG(inode->filetype) || S_ISDIR(inode->filetype); > - sprintf(procname, "%i", inode->fd); > if (open_inode) { > - fd = openat(lo->proc_self_fd, procname, O_RDONLY); > + fd = lo_inode_open(lo, inode, O_RDONLY); > if (fd < 0) { > - saverr = errno; > + saverr = -fd; > goto out; > } > } else { > + sprintf(procname, "%i", inode->fd); > /* fchdir should not fail here */ > FCHDIR_NOFAIL(lo->proc_self_fd); > } > @@ -3317,16 +3328,16 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name) > fuse_log(FUSE_LOG_DEBUG, "lo_removexattr(ino=%" PRIu64 ", name=%s)\n", ino, > name); > > - sprintf(procname, "%i", inode->fd); > if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) { > - fd = openat(lo->proc_self_fd, procname, O_RDONLY); > + fd = lo_inode_open(lo, inode, O_RDONLY); > if (fd < 0) { > - saverr = errno; > + saverr = -fd; > goto out; > } > ret = fremovexattr(fd, name); > saverr = ret == -1 ? errno : 0; > } else { > + sprintf(procname, "%i", inode->fd); > /* fchdir should not fail here */ > FCHDIR_NOFAIL(lo->proc_self_fd); > ret = removexattr(procname, name); > -- > 2.31.1 >
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index fb5e073e6a..a444c3a7e2 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -1729,18 +1729,26 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, { int error = ENOMEM; struct lo_data *lo = lo_data(req); - struct lo_dirp *d; + struct lo_inode *inode; + struct lo_dirp *d = NULL; int fd; ssize_t fh; + inode = lo_inode(req, ino); + if (!inode) { + error = EBADF; + goto out_err; + } + d = calloc(1, sizeof(struct lo_dirp)); if (d == NULL) { goto out_err; } - fd = openat(lo_fd(req, ino), ".", O_RDONLY); - if (fd == -1) { - goto out_errno; + fd = lo_inode_open(lo, inode, O_RDONLY); + if (fd < 0) { + error = -fd; + goto out_err; } d->dp = fdopendir(fd); @@ -1769,6 +1777,7 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, out_errno: error = errno; out_err: + lo_inode_put(lo, &inode); if (d) { if (d->dp) { closedir(d->dp); @@ -2973,7 +2982,6 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, } } - sprintf(procname, "%i", inode->fd); /* * It is not safe to open() non-regular/non-dir files in file server * unless O_PATH is used, so use that method for regular files/dir @@ -2981,13 +2989,15 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, * Otherwise, call fchdir() to avoid open(). */ if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) { - fd = openat(lo->proc_self_fd, procname, O_RDONLY); + fd = lo_inode_open(lo, inode, O_RDONLY); if (fd < 0) { - goto out_err; + saverr = -fd; + goto out; } ret = fgetxattr(fd, name, value, size); saverr = ret == -1 ? errno : 0; } else { + sprintf(procname, "%i", inode->fd); /* fchdir should not fail here */ FCHDIR_NOFAIL(lo->proc_self_fd); ret = getxattr(procname, name, value, size); @@ -3054,15 +3064,16 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size) } } - sprintf(procname, "%i", inode->fd); if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) { - fd = openat(lo->proc_self_fd, procname, O_RDONLY); + fd = lo_inode_open(lo, inode, O_RDONLY); if (fd < 0) { - goto out_err; + saverr = -fd; + goto out; } ret = flistxattr(fd, value, size); saverr = ret == -1 ? errno : 0; } else { + sprintf(procname, "%i", inode->fd); /* fchdir should not fail here */ FCHDIR_NOFAIL(lo->proc_self_fd); ret = listxattr(procname, value, size); @@ -3211,14 +3222,14 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, * setxattr() on the link's filename there. */ open_inode = S_ISREG(inode->filetype) || S_ISDIR(inode->filetype); - sprintf(procname, "%i", inode->fd); if (open_inode) { - fd = openat(lo->proc_self_fd, procname, O_RDONLY); + fd = lo_inode_open(lo, inode, O_RDONLY); if (fd < 0) { - saverr = errno; + saverr = -fd; goto out; } } else { + sprintf(procname, "%i", inode->fd); /* fchdir should not fail here */ FCHDIR_NOFAIL(lo->proc_self_fd); } @@ -3317,16 +3328,16 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name) fuse_log(FUSE_LOG_DEBUG, "lo_removexattr(ino=%" PRIu64 ", name=%s)\n", ino, name); - sprintf(procname, "%i", inode->fd); if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) { - fd = openat(lo->proc_self_fd, procname, O_RDONLY); + fd = lo_inode_open(lo, inode, O_RDONLY); if (fd < 0) { - saverr = errno; + saverr = -fd; goto out; } ret = fremovexattr(fd, name); saverr = ret == -1 ? errno : 0; } else { + sprintf(procname, "%i", inode->fd); /* fchdir should not fail here */ FCHDIR_NOFAIL(lo->proc_self_fd); ret = removexattr(procname, name);
The xattr functions want a non-O_PATH FD, so they reopen the lo_inode.fd with the flags they need through /proc/self/fd. Similarly, lo_opendir() needs an O_RDONLY FD. Instead of the /proc/self/fd trick, it just uses openat(fd, "."), because the FD is guaranteed to be a directory, so this works. All cases have one problem in common, though: In the future, when we may have a file handle in the lo_inode instead of an FD, querying an lo_inode FD may incur an open_by_handle_at() call. It does not make sense to then reopen that FD with custom flags, those should have been passed to open_by_handle_at() instead. Use lo_inode_open() instead of openat(). As part of the file handle change, lo_inode_open() will be made to invoke openat() only if lo_inode.fd is valid. Otherwise, it will invoke open_by_handle_at() with the right flags from the start. Consequently, after this patch, lo_inode_open() is the only place to invoke openat() to reopen an existing FD with different flags. Signed-off-by: Max Reitz <mreitz@redhat.com> --- tools/virtiofsd/passthrough_ll.c | 43 ++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 16 deletions(-)