On Thu, Sep 16, 2021 at 10:40:34AM +0200, Hanna Reitz wrote: > File handles are specific to mounts, and so name_to_handle_at() returns > the respective mount ID. However, open_by_handle_at() is not content > with an ID, it wants a file descriptor for some inode on the mount, > which we have to open. > > We want to use /proc/self/mountinfo to find the mounts' root directories > so we can open them and pass the respective FDs to open_by_handle_at(). > (We need to use the root directory, because we want the inode belonging > to every mount FD be deletable. Before the root directory can be > deleted, all entries within must have been closed, and so when it is > deleted, there should not be any file handles left that need its FD as > their mount FD. Thus, we can then close that FD and the inode can be > deleted.[1]) > > That is why we need to open /proc/self/mountinfo so that we can use it > to translate mount IDs into root directory paths. We have to open it > after setup_mounts() was called, because if we try to open it before, it > will appear as an empty file after setup_mounts(). > > [1] Note that in practice, you still cannot delete the mount root > directory. It is a mount point on the host, after all, and mount points > cannot be deleted. But by using the mount point as the mount FD, we > will at least not hog any actually deletable inodes. > > Signed-off-by: Hanna Reitz <hre...@redhat.com> > --- > tools/virtiofsd/passthrough_ll.c | 40 ++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/tools/virtiofsd/passthrough_ll.c > b/tools/virtiofsd/passthrough_ll.c > index 38b2af8599..6511a6acb4 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -172,6 +172,8 @@ struct lo_data { > > /* An O_PATH file descriptor to /proc/self/fd/ */ > int proc_self_fd; > + /* A read-only FILE pointer for /proc/self/mountinfo */ > + FILE *mountinfo_fp; > int user_killpriv_v2, killpriv_v2; > /* If set, virtiofsd is responsible for setting umask during creation */ > bool change_umask; > @@ -3718,6 +3720,19 @@ static void setup_chroot(struct lo_data *lo) > static void setup_sandbox(struct lo_data *lo, struct fuse_session *se, > bool enable_syslog) > { > + int proc_self, mountinfo_fd; > + int saverr; > + > + /* > + * Open /proc/self before we pivot to the new root so we can still > + * open /proc/self/mountinfo afterwards > + */ > + proc_self = open("/proc/self", O_PATH); > + if (proc_self < 0) { > + fuse_log(FUSE_LOG_WARNING, "Failed to open /proc/self: %m; " > + "will not be able to use file handles\n"); > + } > +
Hi Hanna, Should we open /proc/self and /proc/self/mountinfo only if user wants to file handle. We have already parsed options by now so we know. Also, if user asked for file handles, and we can't open /proc/self or /proc/self/mountinfo successfully, I would think we should error out and not continue (instead of just log it and continue). That seems to be general theme. If user asked for a feature and if we can't enable it, we error out and let user retry without that particular feature. > if (lo->sandbox == SANDBOX_NAMESPACE) { > setup_namespaces(lo, se); > setup_mounts(lo->source); > @@ -3725,6 +3740,31 @@ static void setup_sandbox(struct lo_data *lo, struct > fuse_session *se, > setup_chroot(lo); > } > > + /* > + * Opening /proc/self/mountinfo before the umount2() call in > + * setup_mounts() leads to the file appearing empty. That is why > + * we defer opening it until here. > + */ > + lo->mountinfo_fp = NULL; > + if (proc_self >= 0) { > + mountinfo_fd = openat(proc_self, "mountinfo", O_RDONLY); > + if (mountinfo_fd < 0) { > + saverr = errno; > + } else if (mountinfo_fd >= 0) { > + lo->mountinfo_fp = fdopen(mountinfo_fd, "r"); > + if (!lo->mountinfo_fp) { > + saverr = errno; > + close(mountinfo_fd); > + } > + } > + if (!lo->mountinfo_fp) { > + fuse_log(FUSE_LOG_WARNING, "Failed to open /proc/self/mountinfo: > " > + "%s; will not be able to use file handles\n", > + strerror(saverr)); > + } > + close(proc_self); > + } > + Above code couple probably be moved in a helper function. Makes it easier to read setup_sandbox(). Same here, open mountinfo only if user wants file handle support and error out if file handle support can't be enabled. Thanks Vivek > setup_seccomp(enable_syslog); > setup_capabilities(g_strdup(lo->modcaps)); > } > -- > 2.31.1 >