[Virtio-fs] [PATCH v2 2/2] virtiofsd: Add support of posix_acl
misono.tomohiro at fujitsu.com
misono.tomohiro at fujitsu.com
Fri Jan 31 01:59:26 UTC 2020
> On Tue, Jan 28, 2020 at 07:18:19PM +0900, Misono Tomohiro wrote:
> > This commit adds support of posix_acl which is enabled by 'posix_acl'
> > option (default: disabled).
> >
> > If posix_acl is enabled, virtiofsd handles umask handling otherwise
> > done in guest kernel in order to treat default ACL correctly.
> >
> > With this, ACL-related xfstest generic/{099,237,319,444} passes on XFS
> > backend.
>
> Can you please explain what's the problem with current code.
Default ACL can be set to directory only.
Usually default permission when creating files is masked by current umask value.
If directory has default ACL value, however, its value takes priority over umask
(i.e. default ACL value is used and umask value is ignored).
Without this patch (without specifying FUSE_CAP_DONT_MASK),
fuse kernel module always performs umasking before sending requests regardless of ACL settings:
https://github.com/torvalds/linux/blob/master/fs/fuse/dir.c#L459-L460
So, default ACL does not work properly with current code,
I believe FUSE_CAP_DONT_MASK was actually introduced to fix this problem:
https://github.com/torvalds/linux/commit/e0a43ddcc08c34dbd666d93600fd23914505f4aa
Thanks,
Misono
>
> Thanks
> Vivek
> >
> > Implementation:
> > virtiofsd adds FUSE_CAP_POSIX_ACL/FUSE_CAP_DONT_MASK to INIT replay
> > if posix_acl is enabled. This results in skipping umask handling in
> > guest kernel. In turn, virtiofsd sets umask before creating files by
> > using umask value included in fuse_request and restore umask after that.
> > Note that calling umask() is ok since now each worker thread has
> > called
> > unshare(CLONE_FS) when starting up.
> >
> > Signed-off-by: Misono Tomohiro <misono.tomohiro at jp.fujitsu.com>
> > ---
> > tools/virtiofsd/helper.c | 3 +++
> > tools/virtiofsd/passthrough_ll.c | 36 ++++++++++++++++++++++++++------
> > tools/virtiofsd/seccomp.c | 1 +
> > 3 files changed, 34 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c index
> > 0801cf752c..97968a9385 100644
> > --- a/tools/virtiofsd/helper.c
> > +++ b/tools/virtiofsd/helper.c
> > @@ -171,6 +171,9 @@ void fuse_cmdline_help(void)
> > " default: no_writeback\n"
> > " -o xattr|no_xattr enable/disable xattr\n"
> > " default: no_xattr\n"
> > + " -o posix_acl|no_posxi_acl enable/disable posix_acl\n"
> > + " enabling this also enables xattr\n"
> > + " default: no_posix_acl\n"
> > );
> > }
> >
> > diff --git a/tools/virtiofsd/passthrough_ll.c
> > b/tools/virtiofsd/passthrough_ll.c
> > index 6bcc33e0eb..e1bb261b51 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -152,6 +152,7 @@ struct lo_data {
> > int flock;
> > int posix_lock;
> > int xattr;
> > + int posix_acl;
> > char *source;
> > double timeout;
> > int cache;
> > @@ -182,6 +183,8 @@ static const struct fuse_opt lo_opts[] = {
> > { "no_posix_lock", offsetof(struct lo_data, posix_lock), 0 },
> > { "xattr", offsetof(struct lo_data, xattr), 1 },
> > { "no_xattr", offsetof(struct lo_data, xattr), 0 },
> > + { "posix_acl", offsetof(struct lo_data, posix_acl), 1 },
> > + { "no_posix_acl", offsetof(struct lo_data, posix_acl), 0 },
> > { "timeout=%lf", offsetof(struct lo_data, timeout), 0 },
> > { "timeout=", offsetof(struct lo_data, timeout_set), 1 },
> > { "cache=none", offsetof(struct lo_data, cache), CACHE_NONE }, @@
> > -587,6 +590,17 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
> > fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling readdirplus\n");
> > conn->want &= ~FUSE_CAP_READDIRPLUS;
> > }
> > +
> > + if (conn->capable & FUSE_CAP_POSIX_ACL) {
> > + if (lo->posix_acl) {
> > + if (!lo->xattr) {
> > + fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling xattr for ACL\n");
> > + lo->xattr = 1;
> > + }
> > + fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling ACL\n");
> > + conn->want |= (FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK);
> > + }
> > + }
> > }
> >
> > static int64_t *version_ptr(struct lo_data *lo, struct lo_inode
> > *inode) @@ -1137,9 +1151,11 @@ static void lo_lookup(fuse_req_t req,
> > fuse_ino_t parent, const char *name)
> > /*
> > * Change to uid/gid of caller so that file is created with
> > * ownership of caller.
> > + * Also update umask if posix_acl is enabled.
> > * TODO: What about selinux context?
> > */
> > -static int lo_change_cred(fuse_req_t req, struct lo_cred *old)
> > +static int lo_change_cred_and_umask(fuse_req_t req, struct lo_cred *old,
> > + struct lo_data *lo)
> > {
> > int res;
> >
> > @@ -1159,11 +1175,15 @@ static int lo_change_cred(fuse_req_t req, struct lo_cred *old)
> > return errno_save;
> > }
> >
> > + if (lo->posix_acl) {
> > + umask(req->ctx.umask);
> > + }
> > +
> > return 0;
> > }
> >
> > /* Regain Privileges */
> > -static void lo_restore_cred(struct lo_cred *old)
> > +static void lo_restore_cred_and_umask(struct lo_cred *old, struct
> > +lo_data *lo)
> > {
> > int res;
> >
> > @@ -1178,6 +1198,10 @@ static void lo_restore_cred(struct lo_cred *old)
> > fuse_log(FUSE_LOG_ERR, "setegid(%u): %m\n", old->egid);
> > exit(1);
> > }
> > +
> > + if (lo->posix_acl) {
> > + umask(0);
> > + }
> > }
> >
> > static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, @@
> > -1204,7 +1228,7 @@ static void lo_mknod_symlink(fuse_req_t req,
> > fuse_ino_t parent,
> >
> > saverr = ENOMEM;
> >
> > - saverr = lo_change_cred(req, &old);
> > + saverr = lo_change_cred_and_umask(req, &old, lo);
> > if (saverr) {
> > goto out;
> > }
> > @@ -1213,7 +1237,7 @@ static void lo_mknod_symlink(fuse_req_t req,
> > fuse_ino_t parent,
> >
> > saverr = errno;
> >
> > - lo_restore_cred(&old);
> > + lo_restore_cred_and_umask(&old, lo);
> >
> > if (res == -1) {
> > goto out;
> > @@ -1898,7 +1922,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > return;
> > }
> >
> > - err = lo_change_cred(req, &old);
> > + err = lo_change_cred_and_umask(req, &old, lo);
> > if (err) {
> > goto out;
> > }
> > @@ -1908,7 +1932,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> > mode);
> > err = fd == -1 ? errno : 0;
> > - lo_restore_cred(&old);
> > + lo_restore_cred_and_umask(&old, lo);
> >
> > if (!err) {
> > ssize_t fh;
> > diff --git a/tools/virtiofsd/seccomp.c b/tools/virtiofsd/seccomp.c
> > index 9ee99e4725..1b6f579452 100644
> > --- a/tools/virtiofsd/seccomp.c
> > +++ b/tools/virtiofsd/seccomp.c
> > @@ -102,6 +102,7 @@ static const int syscall_whitelist[] = {
> > SCMP_SYS(symlinkat),
> > SCMP_SYS(time), /* Rarely needed, except on static builds */
> > SCMP_SYS(tgkill),
> > + SCMP_SYS(umask),
> > SCMP_SYS(unlinkat),
> > SCMP_SYS(unshare),
> > SCMP_SYS(utimensat),
> > --
> > 2.21.1
> >
More information about the Virtio-fs
mailing list