From: Vivek Goyal <vgoyal@redhat.com>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
virtio-fs-list <virtio-fs@redhat.com>,
Luis Henriques <lhenriques@suse.de>,
"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
Seth Forshee <seth.forshee@canonical.com>
Subject: Re: [PATCH v2 0/2] fuse: Fix clearing SGID when access ACL is set
Date: Thu, 17 Jun 2021 10:35:31 -0400 [thread overview]
Message-ID: <20210617143531.GB1142820@redhat.com> (raw)
In-Reply-To: <CAJfpegvU9zjT7qV=Rj4ok4kfYz-9BPhjp+xKz9odfSWaFxshyA@mail.gmail.com>
On Wed, Apr 14, 2021 at 01:57:01PM +0200, Miklos Szeredi wrote:
> On Thu, Mar 25, 2021 at 4:19 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> >
> > Hi,
> >
> > This is V2 of the patchset. Posted V1 here.
> >
> > https://lore.kernel.org/linux-fsdevel/20210319195547.427371-1-vgoyal@redhat.com/
> >
> > Changes since V1:
> >
> > - Dropped the helper to determine if SGID should be cleared and open
> > coded it instead. I will follow up on helper separately in a different
> > patch series. There are few places already which open code this, so
> > for now fuse can do the same. Atleast I can make progress on this
> > and virtiofs can enable ACL support.
> >
> > Luis reported that xfstests generic/375 fails with virtiofs. Little
> > debugging showed that when posix access acl is set that in some
> > cases SGID needs to be cleared and that does not happen with virtiofs.
> >
> > Setting posix access acl can lead to mode change and it can also lead
> > to clear of SGID. fuse relies on file server taking care of all
> > the mode changes. But file server does not have enough information to
> > determine whether SGID should be cleared or not.
> >
> > Hence this patch series add support to send a flag in SETXATTR message
> > to tell server to clear SGID.
>
> Changed it to have a single extended structure for the request, which
> is how this has always been handled in the fuse API.
>
> The ABI is unchanged, but you'll need to update the userspace part
> according to the API change. Otherwise looks good.
Hi Miklos,
I started looking at ACL patches for virtiofsd again. And realized
that this SETXATTR_EXT patch, changes API. So if I update kernel
headers and recompile virtiofsd, setxattr is broken.
# setfattr -n "user.foo" -v "bar" foo.txt
setfattr: foo.txt: Numerical result out of range
I can fix it using following patch. But I am little concerned that
all the users of fuse will have to apply similar patch if they update
kernel headers (including libfuse) and recompile their app.
Is that a concern, or should we rework this so that kernel header
update does not break users.
Thanks
Vivek
---
tools/virtiofsd/fuse_common.h | 5 +++++
tools/virtiofsd/fuse_lowlevel.c | 7 ++++++-
2 files changed, 11 insertions(+), 1 deletion(-)
Index: rhvgoyal-qemu/tools/virtiofsd/fuse_lowlevel.c
===================================================================
--- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_lowlevel.c 2021-06-16 17:39:16.387405071 -0400
+++ rhvgoyal-qemu/tools/virtiofsd/fuse_lowlevel.c 2021-06-17 10:22:04.879150980 -0400
@@ -1419,8 +1419,13 @@ static void do_setxattr(fuse_req_t req,
struct fuse_setxattr_in *arg;
const char *name;
const char *value;
+ bool setxattr_ext = req->se->conn.want & FUSE_CAP_SETXATTR_EXT;
+
+ if (setxattr_ext)
+ arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
+ else
+ arg = fuse_mbuf_iter_advance(iter, FUSE_COMPAT_SETXATTR_IN_SIZE);
- arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
name = fuse_mbuf_iter_advance_str(iter);
if (!arg || !name) {
fuse_reply_err(req, EINVAL);
Index: rhvgoyal-qemu/tools/virtiofsd/fuse_common.h
===================================================================
--- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_common.h 2021-06-16 17:39:16.387405071 -0400
+++ rhvgoyal-qemu/tools/virtiofsd/fuse_common.h 2021-06-17 10:24:20.905937326 -0400
@@ -373,6 +373,11 @@ struct fuse_file_info {
#define FUSE_CAP_HANDLE_KILLPRIV_V2 (1 << 28)
/**
+ * Indicates that file server supports extended struct fuse_setxattr_in
+ */
+#define FUSE_CAP_SETXATTR_EXT (1 << 29)
+
+/**
* Ioctl flags
*
* FUSE_IOCTL_COMPAT: 32bit compat ioctl on 64bit machine
prev parent reply other threads:[~2021-06-17 14:35 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-25 15:18 [PATCH v2 0/2] fuse: Fix clearing SGID when access ACL is set Vivek Goyal
2021-03-25 15:18 ` [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2 Vivek Goyal
2021-03-29 14:50 ` Luis Henriques
2021-03-29 18:16 ` Vivek Goyal
2021-03-29 14:54 ` Luis Henriques
2021-03-29 18:24 ` Vivek Goyal
2021-03-29 20:27 ` Luis Henriques
2021-03-25 15:18 ` [PATCH v2 2/2] fuse: Add a flag FUSE_SETXATTR_ACL_KILL_SGID to kill SGID Vivek Goyal
2021-04-13 20:41 ` [Virtio-fs] [PATCH v2 0/2] fuse: Fix clearing SGID when access ACL is set Vivek Goyal
2021-04-14 11:57 ` Miklos Szeredi
2021-04-14 12:58 ` Vivek Goyal
2021-06-17 14:35 ` Vivek Goyal [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210617143531.GB1142820@redhat.com \
--to=vgoyal@redhat.com \
--cc=dgilbert@redhat.com \
--cc=lhenriques@suse.de \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=miklos@szeredi.hu \
--cc=seth.forshee@canonical.com \
--cc=virtio-fs@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).