* [PATCH 1/9] virtiofsd: Add TempFd structure
2021-06-04 16:13 [PATCH 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs Max Reitz
@ 2021-06-04 16:13 ` Max Reitz
2021-06-04 16:13 ` [PATCH 2/9] virtiofsd: Use lo_inode_open() instead of openat() Max Reitz
` (8 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Max Reitz @ 2021-06-04 16:13 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: Dr . David Alan Gilbert, Stefan Hajnoczi, Max Reitz
We are planning to add file handles to lo_inode objects as an
alternative to lo_inode.fd. That means that everywhere where we
currently reference lo_inode.fd, we will have to open a temporary file
descriptor that needs to be closed after use.
So instead of directly accessing lo_inode.fd, there will be a helper
function (lo_inode_fd()) that either returns lo_inode.fd, or opens a new
file descriptor with open_by_handle_at(). It encapsulates this result
in a TempFd structure to let the caller know whether the FD needs to be
closed after use (opened from the handle) or not (copied from
lo_inode.fd).
By using g_auto(TempFd) to store this result, callers will not even have
to care about closing a temporary FD after use. It will be done
automatically once the object goes out of scope.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tools/virtiofsd/passthrough_ll.c | 49 ++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 49c21fd855..a4674aba80 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -174,6 +174,28 @@ struct lo_data {
int user_killpriv_v2, killpriv_v2;
};
+/**
+ * Represents a file descriptor that may either be owned by this
+ * TempFd, or only referenced (i.e. the ownership belongs to some
+ * other object, and the value has just been copied into this TempFd).
+ *
+ * The purpose of this encapsulation is to be used as g_auto(TempFd)
+ * to automatically clean up owned file descriptors when this object
+ * goes out of scope.
+ *
+ * Use temp_fd_steal() to get an owned file descriptor that will not
+ * be closed when the TempFd goes out of scope.
+ */
+typedef struct {
+ int fd;
+ bool owned; /* fd owned by this object? */
+} TempFd;
+
+#define TEMP_FD_INIT ((TempFd) { .fd = -1, .owned = false })
+
+static void temp_fd_clear(TempFd *temp_fd);
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(TempFd, temp_fd_clear);
+
static const struct fuse_opt lo_opts[] = {
{ "sandbox=namespace",
offsetof(struct lo_data, sandbox),
@@ -249,6 +271,33 @@ static struct lo_data *lo_data(fuse_req_t req)
return (struct lo_data *)fuse_req_userdata(req);
}
+/**
+ * Clean-up function for TempFds
+ */
+static void temp_fd_clear(TempFd *temp_fd)
+{
+ if (temp_fd->owned) {
+ close(temp_fd->fd);
+ *temp_fd = TEMP_FD_INIT;
+ }
+}
+
+/**
+ * Return an owned fd from *temp_fd that will not be closed when
+ * *temp_fd goes out of scope.
+ *
+ * (TODO: Remove __attribute__ once this is used.)
+ */
+static __attribute__((unused)) int temp_fd_steal(TempFd *temp_fd)
+{
+ if (temp_fd->owned) {
+ temp_fd->owned = false;
+ return temp_fd->fd;
+ } else {
+ return dup(temp_fd->fd);
+ }
+}
+
/*
* Load capng's state from our saved state if the current thread
* hadn't previously been loaded.
--
2.31.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 2/9] virtiofsd: Use lo_inode_open() instead of openat()
2021-06-04 16:13 [PATCH 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs Max Reitz
2021-06-04 16:13 ` [PATCH 1/9] virtiofsd: Add TempFd structure Max Reitz
@ 2021-06-04 16:13 ` Max Reitz
2021-06-04 16:13 ` [PATCH 3/9] virtiofsd: Add lo_inode_fd() helper Max Reitz
` (7 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Max Reitz @ 2021-06-04 16:13 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: Dr . David Alan Gilbert, Stefan Hajnoczi, Max Reitz
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(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index a4674aba80..436f771d2a 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -1645,18 +1645,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);
@@ -1685,6 +1693,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);
@@ -2827,7 +2836,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
@@ -2835,12 +2843,14 @@ 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);
} else {
+ sprintf(procname, "%i", inode->fd);
/* fchdir should not fail here */
FCHDIR_NOFAIL(lo->proc_self_fd);
ret = getxattr(procname, name, value, size);
@@ -2906,14 +2916,15 @@ 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);
} else {
+ sprintf(procname, "%i", inode->fd);
/* fchdir should not fail here */
FCHDIR_NOFAIL(lo->proc_self_fd);
ret = listxattr(procname, value, size);
@@ -3039,15 +3050,15 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
fuse_log(FUSE_LOG_DEBUG, "lo_setxattr(ino=%" PRIu64
", name=%s value=%s size=%zd)\n", ino, name, value, 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) {
- saverr = errno;
+ saverr = -fd;
goto out;
}
ret = fsetxattr(fd, name, value, size, flags);
} else {
+ sprintf(procname, "%i", inode->fd);
/* fchdir should not fail here */
FCHDIR_NOFAIL(lo->proc_self_fd);
ret = setxattr(procname, name, value, size, flags);
@@ -3105,15 +3116,15 @@ 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);
} else {
+ sprintf(procname, "%i", inode->fd);
/* fchdir should not fail here */
FCHDIR_NOFAIL(lo->proc_self_fd);
ret = removexattr(procname, name);
--
2.31.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 3/9] virtiofsd: Add lo_inode_fd() helper
2021-06-04 16:13 [PATCH 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs Max Reitz
2021-06-04 16:13 ` [PATCH 1/9] virtiofsd: Add TempFd structure Max Reitz
2021-06-04 16:13 ` [PATCH 2/9] virtiofsd: Use lo_inode_open() instead of openat() Max Reitz
@ 2021-06-04 16:13 ` Max Reitz
2021-06-04 16:13 ` [PATCH 4/9] virtiofsd: Let lo_fd() return a TempFd Max Reitz
` (6 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Max Reitz @ 2021-06-04 16:13 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: Dr . David Alan Gilbert, Stefan Hajnoczi, Max Reitz
Once we let lo_inode.fd be optional, we will need its users to open the
file handle stored in lo_inode instead. This function will do that.
For now, it just returns lo_inode.fd, though.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tools/virtiofsd/passthrough_ll.c | 138 ++++++++++++++++++++++++++-----
1 file changed, 117 insertions(+), 21 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 436f771d2a..46c9dfe200 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -629,6 +629,16 @@ static struct lo_inode *lo_inode(fuse_req_t req, fuse_ino_t ino)
return elem->inode;
}
+static int lo_inode_fd(const struct lo_inode *inode, TempFd *tfd)
+{
+ *tfd = (TempFd) {
+ .fd = inode->fd,
+ .owned = false,
+ };
+
+ return 0;
+}
+
/*
* TODO Remove this helper and force callers to hold an inode refcount until
* they are done with the fd. This will be done in a later patch to make
@@ -790,11 +800,11 @@ static int lo_fi_fd(fuse_req_t req, struct fuse_file_info *fi)
static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
int valid, struct fuse_file_info *fi)
{
+ g_auto(TempFd) inode_fd = TEMP_FD_INIT;
int saverr;
char procname[64];
struct lo_data *lo = lo_data(req);
struct lo_inode *inode;
- int ifd;
int res;
int fd = -1;
@@ -804,7 +814,11 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
return;
}
- ifd = inode->fd;
+ res = lo_inode_fd(inode, &inode_fd);
+ if (res < 0) {
+ saverr = -res;
+ goto out_err;
+ }
/* If fi->fh is invalid we'll report EBADF later */
if (fi) {
@@ -815,7 +829,7 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
if (fi) {
res = fchmod(fd, attr->st_mode);
} else {
- sprintf(procname, "%i", ifd);
+ sprintf(procname, "%i", inode_fd.fd);
res = fchmodat(lo->proc_self_fd, procname, attr->st_mode, 0);
}
if (res == -1) {
@@ -827,12 +841,13 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
uid_t uid = (valid & FUSE_SET_ATTR_UID) ? attr->st_uid : (uid_t)-1;
gid_t gid = (valid & FUSE_SET_ATTR_GID) ? attr->st_gid : (gid_t)-1;
- saverr = drop_security_capability(lo, ifd);
+ saverr = drop_security_capability(lo, inode_fd.fd);
if (saverr) {
goto out_err;
}
- res = fchownat(ifd, "", uid, gid, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
+ res = fchownat(inode_fd.fd, "", uid, gid,
+ AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
if (res == -1) {
saverr = errno;
goto out_err;
@@ -911,7 +926,7 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
if (fi) {
res = futimens(fd, tv);
} else {
- sprintf(procname, "%i", inode->fd);
+ sprintf(procname, "%i", inode_fd.fd);
res = utimensat(lo->proc_self_fd, procname, tv, 0);
}
if (res == -1) {
@@ -1026,7 +1041,8 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
struct fuse_entry_param *e,
struct lo_inode **inodep)
{
- int newfd;
+ g_auto(TempFd) dir_fd = TEMP_FD_INIT;
+ int newfd = -1;
int res;
int saverr;
uint64_t mnt_id;
@@ -1056,7 +1072,13 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
name = ".";
}
- newfd = openat(dir->fd, name, O_PATH | O_NOFOLLOW);
+ res = lo_inode_fd(dir, &dir_fd);
+ if (res < 0) {
+ saverr = -res;
+ goto out;
+ }
+
+ newfd = openat(dir_fd.fd, name, O_PATH | O_NOFOLLOW);
if (newfd == -1) {
goto out_err;
}
@@ -1123,6 +1145,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
out_err:
saverr = errno;
+out:
if (newfd != -1) {
close(newfd);
}
@@ -1228,6 +1251,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
const char *name, mode_t mode, dev_t rdev,
const char *link)
{
+ g_auto(TempFd) dir_fd = TEMP_FD_INIT;
int res;
int saverr;
struct lo_data *lo = lo_data(req);
@@ -1251,12 +1275,18 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
return;
}
+ res = lo_inode_fd(dir, &dir_fd);
+ if (res < 0) {
+ saverr = -res;
+ goto out;
+ }
+
saverr = lo_change_cred(req, &old);
if (saverr) {
goto out;
}
- res = mknod_wrapper(dir->fd, name, link, mode, rdev);
+ res = mknod_wrapper(dir_fd.fd, name, link, mode, rdev);
saverr = errno;
@@ -1304,6 +1334,8 @@ static void lo_symlink(fuse_req_t req, const char *link, fuse_ino_t parent,
static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent,
const char *name)
{
+ g_auto(TempFd) inode_fd = TEMP_FD_INIT;
+ g_auto(TempFd) parent_fd = TEMP_FD_INIT;
int res;
struct lo_data *lo = lo_data(req);
struct lo_inode *parent_inode;
@@ -1329,18 +1361,31 @@ static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent,
goto out_err;
}
+ res = lo_inode_fd(inode, &inode_fd);
+ if (res < 0) {
+ errno = -res;
+ goto out_err;
+ }
+
+ res = lo_inode_fd(parent_inode, &parent_fd);
+ if (res < 0) {
+ errno = -res;
+ goto out_err;
+ }
+
memset(&e, 0, sizeof(struct fuse_entry_param));
e.attr_timeout = lo->timeout;
e.entry_timeout = lo->timeout;
- sprintf(procname, "%i", inode->fd);
- res = linkat(lo->proc_self_fd, procname, parent_inode->fd, name,
+ sprintf(procname, "%i", inode_fd.fd);
+ res = linkat(lo->proc_self_fd, procname, parent_fd.fd, name,
AT_SYMLINK_FOLLOW);
if (res == -1) {
goto out_err;
}
- res = fstatat(inode->fd, "", &e.attr, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
+ res = fstatat(inode_fd.fd, "", &e.attr,
+ AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
if (res == -1) {
goto out_err;
}
@@ -1369,6 +1414,7 @@ out_err:
static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
const char *name)
{
+ g_auto(TempFd) dir_fd = TEMP_FD_INIT;
int res;
uint64_t mnt_id;
struct stat attr;
@@ -1379,7 +1425,12 @@ static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
return NULL;
}
- res = do_statx(lo, dir->fd, name, &attr, AT_SYMLINK_NOFOLLOW, &mnt_id);
+ res = lo_inode_fd(dir, &dir_fd);
+ if (res < 0) {
+ return NULL;
+ }
+
+ res = do_statx(lo, dir_fd.fd, name, &attr, AT_SYMLINK_NOFOLLOW, &mnt_id);
lo_inode_put(lo, &dir);
if (res == -1) {
return NULL;
@@ -1421,6 +1472,8 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name,
fuse_ino_t newparent, const char *newname,
unsigned int flags)
{
+ g_auto(TempFd) parent_fd = TEMP_FD_INIT;
+ g_auto(TempFd) newparent_fd = TEMP_FD_INIT;
int res;
struct lo_inode *parent_inode;
struct lo_inode *newparent_inode;
@@ -1453,12 +1506,24 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name,
goto out;
}
+ res = lo_inode_fd(parent_inode, &parent_fd);
+ if (res < 0) {
+ fuse_reply_err(req, -res);
+ goto out;
+ }
+
+ res = lo_inode_fd(newparent_inode, &newparent_fd);
+ if (res < 0) {
+ fuse_reply_err(req, -res);
+ goto out;
+ }
+
if (flags) {
#ifndef SYS_renameat2
fuse_reply_err(req, EINVAL);
#else
- res = syscall(SYS_renameat2, parent_inode->fd, name,
- newparent_inode->fd, newname, flags);
+ res = syscall(SYS_renameat2, parent_fd.fd, name,
+ newparent_fd.fd, newname, flags);
if (res == -1 && errno == ENOSYS) {
fuse_reply_err(req, EINVAL);
} else {
@@ -1468,7 +1533,7 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name,
goto out;
}
- res = renameat(parent_inode->fd, name, newparent_inode->fd, newname);
+ res = renameat(parent_fd.fd, name, newparent_fd.fd, newname);
fuse_reply_err(req, res == -1 ? errno : 0);
out:
@@ -1953,6 +2018,7 @@ static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
mode_t mode, struct fuse_file_info *fi)
{
+ g_auto(TempFd) parent_fd = TEMP_FD_INIT;
int fd = -1;
struct lo_data *lo = lo_data(req);
struct lo_inode *parent_inode;
@@ -1975,6 +2041,12 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
return;
}
+ err = lo_inode_fd(parent_inode, &parent_fd);
+ if (err < 0) {
+ err = -err;
+ goto out;
+ }
+
err = lo_change_cred(req, &old);
if (err) {
goto out;
@@ -1983,7 +2055,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
update_open_flags(lo->writeback, lo->allow_direct_io, fi);
/* Try to create a new file but don't open existing files */
- fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
+ fd = openat(parent_fd.fd, name, fi->flags | O_CREAT | O_EXCL, mode);
err = fd == -1 ? errno : 0;
lo_restore_cred(&old);
@@ -2788,6 +2860,7 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name,
static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
size_t size)
{
+ g_auto(TempFd) inode_fd = TEMP_FD_INIT;
struct lo_data *lo = lo_data(req);
g_autofree char *value = NULL;
char procname[64];
@@ -2850,7 +2923,12 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
}
ret = fgetxattr(fd, name, value, size);
} else {
- sprintf(procname, "%i", inode->fd);
+ ret = lo_inode_fd(inode, &inode_fd);
+ if (ret < 0) {
+ saverr = -ret;
+ goto out;
+ }
+ sprintf(procname, "%i", inode_fd.fd);
/* fchdir should not fail here */
FCHDIR_NOFAIL(lo->proc_self_fd);
ret = getxattr(procname, name, value, size);
@@ -2887,6 +2965,7 @@ out:
static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
{
+ g_auto(TempFd) inode_fd = TEMP_FD_INIT;
struct lo_data *lo = lo_data(req);
g_autofree char *value = NULL;
char procname[64];
@@ -2924,7 +3003,12 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
}
ret = flistxattr(fd, value, size);
} else {
- sprintf(procname, "%i", inode->fd);
+ ret = lo_inode_fd(inode, &inode_fd);
+ if (ret < 0) {
+ saverr = -ret;
+ goto out;
+ }
+ sprintf(procname, "%i", inode_fd.fd);
/* fchdir should not fail here */
FCHDIR_NOFAIL(lo->proc_self_fd);
ret = listxattr(procname, value, size);
@@ -3013,6 +3097,7 @@ out:
static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
const char *value, size_t size, int flags)
{
+ g_auto(TempFd) inode_fd = TEMP_FD_INIT;
char procname[64];
const char *name;
char *mapped_name;
@@ -3058,7 +3143,12 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
}
ret = fsetxattr(fd, name, value, size, flags);
} else {
- sprintf(procname, "%i", inode->fd);
+ ret = lo_inode_fd(inode, &inode_fd);
+ if (ret < 0) {
+ saverr = -ret;
+ goto out;
+ }
+ sprintf(procname, "%i", inode_fd.fd);
/* fchdir should not fail here */
FCHDIR_NOFAIL(lo->proc_self_fd);
ret = setxattr(procname, name, value, size, flags);
@@ -3079,6 +3169,7 @@ out:
static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name)
{
+ g_auto(TempFd) inode_fd = TEMP_FD_INIT;
char procname[64];
const char *name;
char *mapped_name;
@@ -3124,7 +3215,12 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name)
}
ret = fremovexattr(fd, name);
} else {
- sprintf(procname, "%i", inode->fd);
+ ret = lo_inode_fd(inode, &inode_fd);
+ if (ret < 0) {
+ saverr = -ret;
+ goto out;
+ }
+ sprintf(procname, "%i", inode_fd.fd);
/* fchdir should not fail here */
FCHDIR_NOFAIL(lo->proc_self_fd);
ret = removexattr(procname, name);
--
2.31.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 4/9] virtiofsd: Let lo_fd() return a TempFd
2021-06-04 16:13 [PATCH 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs Max Reitz
` (2 preceding siblings ...)
2021-06-04 16:13 ` [PATCH 3/9] virtiofsd: Add lo_inode_fd() helper Max Reitz
@ 2021-06-04 16:13 ` Max Reitz
2021-06-04 16:13 ` [PATCH 5/9] virtiofsd: Let lo_inode_open() " Max Reitz
` (5 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Max Reitz @ 2021-06-04 16:13 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: Dr . David Alan Gilbert, Stefan Hajnoczi, Max Reitz
Accessing lo_inode.fd must generally happen through lo_inode_fd(), and
lo_fd() is no exception; and then it must pass on the TempFd it has
received from lo_inode_fd().
(Note that all lo_fd() calls now use proper error handling, where all of
them were in-line before; i.e. they were used in place of the fd
argument of some function call. This only worked because the only error
that could occur was that lo_inode() failed to find the inode ID: Then
-1 would be passed as the fd, which would result in an EBADF error,
which is precisely what we would want to return to the guest for an
invalid inode ID.
Now, though, lo_inode_fd() might potentially invoke open_by_handle_at(),
which can return many different errors, and they should be properly
handled and returned to the guest. So we can no longer allow lo_fd() to
be used in-line, and instead need to do proper error handling for it.)
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tools/virtiofsd/passthrough_ll.c | 55 +++++++++++++++++++++++++-------
1 file changed, 44 insertions(+), 11 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 46c9dfe200..8f64bcd6c5 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -644,18 +644,19 @@ static int lo_inode_fd(const struct lo_inode *inode, TempFd *tfd)
* they are done with the fd. This will be done in a later patch to make
* review easier.
*/
-static int lo_fd(fuse_req_t req, fuse_ino_t ino)
+static int lo_fd(fuse_req_t req, fuse_ino_t ino, TempFd *tfd)
{
struct lo_inode *inode = lo_inode(req, ino);
- int fd;
+ int res;
if (!inode) {
- return -1;
+ return -EBADF;
}
- fd = inode->fd;
+ res = lo_inode_fd(inode, tfd);
+
lo_inode_put(lo_data(req), &inode);
- return fd;
+ return res;
}
/*
@@ -766,14 +767,19 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
static void lo_getattr(fuse_req_t req, fuse_ino_t ino,
struct fuse_file_info *fi)
{
+ g_auto(TempFd) ino_fd = TEMP_FD_INIT;
int res;
struct stat buf;
struct lo_data *lo = lo_data(req);
(void)fi;
- res =
- fstatat(lo_fd(req, ino), "", &buf, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
+ res = lo_fd(req, ino, &ino_fd);
+ if (res < 0) {
+ return (void)fuse_reply_err(req, -res);
+ }
+
+ res = fstatat(ino_fd.fd, "", &buf, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
if (res == -1) {
return (void)fuse_reply_err(req, errno);
}
@@ -1441,6 +1447,7 @@ static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
static void lo_rmdir(fuse_req_t req, fuse_ino_t parent, const char *name)
{
+ g_auto(TempFd) parent_fd = TEMP_FD_INIT;
int res;
struct lo_inode *inode;
struct lo_data *lo = lo_data(req);
@@ -1455,13 +1462,19 @@ static void lo_rmdir(fuse_req_t req, fuse_ino_t parent, const char *name)
return;
}
+ res = lo_fd(req, parent, &parent_fd);
+ if (res < 0) {
+ fuse_reply_err(req, -res);
+ return;
+ }
+
inode = lookup_name(req, parent, name);
if (!inode) {
fuse_reply_err(req, EIO);
return;
}
- res = unlinkat(lo_fd(req, parent), name, AT_REMOVEDIR);
+ res = unlinkat(parent_fd.fd, name, AT_REMOVEDIR);
fuse_reply_err(req, res == -1 ? errno : 0);
unref_inode_lolocked(lo, inode, 1);
@@ -1547,6 +1560,7 @@ out:
static void lo_unlink(fuse_req_t req, fuse_ino_t parent, const char *name)
{
+ g_auto(TempFd) parent_fd = TEMP_FD_INIT;
int res;
struct lo_inode *inode;
struct lo_data *lo = lo_data(req);
@@ -1561,13 +1575,19 @@ static void lo_unlink(fuse_req_t req, fuse_ino_t parent, const char *name)
return;
}
+ res = lo_fd(req, parent, &parent_fd);
+ if (res < 0) {
+ fuse_reply_err(req, -res);
+ return;
+ }
+
inode = lookup_name(req, parent, name);
if (!inode) {
fuse_reply_err(req, EIO);
return;
}
- res = unlinkat(lo_fd(req, parent), name, 0);
+ res = unlinkat(parent_fd.fd, name, 0);
fuse_reply_err(req, res == -1 ? errno : 0);
unref_inode_lolocked(lo, inode, 1);
@@ -1647,10 +1667,16 @@ static void lo_forget_multi(fuse_req_t req, size_t count,
static void lo_readlink(fuse_req_t req, fuse_ino_t ino)
{
+ g_auto(TempFd) ino_fd = TEMP_FD_INIT;
char buf[PATH_MAX + 1];
int res;
- res = readlinkat(lo_fd(req, ino), "", buf, sizeof(buf));
+ res = lo_fd(req, ino, &ino_fd);
+ if (res < 0) {
+ return (void)fuse_reply_err(req, -res);
+ }
+
+ res = readlinkat(ino_fd.fd, "", buf, sizeof(buf));
if (res == -1) {
return (void)fuse_reply_err(req, errno);
}
@@ -2447,10 +2473,17 @@ static void lo_write_buf(fuse_req_t req, fuse_ino_t ino,
static void lo_statfs(fuse_req_t req, fuse_ino_t ino)
{
+ g_auto(TempFd) ino_fd = TEMP_FD_INIT;
int res;
struct statvfs stbuf;
- res = fstatvfs(lo_fd(req, ino), &stbuf);
+ res = lo_fd(req, ino, &ino_fd);
+ if (res < 0) {
+ fuse_reply_err(req, -res);
+ return;
+ }
+
+ res = fstatvfs(ino_fd.fd, &stbuf);
if (res == -1) {
fuse_reply_err(req, errno);
} else {
--
2.31.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 5/9] virtiofsd: Let lo_inode_open() return a TempFd
2021-06-04 16:13 [PATCH 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs Max Reitz
` (3 preceding siblings ...)
2021-06-04 16:13 ` [PATCH 4/9] virtiofsd: Let lo_fd() return a TempFd Max Reitz
@ 2021-06-04 16:13 ` Max Reitz
2021-06-04 16:13 ` [PATCH 6/9] virtiofsd: Add lo_inode.fhandle Max Reitz
` (4 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Max Reitz @ 2021-06-04 16:13 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: Dr . David Alan Gilbert, Stefan Hajnoczi, Max Reitz
Strictly speaking, this is not necessary, because lo_inode_open() will
always return a new FD owned by the caller, so TempFd.owned will always
be true.
However, auto-cleanup is nice, and in some cases this plays nicely with
an lo_inode_fd() call in another conditional branch (see lo_setattr()).
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tools/virtiofsd/passthrough_ll.c | 137 +++++++++++++------------------
1 file changed, 59 insertions(+), 78 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 8f64bcd6c5..3014e8baf8 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -285,10 +285,8 @@ static void temp_fd_clear(TempFd *temp_fd)
/**
* Return an owned fd from *temp_fd that will not be closed when
* *temp_fd goes out of scope.
- *
- * (TODO: Remove __attribute__ once this is used.)
*/
-static __attribute__((unused)) int temp_fd_steal(TempFd *temp_fd)
+static int temp_fd_steal(TempFd *temp_fd)
{
if (temp_fd->owned) {
temp_fd->owned = false;
@@ -667,9 +665,12 @@ static int lo_fd(fuse_req_t req, fuse_ino_t ino, TempFd *tfd)
* when a malicious client opens special files such as block device nodes.
* Symlink inodes are also rejected since symlinks must already have been
* traversed on the client side.
+ *
+ * The fd is returned in tfd->fd. The return value is 0 on success and -errno
+ * otherwise.
*/
-static int lo_inode_open(struct lo_data *lo, struct lo_inode *inode,
- int open_flags)
+static int lo_inode_open(const struct lo_data *lo, const struct lo_inode *inode,
+ int open_flags, TempFd *tfd)
{
g_autofree char *fd_str = g_strdup_printf("%d", inode->fd);
int fd;
@@ -688,7 +689,13 @@ static int lo_inode_open(struct lo_data *lo, struct lo_inode *inode,
if (fd < 0) {
return -errno;
}
- return fd;
+
+ *tfd = (TempFd) {
+ .fd = fd,
+ .owned = true,
+ };
+
+ return 0;
}
static void lo_init(void *userdata, struct fuse_conn_info *conn)
@@ -820,7 +827,12 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
return;
}
- res = lo_inode_fd(inode, &inode_fd);
+ if (!fi && (valid & FUSE_SET_ATTR_SIZE)) {
+ /* We need an O_RDWR FD for ftruncate() */
+ res = lo_inode_open(lo, inode, O_RDWR, &inode_fd);
+ } else {
+ res = lo_inode_fd(inode, &inode_fd);
+ }
if (res < 0) {
saverr = -res;
goto out_err;
@@ -868,18 +880,11 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
if (fi) {
truncfd = fd;
} else {
- truncfd = lo_inode_open(lo, inode, O_RDWR);
- if (truncfd < 0) {
- saverr = -truncfd;
- goto out_err;
- }
+ truncfd = inode_fd.fd;
}
saverr = drop_security_capability(lo, truncfd);
if (saverr) {
- if (!fi) {
- close(truncfd);
- }
goto out_err;
}
@@ -887,9 +892,6 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
res = drop_effective_cap("FSETID", &cap_fsetid_dropped);
if (res != 0) {
saverr = res;
- if (!fi) {
- close(truncfd);
- }
goto out_err;
}
}
@@ -902,9 +904,6 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_FSETID\n");
}
}
- if (!fi) {
- close(truncfd);
- }
if (res == -1) {
goto out_err;
}
@@ -1734,11 +1733,12 @@ static struct lo_dirp *lo_dirp(fuse_req_t req, struct fuse_file_info *fi)
static void lo_opendir(fuse_req_t req, fuse_ino_t ino,
struct fuse_file_info *fi)
{
+ g_auto(TempFd) inode_fd = TEMP_FD_INIT;
int error = ENOMEM;
struct lo_data *lo = lo_data(req);
struct lo_inode *inode;
struct lo_dirp *d = NULL;
- int fd;
+ int res;
ssize_t fh;
inode = lo_inode(req, ino);
@@ -1752,13 +1752,13 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino,
goto out_err;
}
- fd = lo_inode_open(lo, inode, O_RDONLY);
- if (fd < 0) {
- error = -fd;
+ res = lo_inode_open(lo, inode, O_RDONLY, &inode_fd);
+ if (res < 0) {
+ error = -res;
goto out_err;
}
- d->dp = fdopendir(fd);
+ d->dp = fdopendir(temp_fd_steal(&inode_fd));
if (d->dp == NULL) {
goto out_errno;
}
@@ -1788,8 +1788,6 @@ out_err:
if (d) {
if (d->dp) {
closedir(d->dp);
- } else if (fd != -1) {
- close(fd);
}
free(d);
}
@@ -1989,6 +1987,7 @@ static void update_open_flags(int writeback, int allow_direct_io,
static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
int existing_fd, struct fuse_file_info *fi)
{
+ g_auto(TempFd) inode_fd = TEMP_FD_INIT;
ssize_t fh;
int fd = existing_fd;
int err;
@@ -2005,16 +2004,18 @@ static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
}
}
- fd = lo_inode_open(lo, inode, fi->flags);
+ err = lo_inode_open(lo, inode, fi->flags, &inode_fd);
if (cap_fsetid_dropped) {
if (gain_effective_cap("FSETID")) {
fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_FSETID\n");
}
}
- if (fd < 0) {
- return -fd;
+ if (err < 0) {
+ return -err;
}
+ fd = temp_fd_steal(&inode_fd);
+
if (fi->flags & (O_TRUNC)) {
int err = drop_security_capability(lo, fd);
if (err) {
@@ -2124,8 +2125,9 @@ static struct lo_inode_plock *lookup_create_plock_ctx(struct lo_data *lo,
uint64_t lock_owner,
pid_t pid, int *err)
{
+ g_auto(TempFd) inode_fd = TEMP_FD_INIT;
struct lo_inode_plock *plock;
- int fd;
+ int res;
plock =
g_hash_table_lookup(inode->posix_locks, GUINT_TO_POINTER(lock_owner));
@@ -2142,15 +2144,15 @@ static struct lo_inode_plock *lookup_create_plock_ctx(struct lo_data *lo,
/* Open another instance of file which can be used for ofd locks. */
/* TODO: What if file is not writable? */
- fd = lo_inode_open(lo, inode, O_RDWR);
- if (fd < 0) {
- *err = -fd;
+ res = lo_inode_open(lo, inode, O_RDWR, &inode_fd);
+ if (res < 0) {
+ *err = -res;
free(plock);
return NULL;
}
plock->lock_owner = lock_owner;
- plock->fd = fd;
+ plock->fd = temp_fd_steal(&inode_fd);
g_hash_table_insert(inode->posix_locks, GUINT_TO_POINTER(plock->lock_owner),
plock);
return plock;
@@ -2366,6 +2368,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
static void lo_fsync(fuse_req_t req, fuse_ino_t ino, int datasync,
struct fuse_file_info *fi)
{
+ g_auto(TempFd) inode_fd = TEMP_FD_INIT;
struct lo_inode *inode = lo_inode(req, ino);
struct lo_data *lo = lo_data(req);
int res;
@@ -2380,11 +2383,12 @@ static void lo_fsync(fuse_req_t req, fuse_ino_t ino, int datasync,
}
if (!fi) {
- fd = lo_inode_open(lo, inode, O_RDWR);
- if (fd < 0) {
- res = -fd;
+ res = lo_inode_open(lo, inode, O_RDWR, &inode_fd);
+ if (res < 0) {
+ res = -res;
goto out;
}
+ fd = inode_fd.fd;
} else {
fd = lo_fi_fd(req, fi);
}
@@ -2394,9 +2398,6 @@ static void lo_fsync(fuse_req_t req, fuse_ino_t ino, int datasync,
} else {
res = fsync(fd) == -1 ? errno : 0;
}
- if (!fi) {
- close(fd);
- }
out:
lo_inode_put(lo, &inode);
fuse_reply_err(req, res);
@@ -2902,7 +2903,6 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
struct lo_inode *inode;
ssize_t ret;
int saverr;
- int fd = -1;
mapped_name = NULL;
name = in_name;
@@ -2949,12 +2949,12 @@ 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 = lo_inode_open(lo, inode, O_RDONLY);
- if (fd < 0) {
- saverr = -fd;
+ ret = lo_inode_open(lo, inode, O_RDONLY, &inode_fd);
+ if (ret < 0) {
+ saverr = -ret;
goto out;
}
- ret = fgetxattr(fd, name, value, size);
+ ret = fgetxattr(inode_fd.fd, name, value, size);
} else {
ret = lo_inode_fd(inode, &inode_fd);
if (ret < 0) {
@@ -2981,10 +2981,6 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
fuse_reply_xattr(req, ret);
}
out_free:
- if (fd >= 0) {
- close(fd);
- }
-
lo_inode_put(lo, &inode);
return;
@@ -3005,7 +3001,6 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
struct lo_inode *inode;
ssize_t ret;
int saverr;
- int fd = -1;
inode = lo_inode(req, ino);
if (!inode) {
@@ -3029,12 +3024,12 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
}
if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) {
- fd = lo_inode_open(lo, inode, O_RDONLY);
- if (fd < 0) {
- saverr = -fd;
+ ret = lo_inode_open(lo, inode, O_RDONLY, &inode_fd);
+ if (ret < 0) {
+ saverr = -ret;
goto out;
}
- ret = flistxattr(fd, value, size);
+ ret = flistxattr(inode_fd.fd, value, size);
} else {
ret = lo_inode_fd(inode, &inode_fd);
if (ret < 0) {
@@ -3113,10 +3108,6 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
fuse_reply_xattr(req, ret);
}
out_free:
- if (fd >= 0) {
- close(fd);
- }
-
lo_inode_put(lo, &inode);
return;
@@ -3138,7 +3129,6 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
struct lo_inode *inode;
ssize_t ret;
int saverr;
- int fd = -1;
mapped_name = NULL;
name = in_name;
@@ -3169,12 +3159,12 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
", name=%s value=%s size=%zd)\n", ino, name, value, size);
if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) {
- fd = lo_inode_open(lo, inode, O_RDONLY);
- if (fd < 0) {
- saverr = -fd;
+ ret = lo_inode_open(lo, inode, O_RDONLY, &inode_fd);
+ if (ret < 0) {
+ saverr = -ret;
goto out;
}
- ret = fsetxattr(fd, name, value, size, flags);
+ ret = fsetxattr(inode_fd.fd, name, value, size, flags);
} else {
ret = lo_inode_fd(inode, &inode_fd);
if (ret < 0) {
@@ -3191,10 +3181,6 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
saverr = ret == -1 ? errno : 0;
out:
- if (fd >= 0) {
- close(fd);
- }
-
lo_inode_put(lo, &inode);
g_free(mapped_name);
fuse_reply_err(req, saverr);
@@ -3210,7 +3196,6 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name)
struct lo_inode *inode;
ssize_t ret;
int saverr;
- int fd = -1;
mapped_name = NULL;
name = in_name;
@@ -3241,12 +3226,12 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name)
name);
if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) {
- fd = lo_inode_open(lo, inode, O_RDONLY);
- if (fd < 0) {
- saverr = -fd;
+ ret = lo_inode_open(lo, inode, O_RDONLY, &inode_fd);
+ if (ret < 0) {
+ saverr = -ret;
goto out;
}
- ret = fremovexattr(fd, name);
+ ret = fremovexattr(inode_fd.fd, name);
} else {
ret = lo_inode_fd(inode, &inode_fd);
if (ret < 0) {
@@ -3263,10 +3248,6 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name)
saverr = ret == -1 ? errno : 0;
out:
- if (fd >= 0) {
- close(fd);
- }
-
lo_inode_put(lo, &inode);
g_free(mapped_name);
fuse_reply_err(req, saverr);
--
2.31.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 6/9] virtiofsd: Add lo_inode.fhandle
2021-06-04 16:13 [PATCH 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs Max Reitz
` (4 preceding siblings ...)
2021-06-04 16:13 ` [PATCH 5/9] virtiofsd: Let lo_inode_open() " Max Reitz
@ 2021-06-04 16:13 ` Max Reitz
2021-06-04 16:13 ` [PATCH 7/9] virtiofsd: Add inodes_by_handle hash table Max Reitz
` (3 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Max Reitz @ 2021-06-04 16:13 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: Dr . David Alan Gilbert, Stefan Hajnoczi, Max Reitz
This new field is an alternative to lo_inode.fd: Either of the two must
be set. In case an O_PATH FD is needed for some lo_inode, it is either
taken from lo_inode.fd, if valid, or a temporary FD is opened with
open_by_handle_at().
Using a file handle instead of an FD has the advantage of keeping the
number of open file descriptors low.
Because open_by_handle_at() requires a mount FD (i.e. a non-O_PATH FD
opened on the filesystem to which the file handle refers), but every
lo_fhandle only has a mount ID (as returned by name_to_handle_at()), we
keep a hash map of such FDs in mount_fds (mapping ID to FD).
get_file_handle(), which is added by a later patch, will ensure that
every mount ID for which we have generated a handle has a corresponding
entry in mount_fds.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tools/virtiofsd/passthrough_ll.c | 116 ++++++++++++++++++++++----
tools/virtiofsd/passthrough_seccomp.c | 1 +
2 files changed, 102 insertions(+), 15 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 3014e8baf8..e665575401 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -88,8 +88,25 @@ struct lo_key {
uint64_t mnt_id;
};
+struct lo_fhandle {
+ union {
+ struct file_handle handle;
+ char padding[sizeof(struct file_handle) + MAX_HANDLE_SZ];
+ };
+ int mount_id;
+};
+
+/* Maps mount IDs to an FD that we can pass to open_by_handle_at() */
+static GHashTable *mount_fds;
+pthread_rwlock_t mount_fds_lock = PTHREAD_RWLOCK_INITIALIZER;
+
struct lo_inode {
+ /*
+ * Either of fd or fhandle must be set (i.e. >= 0 or non-NULL,
+ * respectively).
+ */
int fd;
+ struct lo_fhandle *fhandle;
/*
* Atomic reference count for this object. The nlookup field holds a
@@ -296,6 +313,44 @@ static int temp_fd_steal(TempFd *temp_fd)
}
}
+/**
+ * Open the given file handle with the given flags.
+ *
+ * The mount FD to pass to open_by_handle_at() is taken from the
+ * mount_fds hash map.
+ *
+ * On error, return -errno.
+ */
+static int open_file_handle(const struct lo_fhandle *fh, int flags)
+{
+ gpointer mount_fd_ptr;
+ int mount_fd;
+ bool found;
+ int ret;
+
+ ret = pthread_rwlock_rdlock(&mount_fds_lock);
+ if (ret) {
+ return -ret;
+ }
+
+ /* mount_fd == 0 is valid, so we need lookup_extended */
+ found = g_hash_table_lookup_extended(mount_fds,
+ GINT_TO_POINTER(fh->mount_id),
+ NULL, &mount_fd_ptr);
+ pthread_rwlock_unlock(&mount_fds_lock);
+ if (!found) {
+ return -EINVAL;
+ }
+ mount_fd = GPOINTER_TO_INT(mount_fd_ptr);
+
+ ret = open_by_handle_at(mount_fd, (struct file_handle *)&fh->handle, flags);
+ if (ret < 0) {
+ return -errno;
+ }
+
+ return ret;
+}
+
/*
* Load capng's state from our saved state if the current thread
* hadn't previously been loaded.
@@ -602,7 +657,11 @@ static void lo_inode_put(struct lo_data *lo, struct lo_inode **inodep)
*inodep = NULL;
if (g_atomic_int_dec_and_test(&inode->refcount)) {
- close(inode->fd);
+ if (inode->fd >= 0) {
+ close(inode->fd);
+ } else {
+ g_free(inode->fhandle);
+ }
free(inode);
}
}
@@ -629,10 +688,25 @@ static struct lo_inode *lo_inode(fuse_req_t req, fuse_ino_t ino)
static int lo_inode_fd(const struct lo_inode *inode, TempFd *tfd)
{
- *tfd = (TempFd) {
- .fd = inode->fd,
- .owned = false,
- };
+ if (inode->fd >= 0) {
+ *tfd = (TempFd) {
+ .fd = inode->fd,
+ .owned = false,
+ };
+ } else {
+ int fd;
+
+ assert(inode->fhandle != NULL);
+ fd = open_file_handle(inode->fhandle, O_PATH);
+ if (fd < 0) {
+ return -errno;
+ }
+
+ *tfd = (TempFd) {
+ .fd = fd,
+ .owned = true,
+ };
+ }
return 0;
}
@@ -672,22 +746,32 @@ static int lo_fd(fuse_req_t req, fuse_ino_t ino, TempFd *tfd)
static int lo_inode_open(const struct lo_data *lo, const struct lo_inode *inode,
int open_flags, TempFd *tfd)
{
- g_autofree char *fd_str = g_strdup_printf("%d", inode->fd);
+ g_autofree char *fd_str = NULL;
int fd;
if (!S_ISREG(inode->filetype) && !S_ISDIR(inode->filetype)) {
return -EBADF;
}
- /*
- * The file is a symlink so O_NOFOLLOW must be ignored. We checked earlier
- * that the inode is not a special file but if an external process races
- * with us then symlinks are traversed here. It is not possible to escape
- * the shared directory since it is mounted as "/" though.
- */
- fd = openat(lo->proc_self_fd, fd_str, open_flags & ~O_NOFOLLOW);
- if (fd < 0) {
- return -errno;
+ if (inode->fd >= 0) {
+ /*
+ * The file is a symlink so O_NOFOLLOW must be ignored. We checked
+ * earlier that the inode is not a special file but if an external
+ * process races with us then symlinks are traversed here. It is not
+ * possible to escape the shared directory since it is mounted as "/"
+ * though.
+ */
+ fd_str = g_strdup_printf("%d", inode->fd);
+ fd = openat(lo->proc_self_fd, fd_str, open_flags & ~O_NOFOLLOW);
+ if (fd < 0) {
+ return -errno;
+ }
+ } else {
+ assert(inode->fhandle != NULL);
+ fd = open_file_handle(inode->fhandle, open_flags);
+ if (fd < 0) {
+ return fd;
+ }
}
*tfd = (TempFd) {
@@ -3911,6 +3995,8 @@ int main(int argc, char *argv[])
lo.root.fuse_ino = FUSE_ROOT_ID;
lo.cache = CACHE_AUTO;
+ mount_fds = g_hash_table_new(NULL, NULL);
+
/*
* Set up the ino map like this:
* [0] Reserved (will not be used)
diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c
index 62441cfcdb..e948f25ac1 100644
--- a/tools/virtiofsd/passthrough_seccomp.c
+++ b/tools/virtiofsd/passthrough_seccomp.c
@@ -77,6 +77,7 @@ static const int syscall_allowlist[] = {
SCMP_SYS(statx),
SCMP_SYS(open),
SCMP_SYS(openat),
+ SCMP_SYS(open_by_handle_at),
SCMP_SYS(ppoll),
SCMP_SYS(prctl), /* TODO restrict to just PR_SET_NAME? */
SCMP_SYS(preadv),
--
2.31.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 7/9] virtiofsd: Add inodes_by_handle hash table
2021-06-04 16:13 [PATCH 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs Max Reitz
` (5 preceding siblings ...)
2021-06-04 16:13 ` [PATCH 6/9] virtiofsd: Add lo_inode.fhandle Max Reitz
@ 2021-06-04 16:13 ` Max Reitz
2021-06-04 16:13 ` [PATCH 8/9] virtiofsd: Optionally fill lo_inode.fhandle Max Reitz
` (2 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Max Reitz @ 2021-06-04 16:13 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: Dr . David Alan Gilbert, Stefan Hajnoczi, Max Reitz
Currently, lo_inode.fhandle is always NULL and so always keep an O_PATH
FD in lo_inode.fd. Therefore, when the respective inode is unlinked,
its inode ID will remain in use until we drop our lo_inode (and
lo_inode_put() thus closes the FD). Therefore, lo_find() can safely use
the inode ID as an lo_inode key, because any inode with an inode ID we
find in lo_data.inodes (on the same filesystem) must be the exact same
file.
This will change when we start setting lo_inode.fhandle so we do not
have to keep an O_PATH FD open. Then, unlinking such an inode will
immediately remove it, so its ID can then be reused by newly created
files, even while the lo_inode object is still there[1].
So creating a new file can then reuse the old file's inode ID, and
looking up the new file would lead to us finding the old file's
lo_inode, which is not ideal.
Luckily, just as file handles cause this problem, they also solve it: A
file handle contains a generation ID, which changes when an inode ID is
reused, so the new file can be distinguished from the old one. So all
we need to do is to add a second map besides lo_data.inodes that maps
file handles to lo_inodes, namely lo_data.inodes_by_handle. For
clarity, lo_data.inodes is renamed to lo_data.inodes_by_ids.
Unfortunately, we cannot rely on being able to generate file handles
every time. Therefore, we still enter every lo_inode object into
inodes_by_ids, but having an entry in inodes_by_handle is optional. A
potential inodes_by_handle entry then has precedence, the inodes_by_ids
entry is just a fallback.
Note that we do not generate lo_fhandle objects yet, and so we also do
not enter anything into the inodes_by_handle map yet. Also, all lookups
skip that map. We might manually create file handles with some code
that is immediately removed by the next patch again, but that would
break the assumption in lo_find() that every lo_inode with a non-NULL
.fhandle must have an entry in inodes_by_handle and vice versa. So we
leave actually using the inodes_by_handle map for the next patch.
[1] If some application in the guest still has the file open, there is
going to be a corresponding FD mapping in lo_data.fd_map. In such a
case, the inode will only go away once every application in the guest
has closed it. The problem described only applies to cases where the
guest does not have the file open, and it is just in the dentry cache,
basically.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tools/virtiofsd/passthrough_ll.c | 80 +++++++++++++++++++++++++-------
1 file changed, 64 insertions(+), 16 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index e665575401..793d2c333e 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -179,7 +179,8 @@ struct lo_data {
int announce_submounts;
bool use_statx;
struct lo_inode root;
- GHashTable *inodes; /* protected by lo->mutex */
+ GHashTable *inodes_by_ids; /* protected by lo->mutex */
+ GHashTable *inodes_by_handle; /* protected by lo->mutex */
struct lo_map ino_map; /* protected by lo->mutex */
struct lo_map dirp_map; /* protected by lo->mutex */
struct lo_map fd_map; /* protected by lo->mutex */
@@ -257,8 +258,9 @@ static struct {
/* That we loaded cap-ng in the current thread from the saved */
static __thread bool cap_loaded = 0;
-static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
- uint64_t mnt_id);
+static struct lo_inode *lo_find(struct lo_data *lo,
+ const struct lo_fhandle *fhandle,
+ struct stat *st, uint64_t mnt_id);
static int xattr_map_client(const struct lo_data *lo, const char *client_name,
char **out_name);
@@ -1032,18 +1034,39 @@ out_err:
fuse_reply_err(req, saverr);
}
-static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
- uint64_t mnt_id)
+static struct lo_inode *lo_find(struct lo_data *lo,
+ const struct lo_fhandle *fhandle,
+ struct stat *st, uint64_t mnt_id)
{
- struct lo_inode *p;
- struct lo_key key = {
+ struct lo_inode *p = NULL;
+ struct lo_key ids_key = {
.ino = st->st_ino,
.dev = st->st_dev,
.mnt_id = mnt_id,
};
pthread_mutex_lock(&lo->mutex);
- p = g_hash_table_lookup(lo->inodes, &key);
+ if (fhandle) {
+ p = g_hash_table_lookup(lo->inodes_by_handle, fhandle);
+ }
+ if (!p) {
+ p = g_hash_table_lookup(lo->inodes_by_ids, &ids_key);
+ /*
+ * When we had to fall back to looking up an inode by its IDs,
+ * ensure that we hit an entry that does not have a file
+ * handle. Entries with file handles must also have a handle
+ * alt key, so if we have not found it by that handle alt key,
+ * we must have found an entry with a mismatching handle; i.e.
+ * an entry for a different file, even though it has the same
+ * inode ID.
+ * (This can happen when we look up a new file that has reused
+ * the inode ID of some previously unlinked inode for which we
+ * still have an lo_inode object.)
+ */
+ if (p && fhandle != NULL && p->fhandle != NULL) {
+ p = NULL;
+ }
+ }
if (p) {
assert(p->nlookup > 0);
p->nlookup++;
@@ -1183,7 +1206,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
e->attr_flags |= FUSE_ATTR_SUBMOUNT;
}
- inode = lo_find(lo, &e->attr, mnt_id);
+ inode = lo_find(lo, NULL, &e->attr, mnt_id);
if (inode) {
close(newfd);
} else {
@@ -1213,7 +1236,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
}
pthread_mutex_lock(&lo->mutex);
inode->fuse_ino = lo_add_inode_mapping(req, inode);
- g_hash_table_insert(lo->inodes, &inode->key, inode);
+ g_hash_table_insert(lo->inodes_by_ids, &inode->key, inode);
pthread_mutex_unlock(&lo->mutex);
}
e->ino = inode->fuse_ino;
@@ -1525,7 +1548,7 @@ static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
return NULL;
}
- return lo_find(lo, &attr, mnt_id);
+ return lo_find(lo, NULL, &attr, mnt_id);
}
static void lo_rmdir(fuse_req_t req, fuse_ino_t parent, const char *name)
@@ -1688,7 +1711,7 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n)
inode->nlookup -= n;
if (!inode->nlookup) {
lo_map_remove(&lo->ino_map, inode->fuse_ino);
- g_hash_table_remove(lo->inodes, &inode->key);
+ g_hash_table_remove(lo->inodes_by_ids, &inode->key);
if (lo->posix_lock) {
if (g_hash_table_size(inode->posix_locks)) {
fuse_log(FUSE_LOG_WARNING, "Hash table is not empty\n");
@@ -3388,7 +3411,7 @@ static void lo_destroy(void *userdata)
GHashTableIter iter;
gpointer key, value;
- g_hash_table_iter_init(&iter, lo->inodes);
+ g_hash_table_iter_init(&iter, lo->inodes_by_ids);
if (!g_hash_table_iter_next(&iter, &key, &value)) {
break;
}
@@ -3931,10 +3954,34 @@ static gboolean lo_key_equal(gconstpointer a, gconstpointer b)
return la->ino == lb->ino && la->dev == lb->dev && la->mnt_id == lb->mnt_id;
}
+static guint lo_fhandle_hash(gconstpointer key)
+{
+ const struct lo_fhandle *fh = key;
+ guint hash;
+ size_t i;
+
+ /* Basically g_str_hash() */
+ hash = 5381;
+ for (i = 0; i < sizeof(fh->padding); i++) {
+ hash += hash * 33 + (unsigned char)fh->padding[i];
+ }
+ hash += hash * 33 + fh->mount_id;
+
+ return hash;
+}
+
+static gboolean lo_fhandle_equal(gconstpointer a, gconstpointer b)
+{
+ return !memcmp(a, b, sizeof(struct lo_fhandle));
+}
+
static void fuse_lo_data_cleanup(struct lo_data *lo)
{
- if (lo->inodes) {
- g_hash_table_destroy(lo->inodes);
+ if (lo->inodes_by_ids) {
+ g_hash_table_destroy(lo->inodes_by_ids);
+ }
+ if (lo->inodes_by_ids) {
+ g_hash_table_destroy(lo->inodes_by_handle);
}
if (lo->root.posix_locks) {
@@ -3990,7 +4037,8 @@ int main(int argc, char *argv[])
qemu_init_exec_dir(argv[0]);
pthread_mutex_init(&lo.mutex, NULL);
- lo.inodes = g_hash_table_new(lo_key_hash, lo_key_equal);
+ lo.inodes_by_ids = g_hash_table_new(lo_key_hash, lo_key_equal);
+ lo.inodes_by_handle = g_hash_table_new(lo_fhandle_hash, lo_fhandle_equal);
lo.root.fd = -1;
lo.root.fuse_ino = FUSE_ROOT_ID;
lo.cache = CACHE_AUTO;
--
2.31.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 8/9] virtiofsd: Optionally fill lo_inode.fhandle
2021-06-04 16:13 [PATCH 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs Max Reitz
` (6 preceding siblings ...)
2021-06-04 16:13 ` [PATCH 7/9] virtiofsd: Add inodes_by_handle hash table Max Reitz
@ 2021-06-04 16:13 ` Max Reitz
2021-06-08 10:43 ` Dr. David Alan Gilbert
2021-06-04 16:13 ` [PATCH 9/9] virtiofsd: Add lazy lo_do_find() Max Reitz
2021-06-04 21:17 ` [Virtio-fs] [PATCH 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs Connor Kuehl
9 siblings, 1 reply; 13+ messages in thread
From: Max Reitz @ 2021-06-04 16:13 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: Dr . David Alan Gilbert, Stefan Hajnoczi, Max Reitz
When the inode_file_handles option is set, try to generate a file handle
for new inodes instead of opening an O_PATH FD.
Being able to open these again will require CAP_DAC_READ_SEARCH, so the
description text tells the user they will also need to specify
-o modcaps=+dac_read_search.
Generating a file handle returns the mount ID it is valid for. Opening
it will require an FD instead. We have mount_fds to map an ID to an FD.
get_file_handle() fills the hash map by opening the file we have
generated a handle for. To verify that the resulting FD indeed
represents the handle's mount ID, we use statx(). Therefore, using file
handles requires statx() support.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tools/virtiofsd/helper.c | 3 +
tools/virtiofsd/passthrough_ll.c | 170 ++++++++++++++++++++++++--
tools/virtiofsd/passthrough_seccomp.c | 1 +
3 files changed, 165 insertions(+), 9 deletions(-)
diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
index 5e98ed702b..954f8639e6 100644
--- a/tools/virtiofsd/helper.c
+++ b/tools/virtiofsd/helper.c
@@ -186,6 +186,9 @@ void fuse_cmdline_help(void)
" to virtiofsd from guest applications.\n"
" default: no_allow_direct_io\n"
" -o announce_submounts Announce sub-mount points to the guest\n"
+ " -o inode_file_handles Use file handles to reference inodes\n"
+ " instead of O_PATH file descriptors\n"
+ " (requires -o modcaps=+dac_read_search)\n"
);
}
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 793d2c333e..d01f9d3a59 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -190,6 +190,7 @@ struct lo_data {
/* An O_PATH file descriptor to /proc/self/fd/ */
int proc_self_fd;
int user_killpriv_v2, killpriv_v2;
+ int inode_file_handles;
};
/**
@@ -244,6 +245,10 @@ static const struct fuse_opt lo_opts[] = {
{ "announce_submounts", offsetof(struct lo_data, announce_submounts), 1 },
{ "killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 1 },
{ "no_killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 0 },
+ { "inode_file_handles", offsetof(struct lo_data, inode_file_handles), 1 },
+ { "no_inode_file_handles",
+ offsetof(struct lo_data, inode_file_handles),
+ 0 },
FUSE_OPT_END
};
static bool use_syslog = false;
@@ -315,6 +320,108 @@ static int temp_fd_steal(TempFd *temp_fd)
}
}
+/**
+ * Generate a file handle for the given dirfd/name combination.
+ *
+ * If mount_fds does not yet contain an entry for the handle's mount
+ * ID, (re)open dirfd/name in O_RDONLY mode and add it to mount_fds
+ * as the FD for that mount ID. (That is the file that we have
+ * generated a handle for, so it should be representative for the
+ * mount ID. However, to be sure (and to rule out races), we use
+ * statx() to verify that our assumption is correct.)
+ */
+static struct lo_fhandle *get_file_handle(struct lo_data *lo,
+ int dirfd, const char *name)
+{
+ /* We need statx() to verify the mount ID */
+#if defined(CONFIG_STATX) && defined(STATX_MNT_ID)
+ struct lo_fhandle *fh;
+ int ret;
+
+ if (!lo->use_statx || !lo->inode_file_handles) {
+ return NULL;
+ }
+
+ fh = g_new0(struct lo_fhandle, 1);
+
+ fh->handle.handle_bytes = sizeof(fh->padding) - sizeof(fh->handle);
+ ret = name_to_handle_at(dirfd, name, &fh->handle, &fh->mount_id,
+ AT_EMPTY_PATH);
+ if (ret < 0) {
+ goto fail;
+ }
+
+ if (pthread_rwlock_rdlock(&mount_fds_lock)) {
+ goto fail;
+ }
+ if (!g_hash_table_contains(mount_fds, GINT_TO_POINTER(fh->mount_id))) {
+ struct statx stx;
+ int fd;
+
+ pthread_rwlock_unlock(&mount_fds_lock);
+
+ if (name[0]) {
+ fd = openat(dirfd, name, O_RDONLY);
+ } else {
+ char procname[64];
+ snprintf(procname, sizeof(procname), "%i", dirfd);
+ fd = openat(lo->proc_self_fd, procname, O_RDONLY);
+ }
+ if (fd < 0) {
+ goto fail;
+ }
+
+ ret = statx(fd, "", AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW,
+ STATX_MNT_ID, &stx);
+ if (ret < 0) {
+ if (errno == ENOSYS) {
+ lo->use_statx = false;
+ fuse_log(FUSE_LOG_WARNING,
+ "statx() does not work: Will not be able to use file "
+ "handles for inodes\n");
+ }
+ goto fail;
+ }
+ if (!(stx.stx_mask & STATX_MNT_ID) || stx.stx_mnt_id != fh->mount_id) {
+ /*
+ * One reason for stx_mnt_id != mount_id could be that dirfd/name
+ * is a directory, and some other filesystem was mounted there
+ * between us generating the file handle and then opening the FD.
+ * (Other kinds of races might be possible, too.)
+ * Failing this function is not fatal, though, because our caller
+ * (lo_do_lookup()) will just fall back to opening an O_PATH FD to
+ * store in lo_inode.fd instead of storing a file handle in
+ * lo_inode.fhandle. So we do not need to try too hard to get an
+ * FD for fh->mount_id so this function could succeed.
+ */
+ goto fail;
+ }
+
+ if (pthread_rwlock_wrlock(&mount_fds_lock)) {
+ goto fail;
+ }
+
+ /* Check again, might have changed */
+ if (g_hash_table_contains(mount_fds, GINT_TO_POINTER(fh->mount_id))) {
+ close(fd);
+ } else {
+ g_hash_table_insert(mount_fds,
+ GINT_TO_POINTER(fh->mount_id),
+ GINT_TO_POINTER(fd));
+ }
+ }
+ pthread_rwlock_unlock(&mount_fds_lock);
+
+ return fh;
+
+fail:
+ free(fh);
+ return NULL;
+#else /* defined(CONFIG_STATX) && defined(STATX_MNT_ID) */
+ return NULL;
+#endif
+}
+
/**
* Open the given file handle with the given flags.
*
@@ -1132,6 +1239,11 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
return -1;
}
lo->use_statx = false;
+ if (lo->inode_file_handles) {
+ fuse_log(FUSE_LOG_WARNING,
+ "statx() does not work: Will not be able to use file "
+ "handles for inodes\n");
+ }
/* fallback */
}
#endif
@@ -1161,6 +1273,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
struct lo_data *lo = lo_data(req);
struct lo_inode *inode = NULL;
struct lo_inode *dir = lo_inode(req, parent);
+ struct lo_fhandle *fh;
if (inodep) {
*inodep = NULL; /* in case there is an error */
@@ -1190,13 +1303,19 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
goto out;
}
- newfd = openat(dir_fd.fd, name, O_PATH | O_NOFOLLOW);
- if (newfd == -1) {
- goto out_err;
- }
+ fh = get_file_handle(lo, dir_fd.fd, name);
+ if (fh) {
+ res = do_statx(lo, dir_fd.fd, name, &e->attr,
+ AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW, &mnt_id);
+ } else {
+ newfd = openat(dir_fd.fd, name, O_PATH | O_NOFOLLOW);
+ if (newfd == -1) {
+ goto out_err;
+ }
- res = do_statx(lo, newfd, "", &e->attr, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW,
- &mnt_id);
+ res = do_statx(lo, newfd, "", &e->attr,
+ AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW, &mnt_id);
+ }
if (res == -1) {
goto out_err;
}
@@ -1206,9 +1325,19 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
e->attr_flags |= FUSE_ATTR_SUBMOUNT;
}
- inode = lo_find(lo, NULL, &e->attr, mnt_id);
+ /*
+ * Note that fh is always NULL if lo->inode_file_handles is false,
+ * and so we will never do a lookup by file handle here, and
+ * lo->inodes_by_handle will always remain empty. We only need
+ * this map when we do not have an O_PATH fd open for every
+ * lo_inode, though, so if inode_file_handles is false, we do not
+ * need that map anyway.
+ */
+ inode = lo_find(lo, fh, &e->attr, mnt_id);
if (inode) {
- close(newfd);
+ if (newfd != -1) {
+ close(newfd);
+ }
} else {
inode = calloc(1, sizeof(struct lo_inode));
if (!inode) {
@@ -1226,6 +1355,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
inode->nlookup = 1;
inode->fd = newfd;
+ inode->fhandle = fh;
inode->key.ino = e->attr.st_ino;
inode->key.dev = e->attr.st_dev;
inode->key.mnt_id = mnt_id;
@@ -1237,6 +1367,9 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
pthread_mutex_lock(&lo->mutex);
inode->fuse_ino = lo_add_inode_mapping(req, inode);
g_hash_table_insert(lo->inodes_by_ids, &inode->key, inode);
+ if (inode->fhandle) {
+ g_hash_table_insert(lo->inodes_by_handle, inode->fhandle, inode);
+ }
pthread_mutex_unlock(&lo->mutex);
}
e->ino = inode->fuse_ino;
@@ -1530,8 +1663,10 @@ static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
int res;
uint64_t mnt_id;
struct stat attr;
+ struct lo_fhandle *fh;
struct lo_data *lo = lo_data(req);
struct lo_inode *dir = lo_inode(req, parent);
+ struct lo_inode *inode;
if (!dir) {
return NULL;
@@ -1542,13 +1677,19 @@ static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
return NULL;
}
+ fh = get_file_handle(lo, dir_fd.fd, name);
+ /* Ignore errors, this is just an optional key for the lookup */
+
res = do_statx(lo, dir_fd.fd, name, &attr, AT_SYMLINK_NOFOLLOW, &mnt_id);
lo_inode_put(lo, &dir);
if (res == -1) {
return NULL;
}
- return lo_find(lo, NULL, &attr, mnt_id);
+ inode = lo_find(lo, fh, &attr, mnt_id);
+ g_free(fh);
+
+ return inode;
}
static void lo_rmdir(fuse_req_t req, fuse_ino_t parent, const char *name)
@@ -1712,6 +1853,9 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n)
if (!inode->nlookup) {
lo_map_remove(&lo->ino_map, inode->fuse_ino);
g_hash_table_remove(lo->inodes_by_ids, &inode->key);
+ if (inode->fhandle) {
+ g_hash_table_remove(lo->inodes_by_handle, inode->fhandle);
+ }
if (lo->posix_lock) {
if (g_hash_table_size(inode->posix_locks)) {
fuse_log(FUSE_LOG_WARNING, "Hash table is not empty\n");
@@ -4156,6 +4300,14 @@ int main(int argc, char *argv[])
lo.use_statx = true;
+#if !defined(CONFIG_STATX) || !defined(STATX_MNT_ID)
+ if (lo.inode_file_handles) {
+ fuse_log(FUSE_LOG_WARNING,
+ "No statx() or mount ID support: Will not be able to use file "
+ "handles for inodes\n");
+ }
+#endif
+
se = fuse_session_new(&args, &lo_oper, sizeof(lo_oper), &lo);
if (se == NULL) {
goto err_out1;
diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c
index e948f25ac1..ed23e67ba8 100644
--- a/tools/virtiofsd/passthrough_seccomp.c
+++ b/tools/virtiofsd/passthrough_seccomp.c
@@ -73,6 +73,7 @@ static const int syscall_allowlist[] = {
SCMP_SYS(mprotect),
SCMP_SYS(mremap),
SCMP_SYS(munmap),
+ SCMP_SYS(name_to_handle_at),
SCMP_SYS(newfstatat),
SCMP_SYS(statx),
SCMP_SYS(open),
--
2.31.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 8/9] virtiofsd: Optionally fill lo_inode.fhandle
2021-06-04 16:13 ` [PATCH 8/9] virtiofsd: Optionally fill lo_inode.fhandle Max Reitz
@ 2021-06-08 10:43 ` Dr. David Alan Gilbert
2021-06-08 11:05 ` Max Reitz
0 siblings, 1 reply; 13+ messages in thread
From: Dr. David Alan Gilbert @ 2021-06-08 10:43 UTC (permalink / raw)
To: Max Reitz; +Cc: virtio-fs, qemu-devel, Stefan Hajnoczi
* Max Reitz (mreitz@redhat.com) wrote:
> When the inode_file_handles option is set, try to generate a file handle
> for new inodes instead of opening an O_PATH FD.
>
> Being able to open these again will require CAP_DAC_READ_SEARCH, so the
> description text tells the user they will also need to specify
> -o modcaps=+dac_read_search.
>
> Generating a file handle returns the mount ID it is valid for. Opening
> it will require an FD instead. We have mount_fds to map an ID to an FD.
> get_file_handle() fills the hash map by opening the file we have
> generated a handle for. To verify that the resulting FD indeed
> represents the handle's mount ID, we use statx(). Therefore, using file
> handles requires statx() support.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tools/virtiofsd/helper.c | 3 +
> tools/virtiofsd/passthrough_ll.c | 170 ++++++++++++++++++++++++--
> tools/virtiofsd/passthrough_seccomp.c | 1 +
> 3 files changed, 165 insertions(+), 9 deletions(-)
>
> diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
> index 5e98ed702b..954f8639e6 100644
> --- a/tools/virtiofsd/helper.c
> +++ b/tools/virtiofsd/helper.c
> @@ -186,6 +186,9 @@ void fuse_cmdline_help(void)
> " to virtiofsd from guest applications.\n"
> " default: no_allow_direct_io\n"
> " -o announce_submounts Announce sub-mount points to the guest\n"
> + " -o inode_file_handles Use file handles to reference inodes\n"
> + " instead of O_PATH file descriptors\n"
> + " (requires -o modcaps=+dac_read_search)\n"
> );
> }
>
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 793d2c333e..d01f9d3a59 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -190,6 +190,7 @@ struct lo_data {
> /* An O_PATH file descriptor to /proc/self/fd/ */
> int proc_self_fd;
> int user_killpriv_v2, killpriv_v2;
> + int inode_file_handles;
> };
>
> /**
> @@ -244,6 +245,10 @@ static const struct fuse_opt lo_opts[] = {
> { "announce_submounts", offsetof(struct lo_data, announce_submounts), 1 },
> { "killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 1 },
> { "no_killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 0 },
> + { "inode_file_handles", offsetof(struct lo_data, inode_file_handles), 1 },
> + { "no_inode_file_handles",
> + offsetof(struct lo_data, inode_file_handles),
> + 0 },
> FUSE_OPT_END
> };
> static bool use_syslog = false;
> @@ -315,6 +320,108 @@ static int temp_fd_steal(TempFd *temp_fd)
> }
> }
>
> +/**
> + * Generate a file handle for the given dirfd/name combination.
> + *
> + * If mount_fds does not yet contain an entry for the handle's mount
> + * ID, (re)open dirfd/name in O_RDONLY mode and add it to mount_fds
> + * as the FD for that mount ID. (That is the file that we have
> + * generated a handle for, so it should be representative for the
> + * mount ID. However, to be sure (and to rule out races), we use
> + * statx() to verify that our assumption is correct.)
> + */
> +static struct lo_fhandle *get_file_handle(struct lo_data *lo,
> + int dirfd, const char *name)
> +{
> + /* We need statx() to verify the mount ID */
> +#if defined(CONFIG_STATX) && defined(STATX_MNT_ID)
> + struct lo_fhandle *fh;
> + int ret;
> +
> + if (!lo->use_statx || !lo->inode_file_handles) {
> + return NULL;
> + }
> +
> + fh = g_new0(struct lo_fhandle, 1);
> +
> + fh->handle.handle_bytes = sizeof(fh->padding) - sizeof(fh->handle);
> + ret = name_to_handle_at(dirfd, name, &fh->handle, &fh->mount_id,
> + AT_EMPTY_PATH);
> + if (ret < 0) {
> + goto fail;
> + }
> +
> + if (pthread_rwlock_rdlock(&mount_fds_lock)) {
> + goto fail;
> + }
> + if (!g_hash_table_contains(mount_fds, GINT_TO_POINTER(fh->mount_id))) {
> + struct statx stx;
> + int fd;
> +
> + pthread_rwlock_unlock(&mount_fds_lock);
> +
> + if (name[0]) {
> + fd = openat(dirfd, name, O_RDONLY);
But can't that be a device file or other special file that you must not
open?
Dave
> + } else {
> + char procname[64];
> + snprintf(procname, sizeof(procname), "%i", dirfd);
> + fd = openat(lo->proc_self_fd, procname, O_RDONLY);
> + }
> + if (fd < 0) {
> + goto fail;
> + }
> +
> + ret = statx(fd, "", AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW,
> + STATX_MNT_ID, &stx);
> + if (ret < 0) {
> + if (errno == ENOSYS) {
> + lo->use_statx = false;
> + fuse_log(FUSE_LOG_WARNING,
> + "statx() does not work: Will not be able to use file "
> + "handles for inodes\n");
> + }
> + goto fail;
> + }
> + if (!(stx.stx_mask & STATX_MNT_ID) || stx.stx_mnt_id != fh->mount_id) {
> + /*
> + * One reason for stx_mnt_id != mount_id could be that dirfd/name
> + * is a directory, and some other filesystem was mounted there
> + * between us generating the file handle and then opening the FD.
> + * (Other kinds of races might be possible, too.)
> + * Failing this function is not fatal, though, because our caller
> + * (lo_do_lookup()) will just fall back to opening an O_PATH FD to
> + * store in lo_inode.fd instead of storing a file handle in
> + * lo_inode.fhandle. So we do not need to try too hard to get an
> + * FD for fh->mount_id so this function could succeed.
> + */
> + goto fail;
> + }
> +
> + if (pthread_rwlock_wrlock(&mount_fds_lock)) {
> + goto fail;
> + }
> +
> + /* Check again, might have changed */
> + if (g_hash_table_contains(mount_fds, GINT_TO_POINTER(fh->mount_id))) {
> + close(fd);
> + } else {
> + g_hash_table_insert(mount_fds,
> + GINT_TO_POINTER(fh->mount_id),
> + GINT_TO_POINTER(fd));
> + }
> + }
> + pthread_rwlock_unlock(&mount_fds_lock);
> +
> + return fh;
> +
> +fail:
> + free(fh);
> + return NULL;
> +#else /* defined(CONFIG_STATX) && defined(STATX_MNT_ID) */
> + return NULL;
> +#endif
> +}
> +
> /**
> * Open the given file handle with the given flags.
> *
> @@ -1132,6 +1239,11 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
> return -1;
> }
> lo->use_statx = false;
> + if (lo->inode_file_handles) {
> + fuse_log(FUSE_LOG_WARNING,
> + "statx() does not work: Will not be able to use file "
> + "handles for inodes\n");
> + }
> /* fallback */
> }
> #endif
> @@ -1161,6 +1273,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> struct lo_data *lo = lo_data(req);
> struct lo_inode *inode = NULL;
> struct lo_inode *dir = lo_inode(req, parent);
> + struct lo_fhandle *fh;
>
> if (inodep) {
> *inodep = NULL; /* in case there is an error */
> @@ -1190,13 +1303,19 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> goto out;
> }
>
> - newfd = openat(dir_fd.fd, name, O_PATH | O_NOFOLLOW);
> - if (newfd == -1) {
> - goto out_err;
> - }
> + fh = get_file_handle(lo, dir_fd.fd, name);
> + if (fh) {
> + res = do_statx(lo, dir_fd.fd, name, &e->attr,
> + AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW, &mnt_id);
> + } else {
> + newfd = openat(dir_fd.fd, name, O_PATH | O_NOFOLLOW);
> + if (newfd == -1) {
> + goto out_err;
> + }
>
> - res = do_statx(lo, newfd, "", &e->attr, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW,
> - &mnt_id);
> + res = do_statx(lo, newfd, "", &e->attr,
> + AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW, &mnt_id);
> + }
> if (res == -1) {
> goto out_err;
> }
> @@ -1206,9 +1325,19 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> e->attr_flags |= FUSE_ATTR_SUBMOUNT;
> }
>
> - inode = lo_find(lo, NULL, &e->attr, mnt_id);
> + /*
> + * Note that fh is always NULL if lo->inode_file_handles is false,
> + * and so we will never do a lookup by file handle here, and
> + * lo->inodes_by_handle will always remain empty. We only need
> + * this map when we do not have an O_PATH fd open for every
> + * lo_inode, though, so if inode_file_handles is false, we do not
> + * need that map anyway.
> + */
> + inode = lo_find(lo, fh, &e->attr, mnt_id);
> if (inode) {
> - close(newfd);
> + if (newfd != -1) {
> + close(newfd);
> + }
> } else {
> inode = calloc(1, sizeof(struct lo_inode));
> if (!inode) {
> @@ -1226,6 +1355,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
>
> inode->nlookup = 1;
> inode->fd = newfd;
> + inode->fhandle = fh;
> inode->key.ino = e->attr.st_ino;
> inode->key.dev = e->attr.st_dev;
> inode->key.mnt_id = mnt_id;
> @@ -1237,6 +1367,9 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> pthread_mutex_lock(&lo->mutex);
> inode->fuse_ino = lo_add_inode_mapping(req, inode);
> g_hash_table_insert(lo->inodes_by_ids, &inode->key, inode);
> + if (inode->fhandle) {
> + g_hash_table_insert(lo->inodes_by_handle, inode->fhandle, inode);
> + }
> pthread_mutex_unlock(&lo->mutex);
> }
> e->ino = inode->fuse_ino;
> @@ -1530,8 +1663,10 @@ static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
> int res;
> uint64_t mnt_id;
> struct stat attr;
> + struct lo_fhandle *fh;
> struct lo_data *lo = lo_data(req);
> struct lo_inode *dir = lo_inode(req, parent);
> + struct lo_inode *inode;
>
> if (!dir) {
> return NULL;
> @@ -1542,13 +1677,19 @@ static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
> return NULL;
> }
>
> + fh = get_file_handle(lo, dir_fd.fd, name);
> + /* Ignore errors, this is just an optional key for the lookup */
> +
> res = do_statx(lo, dir_fd.fd, name, &attr, AT_SYMLINK_NOFOLLOW, &mnt_id);
> lo_inode_put(lo, &dir);
> if (res == -1) {
> return NULL;
> }
>
> - return lo_find(lo, NULL, &attr, mnt_id);
> + inode = lo_find(lo, fh, &attr, mnt_id);
> + g_free(fh);
> +
> + return inode;
> }
>
> static void lo_rmdir(fuse_req_t req, fuse_ino_t parent, const char *name)
> @@ -1712,6 +1853,9 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n)
> if (!inode->nlookup) {
> lo_map_remove(&lo->ino_map, inode->fuse_ino);
> g_hash_table_remove(lo->inodes_by_ids, &inode->key);
> + if (inode->fhandle) {
> + g_hash_table_remove(lo->inodes_by_handle, inode->fhandle);
> + }
> if (lo->posix_lock) {
> if (g_hash_table_size(inode->posix_locks)) {
> fuse_log(FUSE_LOG_WARNING, "Hash table is not empty\n");
> @@ -4156,6 +4300,14 @@ int main(int argc, char *argv[])
>
> lo.use_statx = true;
>
> +#if !defined(CONFIG_STATX) || !defined(STATX_MNT_ID)
> + if (lo.inode_file_handles) {
> + fuse_log(FUSE_LOG_WARNING,
> + "No statx() or mount ID support: Will not be able to use file "
> + "handles for inodes\n");
> + }
> +#endif
> +
> se = fuse_session_new(&args, &lo_oper, sizeof(lo_oper), &lo);
> if (se == NULL) {
> goto err_out1;
> diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c
> index e948f25ac1..ed23e67ba8 100644
> --- a/tools/virtiofsd/passthrough_seccomp.c
> +++ b/tools/virtiofsd/passthrough_seccomp.c
> @@ -73,6 +73,7 @@ static const int syscall_allowlist[] = {
> SCMP_SYS(mprotect),
> SCMP_SYS(mremap),
> SCMP_SYS(munmap),
> + SCMP_SYS(name_to_handle_at),
> SCMP_SYS(newfstatat),
> SCMP_SYS(statx),
> SCMP_SYS(open),
> --
> 2.31.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 8/9] virtiofsd: Optionally fill lo_inode.fhandle
2021-06-08 10:43 ` Dr. David Alan Gilbert
@ 2021-06-08 11:05 ` Max Reitz
0 siblings, 0 replies; 13+ messages in thread
From: Max Reitz @ 2021-06-08 11:05 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: virtio-fs, qemu-devel, Stefan Hajnoczi
On 08.06.21 12:43, Dr. David Alan Gilbert wrote:
> * Max Reitz (mreitz@redhat.com) wrote:
>> When the inode_file_handles option is set, try to generate a file handle
>> for new inodes instead of opening an O_PATH FD.
>>
>> Being able to open these again will require CAP_DAC_READ_SEARCH, so the
>> description text tells the user they will also need to specify
>> -o modcaps=+dac_read_search.
>>
>> Generating a file handle returns the mount ID it is valid for. Opening
>> it will require an FD instead. We have mount_fds to map an ID to an FD.
>> get_file_handle() fills the hash map by opening the file we have
>> generated a handle for. To verify that the resulting FD indeed
>> represents the handle's mount ID, we use statx(). Therefore, using file
>> handles requires statx() support.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> tools/virtiofsd/helper.c | 3 +
>> tools/virtiofsd/passthrough_ll.c | 170 ++++++++++++++++++++++++--
>> tools/virtiofsd/passthrough_seccomp.c | 1 +
>> 3 files changed, 165 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
>> index 5e98ed702b..954f8639e6 100644
>> --- a/tools/virtiofsd/helper.c
>> +++ b/tools/virtiofsd/helper.c
>> @@ -186,6 +186,9 @@ void fuse_cmdline_help(void)
>> " to virtiofsd from guest applications.\n"
>> " default: no_allow_direct_io\n"
>> " -o announce_submounts Announce sub-mount points to the guest\n"
>> + " -o inode_file_handles Use file handles to reference inodes\n"
>> + " instead of O_PATH file descriptors\n"
>> + " (requires -o modcaps=+dac_read_search)\n"
>> );
>> }
>>
>> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
>> index 793d2c333e..d01f9d3a59 100644
>> --- a/tools/virtiofsd/passthrough_ll.c
>> +++ b/tools/virtiofsd/passthrough_ll.c
>> @@ -190,6 +190,7 @@ struct lo_data {
>> /* An O_PATH file descriptor to /proc/self/fd/ */
>> int proc_self_fd;
>> int user_killpriv_v2, killpriv_v2;
>> + int inode_file_handles;
>> };
>>
>> /**
>> @@ -244,6 +245,10 @@ static const struct fuse_opt lo_opts[] = {
>> { "announce_submounts", offsetof(struct lo_data, announce_submounts), 1 },
>> { "killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 1 },
>> { "no_killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 0 },
>> + { "inode_file_handles", offsetof(struct lo_data, inode_file_handles), 1 },
>> + { "no_inode_file_handles",
>> + offsetof(struct lo_data, inode_file_handles),
>> + 0 },
>> FUSE_OPT_END
>> };
>> static bool use_syslog = false;
>> @@ -315,6 +320,108 @@ static int temp_fd_steal(TempFd *temp_fd)
>> }
>> }
>>
>> +/**
>> + * Generate a file handle for the given dirfd/name combination.
>> + *
>> + * If mount_fds does not yet contain an entry for the handle's mount
>> + * ID, (re)open dirfd/name in O_RDONLY mode and add it to mount_fds
>> + * as the FD for that mount ID. (That is the file that we have
>> + * generated a handle for, so it should be representative for the
>> + * mount ID. However, to be sure (and to rule out races), we use
>> + * statx() to verify that our assumption is correct.)
>> + */
>> +static struct lo_fhandle *get_file_handle(struct lo_data *lo,
>> + int dirfd, const char *name)
>> +{
>> + /* We need statx() to verify the mount ID */
>> +#if defined(CONFIG_STATX) && defined(STATX_MNT_ID)
>> + struct lo_fhandle *fh;
>> + int ret;
>> +
>> + if (!lo->use_statx || !lo->inode_file_handles) {
>> + return NULL;
>> + }
>> +
>> + fh = g_new0(struct lo_fhandle, 1);
>> +
>> + fh->handle.handle_bytes = sizeof(fh->padding) - sizeof(fh->handle);
>> + ret = name_to_handle_at(dirfd, name, &fh->handle, &fh->mount_id,
>> + AT_EMPTY_PATH);
>> + if (ret < 0) {
>> + goto fail;
>> + }
>> +
>> + if (pthread_rwlock_rdlock(&mount_fds_lock)) {
>> + goto fail;
>> + }
>> + if (!g_hash_table_contains(mount_fds, GINT_TO_POINTER(fh->mount_id))) {
>> + struct statx stx;
>> + int fd;
>> +
>> + pthread_rwlock_unlock(&mount_fds_lock);
>> +
>> + if (name[0]) {
>> + fd = openat(dirfd, name, O_RDONLY);
> But can't that be a device file or other special file that you must not
> open?
Yes. So I think the right thing to do here is to use O_PATH (which
should be safe), then statx() to find out what we need to know (now not
only the mount ID, but the file type, too), and if it’s safe, we can
then openat(proc_self_fd, str(fd), O_RDONLY) (and close the O_PATH fd).
If it isn’t safe, we just return an error, and the inode gets an O_PATH fd.
Max
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 9/9] virtiofsd: Add lazy lo_do_find()
2021-06-04 16:13 [PATCH 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs Max Reitz
` (7 preceding siblings ...)
2021-06-04 16:13 ` [PATCH 8/9] virtiofsd: Optionally fill lo_inode.fhandle Max Reitz
@ 2021-06-04 16:13 ` Max Reitz
2021-06-04 21:17 ` [Virtio-fs] [PATCH 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs Connor Kuehl
9 siblings, 0 replies; 13+ messages in thread
From: Max Reitz @ 2021-06-04 16:13 UTC (permalink / raw)
To: qemu-devel, virtio-fs; +Cc: Dr . David Alan Gilbert, Stefan Hajnoczi, Max Reitz
lo_find() right now takes two lookup keys for two maps, namely the file
handle for inodes_by_handle and the statx information for inodes_by_ids.
However, we only need the statx information if looking up the inode by
the file handle failed.
There are two callers of lo_find(): The first one, lo_do_lookup(), has
both keys anyway, so passing them does not incur any additional cost.
The second one, lookup_name(), though, needs to explicitly invoke
name_to_handle_at() (through get_file_handle()) and statx() (through
do_statx()). We need to try to get a file handle as the primary key, so
we cannot get rid of get_file_handle(), but we only need the statx
information if looking up an inode by handle failed; so we can defer
that until the lookup has indeed failed.
To this end, replace lo_find()'s st/mnt_id parameters by a get_ids()
closure that is invoked to fill the lo_key struct if necessary.
Also, lo_find() is renamed to lo_do_find(), so we can add a new
lo_find() wrapper whose closure just initializes the lo_key from the
st/mnt_id parameters, just like the old lo_find() did.
lookup_name() directly calls lo_do_find() now and passes its own
closure, which performs the do_statx() call.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tools/virtiofsd/passthrough_ll.c | 93 ++++++++++++++++++++++++++------
1 file changed, 76 insertions(+), 17 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index d01f9d3a59..ef10a3ace6 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -1141,22 +1141,23 @@ out_err:
fuse_reply_err(req, saverr);
}
-static struct lo_inode *lo_find(struct lo_data *lo,
- const struct lo_fhandle *fhandle,
- struct stat *st, uint64_t mnt_id)
+/*
+ * get_ids() will be called to get the key for lo->inodes_by_ids if
+ * the lookup by file handle has failed.
+ */
+static struct lo_inode *lo_do_find(struct lo_data *lo,
+ const struct lo_fhandle *fhandle,
+ int (*get_ids)(struct lo_key *, const void *),
+ const void *get_ids_opaque)
{
struct lo_inode *p = NULL;
- struct lo_key ids_key = {
- .ino = st->st_ino,
- .dev = st->st_dev,
- .mnt_id = mnt_id,
- };
+ struct lo_key ids_key;
pthread_mutex_lock(&lo->mutex);
if (fhandle) {
p = g_hash_table_lookup(lo->inodes_by_handle, fhandle);
}
- if (!p) {
+ if (!p && get_ids(&ids_key, get_ids_opaque) == 0) {
p = g_hash_table_lookup(lo->inodes_by_ids, &ids_key);
/*
* When we had to fall back to looking up an inode by its IDs,
@@ -1184,6 +1185,36 @@ static struct lo_inode *lo_find(struct lo_data *lo,
return p;
}
+struct lo_find_get_ids_key_opaque {
+ const struct stat *st;
+ uint64_t mnt_id;
+};
+
+static int lo_find_get_ids_key(struct lo_key *ids_key, const void *opaque)
+{
+ const struct lo_find_get_ids_key_opaque *stat_info = opaque;
+
+ *ids_key = (struct lo_key){
+ .ino = stat_info->st->st_ino,
+ .dev = stat_info->st->st_dev,
+ .mnt_id = stat_info->mnt_id,
+ };
+
+ return 0;
+}
+
+static struct lo_inode *lo_find(struct lo_data *lo,
+ const struct lo_fhandle *fhandle,
+ struct stat *st, uint64_t mnt_id)
+{
+ const struct lo_find_get_ids_key_opaque stat_info = {
+ .st = st,
+ .mnt_id = mnt_id,
+ };
+
+ return lo_do_find(lo, fhandle, lo_find_get_ids_key, &stat_info);
+}
+
/* value_destroy_func for posix_locks GHashTable */
static void posix_locks_value_destroy(gpointer data)
{
@@ -1655,14 +1686,41 @@ out_err:
fuse_reply_err(req, saverr);
}
+struct lookup_name_get_ids_key_opaque {
+ struct lo_data *lo;
+ int parent_fd;
+ const char *name;
+};
+
+static int lookup_name_get_ids_key(struct lo_key *ids_key, const void *opaque)
+{
+ const struct lookup_name_get_ids_key_opaque *stat_params = opaque;
+ uint64_t mnt_id;
+ struct stat attr;
+ int res;
+
+ res = do_statx(stat_params->lo, stat_params->parent_fd, stat_params->name,
+ &attr, AT_SYMLINK_NOFOLLOW, &mnt_id);
+ if (res < 0) {
+ return -errno;
+ }
+
+ *ids_key = (struct lo_key){
+ .ino = attr.st_ino,
+ .dev = attr.st_dev,
+ .mnt_id = mnt_id,
+ };
+
+ return 0;
+}
+
/* Increments nlookup and caller must release refcount using lo_inode_put() */
static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
const char *name)
{
g_auto(TempFd) dir_fd = TEMP_FD_INIT;
int res;
- uint64_t mnt_id;
- struct stat attr;
+ struct lookup_name_get_ids_key_opaque stat_params;
struct lo_fhandle *fh;
struct lo_data *lo = lo_data(req);
struct lo_inode *dir = lo_inode(req, parent);
@@ -1680,13 +1738,14 @@ static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
fh = get_file_handle(lo, dir_fd.fd, name);
/* Ignore errors, this is just an optional key for the lookup */
- res = do_statx(lo, dir_fd.fd, name, &attr, AT_SYMLINK_NOFOLLOW, &mnt_id);
- lo_inode_put(lo, &dir);
- if (res == -1) {
- return NULL;
- }
+ stat_params = (struct lookup_name_get_ids_key_opaque){
+ .lo = lo,
+ .parent_fd = dir_fd.fd,
+ .name = name,
+ };
- inode = lo_find(lo, fh, &attr, mnt_id);
+ inode = lo_do_find(lo, fh, lookup_name_get_ids_key, &stat_params);
+ lo_inode_put(lo, &dir);
g_free(fh);
return inode;
--
2.31.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Virtio-fs] [PATCH 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs
2021-06-04 16:13 [PATCH 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs Max Reitz
` (8 preceding siblings ...)
2021-06-04 16:13 ` [PATCH 9/9] virtiofsd: Add lazy lo_do_find() Max Reitz
@ 2021-06-04 21:17 ` Connor Kuehl
9 siblings, 0 replies; 13+ messages in thread
From: Connor Kuehl @ 2021-06-04 21:17 UTC (permalink / raw)
To: Max Reitz, qemu-devel, virtio-fs
On 6/4/21 11:13 AM, Max Reitz wrote:
> Hi,
>
> This is the C virtiofsd equivalent to
> https://gitlab.com/virtio-fs/virtiofsd-rs/-/merge_requests/26. As such,
> the summary is pretty much the same:
>
> Storing an O_PATH file descriptor in every lo_inode object means we have
> a lot of FDs open, which is sometimes bad. This series adds an option
> (-o inode_file_handles) that will make virtiofsd attempt to generate a
> file handle for new inodes and store that instead of an FD. When an FD
> is needed for a given inode, we open the handle.
>
> To accomplish this, lo_inode.fd is should not be accessed directly
> anymore, but only through helper functions (mainly lo_inode_fd() and
> lo_inode_open()). A TempFd object is added to hide the difference
> between FDs that are bound to the lo_inode object (and so need not be
> closed after use) and temporary FDs from open_by_handle_at() (which do
> need to be closed after use).
>
> To prevent the problem I spent a long time talking about (if we don’t
> have an FD open for every inode, the inode can be deleted, its ID
> reused, and then the lookup in lo_data.inodes will find the old deleted
> inode), patch 7 adds a second hash table lo_data.inodes_by_handle that
> maps file handles to lo_inode objects. (Because file handles include a
> generation ID, so we can discern between the old and the new inode.)
>
> Patch 9 is completely optional, but I just really felt compelled to
> write it.
>
>
> Max Reitz (9):
> virtiofsd: Add TempFd structure
> virtiofsd: Use lo_inode_open() instead of openat()
> virtiofsd: Add lo_inode_fd() helper
> virtiofsd: Let lo_fd() return a TempFd
> virtiofsd: Let lo_inode_open() return a TempFd
> virtiofsd: Add lo_inode.fhandle
> virtiofsd: Add inodes_by_handle hash table
> virtiofsd: Optionally fill lo_inode.fhandle
> virtiofsd: Add lazy lo_do_find()
>
> tools/virtiofsd/helper.c | 3 +
> tools/virtiofsd/passthrough_ll.c | 809 +++++++++++++++++++++-----
> tools/virtiofsd/passthrough_seccomp.c | 2 +
> 3 files changed, 667 insertions(+), 147 deletions(-)
>
For the series:
Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
^ permalink raw reply [flat|nested] 13+ messages in thread