[PATCH 0/1] fuse: acl: Send file mode updates using SETATTR

[PATCH 0/1] fuse: acl: Send file mode updates using SETATTR

[Vivek Goyal]

Hi Miklos,

Please find attached a patch to fix the SGID clearing issue upon 
ACL change. 

Luis reported that currently fstests generic/375 fails on virtiofs. And
reason being that we don't clear SGID when it should be.

Setting ACL can lead to file mode change. And this in-turn also can
lead to clearing SGID bit if.

- None of caller's groups match file owner group.
AND
- Caller does not have CAP_FSETID.

Current implementation relies on server updating the mode. But file
server does not have enough information to do so. 

Initially I thought of sending CAP_FSETID information to server but
then I realized, it is just one of the pieces. What about all the
groups caller is a member of. If this has to work correctly, then
all the information will have to be sent to virtiofsd somehow. Just
sending CAP_FSETID information required adding V2 of fuse_setxattr_in
because we don't have any space for sending extra information.

https://github.com/rhvgoyal/linux/commit/681cf5bdbba9c965c3dbd4337c16e9b17f27debe

Also this approach will not work with idmapped mounts because server
does not have information about idmapped mappings.

So I started to look at the approach of sending file mode updates
using SETATTR. As filesystems like 9pfs and ceph are doing. This
seems simpler approach. Though it has its issues too.

- File mode update and setxattr(system.posix_acl_access) are not atomic.

None of the approaches seem very clean to me. But sending SETATTR
explicitly seems to be lesser of two evils to me at this point of time.
Hence I am proposing this patch. 

I have run fstests acl tests and they pass. (./check -g acl).

Corresponding virtiofsd patches are here.

https://github.com/rhvgoyal/qemu/commits/acl-sgid-setattr

What do you think.

Vivek Goyal (1):
  fuse: Add a mode where fuse client sends mode changes on ACL change

 fs/fuse/acl.c             | 54 ++++++++++++++++++++++++++++++++++++---
 fs/fuse/dir.c             | 11 ++++----
 fs/fuse/fuse_i.h          |  9 ++++++-
 fs/fuse/inode.c           |  4 ++-
 include/uapi/linux/fuse.h |  5 ++++
 5 files changed, 71 insertions(+), 12 deletions(-)

-- 
2.25.4

[PATCH 1/1] fuse: send file mode updates using SETATTR

[Vivek Goyal]

If ACL changes, it is possible that file mode permission bits change. As of
now fuse client relies on file server to make those changes. But it does
not send enough information to server so that it can decide where SGID
bit should be cleared or not. Server does not know if caller has CAP_FSETID
or not. It also does not know what are caller's group memberships and if any
of the groups match file owner group.

So add a flag FUSE_POSIX_ACL_UPDATE_MODE where server can specify that if
mode changes due to ACL change, then client needs to send explicit SETATTR
to update mode.

Reported-by: Luis Henriques <lhenriques@suse.de>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/acl.c             | 54 ++++++++++++++++++++++++++++++++++++---
 fs/fuse/dir.c             | 11 ++++----
 fs/fuse/fuse_i.h          |  9 ++++++-
 fs/fuse/inode.c           |  4 ++-
 include/uapi/linux/fuse.h |  5 ++++
 5 files changed, 71 insertions(+), 12 deletions(-)

diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
index e9c0f916349d..38920dcfb710 100644
--- a/fs/fuse/acl.c
+++ b/fs/fuse/acl.c
@@ -50,10 +50,31 @@ struct posix_acl *fuse_get_acl(struct inode *inode, int type)
 	return acl;
 }
 
+static int fuse_acl_mode_setattr(struct inode *inode, umode_t mode)
+{
+	struct fuse_mount *fm = get_fuse_mount(inode);
+	FUSE_ARGS(args);
+	struct fuse_setattr_in inarg;
+	struct fuse_attr_out outarg;
+
+	memset(&inarg, 0, sizeof(inarg));
+	memset(&outarg, 0, sizeof(outarg));
+
+	inarg.valid = FATTR_MODE;
+	inarg.mode = mode;
+	fuse_setattr_fill(fm->fc, &args, inode, &inarg, &outarg);
+
+	return fuse_simple_request(fm, &args);
+}
+
 int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
 		 struct posix_acl *acl, int type)
 {
 	struct fuse_conn *fc = get_fuse_conn(inode);
+	umode_t new_mode;
+	bool update_mode = false;
+	size_t size = 0;
+	void *value =  NULL;
 	const char *name;
 	int ret;
 
@@ -63,9 +84,24 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
 	if (!fc->posix_acl || fc->no_setxattr)
 		return -EOPNOTSUPP;
 
-	if (type == ACL_TYPE_ACCESS)
+	if (type == ACL_TYPE_ACCESS) {
 		name = XATTR_NAME_POSIX_ACL_ACCESS;
-	else if (type == ACL_TYPE_DEFAULT)
+		if (acl && fc->posix_acl_update_mode) {
+			/*
+			 * Setting access ACL might clear SGID.
+			 * Refresh inode->i_mode before making a decision.
+			 */
+			ret = fuse_do_getattr(inode, NULL, NULL);
+			if (ret)
+				return ret;
+			ret = posix_acl_update_mode(&init_user_ns, inode,
+						    &new_mode, &acl);
+			if (ret)
+				return ret;
+			if (new_mode != inode->i_mode)
+				update_mode = true;
+		}
+	} else if (type == ACL_TYPE_DEFAULT)
 		name = XATTR_NAME_POSIX_ACL_DEFAULT;
 	else
 		return -EINVAL;
@@ -78,8 +114,7 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
 		 * them to be refreshed the next time they are used,
 		 * and it also updates i_ctime.
 		 */
-		size_t size = posix_acl_xattr_size(acl->a_count);
-		void *value;
+		size = posix_acl_xattr_size(acl->a_count);
 
 		if (size > PAGE_SIZE)
 			return -E2BIG;
@@ -93,8 +128,19 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
 			kfree(value);
 			return ret;
 		}
+	}
+
+	if (update_mode) {
+		ret = fuse_acl_mode_setattr(inode, new_mode);
+		if (ret < 0) {
+			kfree(value);
+			return ret;
+		}
+	}
 
+	if (acl) {
 		ret = fuse_setxattr(inode, name, value, size, 0);
+		/* TODO: If setxattr failed, should we restore mode ? */
 		kfree(value);
 	} else {
 		ret = fuse_removexattr(inode, name);
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 06a18700a845..1d5c5aafb82d 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1022,8 +1022,7 @@ static void fuse_fillattr(struct inode *inode, struct fuse_attr *attr,
 	stat->blksize = 1 << blkbits;
 }
 
-static int fuse_do_getattr(struct inode *inode, struct kstat *stat,
-			   struct file *file)
+int fuse_do_getattr(struct inode *inode, struct kstat *stat, struct file *file)
 {
 	int err;
 	struct fuse_getattr_in inarg;
@@ -1541,10 +1540,10 @@ void fuse_release_nowrite(struct inode *inode)
 	spin_unlock(&fi->lock);
 }
 
-static void fuse_setattr_fill(struct fuse_conn *fc, struct fuse_args *args,
-			      struct inode *inode,
-			      struct fuse_setattr_in *inarg_p,
-			      struct fuse_attr_out *outarg_p)
+void fuse_setattr_fill(struct fuse_conn *fc, struct fuse_args *args,
+		       struct inode *inode,
+		       struct fuse_setattr_in *inarg_p,
+		       struct fuse_attr_out *outarg_p)
 {
 	args->opcode = FUSE_SETATTR;
 	args->nodeid = get_node_id(inode);
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 68cca8d4db6e..ba836daecd08 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -731,6 +731,9 @@ struct fuse_conn {
 	/** Does the filesystem support posix acls? */
 	unsigned posix_acl:1;
 
+	/** If posix acl results in file mode change, send update to fs */
+	unsigned posix_acl_update_mode:1;
+
 	/** Check permissions based on the file mode or not? */
 	unsigned default_permissions:1;
 
@@ -1097,6 +1100,7 @@ u64 fuse_lock_owner_id(struct fuse_conn *fc, fl_owner_t id);
 
 void fuse_update_ctime(struct inode *inode);
 
+int fuse_do_getattr(struct inode *inode, struct kstat *stat, struct file *file);
 int fuse_update_attributes(struct inode *inode, struct file *file);
 
 void fuse_flush_writepages(struct inode *inode);
@@ -1162,7 +1166,10 @@ int fuse_write_inode(struct inode *inode, struct writeback_control *wbc);
 
 int fuse_do_setattr(struct dentry *dentry, struct iattr *attr,
 		    struct file *file);
-
+void fuse_setattr_fill(struct fuse_conn *fc, struct fuse_args *args,
+                       struct inode *inode,
+                       struct fuse_setattr_in *inarg_p,
+                       struct fuse_attr_out *outarg_p);
 void fuse_set_initialized(struct fuse_conn *fc);
 
 void fuse_unlock_inode(struct inode *inode, bool locked);
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index b0e18b470e91..678145f987d1 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1052,6 +1052,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
 				fc->handle_killpriv_v2 = 1;
 				fm->sb->s_flags |= SB_NOSEC;
 			}
+			if (arg->flags & FUSE_POSIX_ACL_UPDATE_MODE)
+				fc->posix_acl_update_mode = 1;
 		} else {
 			ra_pages = fc->max_read / PAGE_SIZE;
 			fc->no_lock = 1;
@@ -1095,7 +1097,7 @@ void fuse_send_init(struct fuse_mount *fm)
 		FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
 		FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
 		FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
-		FUSE_HANDLE_KILLPRIV_V2;
+		FUSE_HANDLE_KILLPRIV_V2 | FUSE_POSIX_ACL_UPDATE_MODE;
 #ifdef CONFIG_FUSE_DAX
 	if (fm->fc->dax)
 		ia->in.flags |= FUSE_MAP_ALIGNMENT;
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 98ca64d1beb6..89c3a88354f4 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -179,6 +179,7 @@
  *  7.33
  *  - add FUSE_HANDLE_KILLPRIV_V2, FUSE_WRITE_KILL_SUIDGID, FATTR_KILL_SUIDGID
  *  - add FUSE_OPEN_KILL_SUIDGID
+ *  - add FUSE_POSIX_ACL_UPDATE_MODE
  */
 
 #ifndef _LINUX_FUSE_H
@@ -330,6 +331,9 @@ struct fuse_file_lock {
  *			does not have CAP_FSETID. Additionally upon
  *			write/truncate sgid is killed only if file has group
  *			execute permission. (Same as Linux VFS behavior).
+ * FUSE_POSIX_ACL_UPDATE_MODE: When posix acl setting results in a mode change,
+ *                             client should send explicit setattr message to
+ *                             update mode.
  */
 #define FUSE_ASYNC_READ		(1 << 0)
 #define FUSE_POSIX_LOCKS	(1 << 1)
@@ -360,6 +364,7 @@ struct fuse_file_lock {
 #define FUSE_MAP_ALIGNMENT	(1 << 26)
 #define FUSE_SUBMOUNTS		(1 << 27)
 #define FUSE_HANDLE_KILLPRIV_V2	(1 << 28)
+#define FUSE_POSIX_ACL_UPDATE_MODE	(1 << 29)
 
 /**
  * CUSE INIT request/reply flags
-- 
2.25.4

Re: [PATCH 1/1] fuse: send file mode updates using SETATTR

[Miklos Szeredi]

On Tue, Mar 16, 2021 at 5:02 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> If ACL changes, it is possible that file mode permission bits change. As of
> now fuse client relies on file server to make those changes. But it does
> not send enough information to server so that it can decide where SGID
> bit should be cleared or not. Server does not know if caller has CAP_FSETID
> or not. It also does not know what are caller's group memberships and if any
> of the groups match file owner group.

Right.  So what about performing the capability and group membership
check in the client and sending the result of this check to the
server?

Yes, need to extend fuse_setxattr_in.

There's still a race with uid and gid changing on the underlying
filesystem, so the attributes need to be refreshed, but I don't think
that's a big worry.

Thanks,
Miklos

Re: [PATCH 1/1] fuse: send file mode updates using SETATTR

[Vivek Goyal]

On Wed, Mar 17, 2021 at 04:43:35PM +0100, Miklos Szeredi wrote:
> On Tue, Mar 16, 2021 at 5:02 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > If ACL changes, it is possible that file mode permission bits change. As of
> > now fuse client relies on file server to make those changes. But it does
> > not send enough information to server so that it can decide where SGID
> > bit should be cleared or not. Server does not know if caller has CAP_FSETID
> > or not. It also does not know what are caller's group memberships and if any
> > of the groups match file owner group.
> 
> Right.  So what about performing the capability and group membership
> check in the client and sending the result of this check to the
> server?

Hi Miklos,

But that will still be non-atomic, right? I mean server probably will
do setxattr first, then check if SGID was cleared or not, and if it
has not been cleared, then it needs to set the mode.

IOW, we still have two operations (setxattr followed by mode setting).

I had thought about that option. But could not understand what does
it buy us as opposed to guest sending a SETATTR.

> 
> Yes, need to extend fuse_setxattr_in.

Ok.
> 
> There's still a race with uid and gid changing on the underlying
> filesystem, so the attributes need to be refreshed, but I don't think
> that's a big worry.

Yes, attributes will need to be refreshed.

Thanks
Vivek

Re: [PATCH 1/1] fuse: send file mode updates using SETATTR

[Miklos Szeredi]

On Wed, Mar 17, 2021 at 6:01 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Wed, Mar 17, 2021 at 04:43:35PM +0100, Miklos Szeredi wrote:
> > On Tue, Mar 16, 2021 at 5:02 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > If ACL changes, it is possible that file mode permission bits change. As of
> > > now fuse client relies on file server to make those changes. But it does
> > > not send enough information to server so that it can decide where SGID
> > > bit should be cleared or not. Server does not know if caller has CAP_FSETID
> > > or not. It also does not know what are caller's group memberships and if any
> > > of the groups match file owner group.
> >
> > Right.  So what about performing the capability and group membership
> > check in the client and sending the result of this check to the
> > server?
>
> Hi Miklos,
>
> But that will still be non-atomic, right? I mean server probably will
> do setxattr first, then check if SGID was cleared or not, and if it
> has not been cleared, then it needs to set the mode.
>
> IOW, we still have two operations (setxattr followed by mode setting).
>
> I had thought about that option. But could not understand what does
> it buy us as opposed to guest sending a SETATTR.

If the non-atomic SETXATTR + SETATTR is done in the client, then the
server has no chance of ever operating correctly.

If the responsibility for clearing sgid is in the server, then it's up
to the server to decide how to best deal with this.  That may be the
racy way, but at least it's not the only possibility.

Not sure if virtiofsd can do this atomically or not.
setgid()/setgroups() require CAP_SETGID, but that's relative to the
user namespace of the daemon, so this might be possible to do, I
haven't put a lot of thought into this.

Thanks,
Miklos

Re: [PATCH 1/1] fuse: send file mode updates using SETATTR

[Vivek Goyal]

On Wed, Mar 17, 2021 at 08:25:51PM +0100, Miklos Szeredi wrote:
> On Wed, Mar 17, 2021 at 6:01 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Wed, Mar 17, 2021 at 04:43:35PM +0100, Miklos Szeredi wrote:
> > > On Tue, Mar 16, 2021 at 5:02 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > >
> > > > If ACL changes, it is possible that file mode permission bits change. As of
> > > > now fuse client relies on file server to make those changes. But it does
> > > > not send enough information to server so that it can decide where SGID
> > > > bit should be cleared or not. Server does not know if caller has CAP_FSETID
> > > > or not. It also does not know what are caller's group memberships and if any
> > > > of the groups match file owner group.
> > >
> > > Right.  So what about performing the capability and group membership
> > > check in the client and sending the result of this check to the
> > > server?
> >
> > Hi Miklos,
> >
> > But that will still be non-atomic, right? I mean server probably will
> > do setxattr first, then check if SGID was cleared or not, and if it
> > has not been cleared, then it needs to set the mode.
> >
> > IOW, we still have two operations (setxattr followed by mode setting).
> >
> > I had thought about that option. But could not understand what does
> > it buy us as opposed to guest sending a SETATTR.
> 
> If the non-atomic SETXATTR + SETATTR is done in the client, then the
> server has no chance of ever operating correctly.
> 
> If the responsibility for clearing sgid is in the server, then it's up
> to the server to decide how to best deal with this.  That may be the
> racy way, but at least it's not the only possibility.
> 
> Not sure if virtiofsd can do this atomically or not.
> setgid()/setgroups() require CAP_SETGID, but that's relative to the
> user namespace of the daemon, so this might be possible to do, I
> haven't put a lot of thought into this.

I guess you are right. virtiofsd can do it atomically. If client says to
clear SGID, then virtiofsd can do following.

A. Query who is file owner group.
B. setgid(non-file-owner-group)
C. drop CAP_FSETID
D. setxattr(system.posix_acl_access).

If file owner group is not root, then we don't have to do any setgid at all.
If file owner group is root, then we have to find a gid which is valid
in mount namespace of virtiofsd and switch to it. I guess at virtiofsd
start time we can look at /proc/$$/gid_map and store on  non-root gid
which is valid.

Only race here will be if another client changes file owner group
between A and D and sets to same non-file-onwer-group we changed to. In
that case it will not be cleared. 

Hmm.., may be post setxattr we can do another stat() and make sure SGID
got cleared. If not, do a chmod. IOW, if atomic approach races, fall back
to non-atomic approach.

Litle twisted but should probably work. Will try.

Thanks
Vivek

Re: [PATCH 0/1] fuse: acl: Send file mode updates using SETATTR

[Luis Henriques]

On Tue, Mar 16, 2021 at 12:01:46PM -0400, Vivek Goyal wrote:
> Hi Miklos,
> 
> Please find attached a patch to fix the SGID clearing issue upon 
> ACL change. 
> 
> Luis reported that currently fstests generic/375 fails on virtiofs. And
> reason being that we don't clear SGID when it should be.
> 
> Setting ACL can lead to file mode change. And this in-turn also can
> lead to clearing SGID bit if.
> 
> - None of caller's groups match file owner group.
> AND
> - Caller does not have CAP_FSETID.
> 
> Current implementation relies on server updating the mode. But file
> server does not have enough information to do so. 
> 
> Initially I thought of sending CAP_FSETID information to server but
> then I realized, it is just one of the pieces. What about all the
> groups caller is a member of. If this has to work correctly, then
> all the information will have to be sent to virtiofsd somehow. Just
> sending CAP_FSETID information required adding V2 of fuse_setxattr_in
> because we don't have any space for sending extra information.
> 
> https://github.com/rhvgoyal/linux/commit/681cf5bdbba9c965c3dbd4337c16e9b17f27debe
> 
> Also this approach will not work with idmapped mounts because server
> does not have information about idmapped mappings.
> 
> So I started to look at the approach of sending file mode updates
> using SETATTR. As filesystems like 9pfs and ceph are doing. This
> seems simpler approach. Though it has its issues too.
> 
> - File mode update and setxattr(system.posix_acl_access) are not atomic.

After reviewing (and testing) the patch, the only comment I have is that
we should at least pr_warn() an eventual failure in setxattr().  But f
that operation fails at that point, probably something went wrong on the
other side and the kernel is unlikely to be able to revert the mode
changes anyway.

(And a nit: your patch seems to require some whitespaces clean-up.)

Cheers,
--
Luís


> None of the approaches seem very clean to me. But sending SETATTR
> explicitly seems to be lesser of two evils to me at this point of time.
> Hence I am proposing this patch. 
> 
> I have run fstests acl tests and they pass. (./check -g acl).
> 
> Corresponding virtiofsd patches are here.
> 
> https://github.com/rhvgoyal/qemu/commits/acl-sgid-setattr
> 
> What do you think.
> 
> Vivek Goyal (1):
>   fuse: Add a mode where fuse client sends mode changes on ACL change
> 
>  fs/fuse/acl.c             | 54 ++++++++++++++++++++++++++++++++++++---
>  fs/fuse/dir.c             | 11 ++++----
>  fs/fuse/fuse_i.h          |  9 ++++++-
>  fs/fuse/inode.c           |  4 ++-
>  include/uapi/linux/fuse.h |  5 ++++
>  5 files changed, 71 insertions(+), 12 deletions(-)
> 
> -- 
> 2.25.4
> 

Re: [PATCH 0/1] fuse: acl: Send file mode updates using SETATTR

[Vivek Goyal]

On Wed, Mar 17, 2021 at 02:29:03PM +0000, Luis Henriques wrote:
> On Tue, Mar 16, 2021 at 12:01:46PM -0400, Vivek Goyal wrote:
> > Hi Miklos,
> > 
> > Please find attached a patch to fix the SGID clearing issue upon 
> > ACL change. 
> > 
> > Luis reported that currently fstests generic/375 fails on virtiofs. And
> > reason being that we don't clear SGID when it should be.
> > 
> > Setting ACL can lead to file mode change. And this in-turn also can
> > lead to clearing SGID bit if.
> > 
> > - None of caller's groups match file owner group.
> > AND
> > - Caller does not have CAP_FSETID.
> > 
> > Current implementation relies on server updating the mode. But file
> > server does not have enough information to do so. 
> > 
> > Initially I thought of sending CAP_FSETID information to server but
> > then I realized, it is just one of the pieces. What about all the
> > groups caller is a member of. If this has to work correctly, then
> > all the information will have to be sent to virtiofsd somehow. Just
> > sending CAP_FSETID information required adding V2 of fuse_setxattr_in
> > because we don't have any space for sending extra information.
> > 
> > https://github.com/rhvgoyal/linux/commit/681cf5bdbba9c965c3dbd4337c16e9b17f27debe
> > 
> > Also this approach will not work with idmapped mounts because server
> > does not have information about idmapped mappings.
> > 
> > So I started to look at the approach of sending file mode updates
> > using SETATTR. As filesystems like 9pfs and ceph are doing. This
> > seems simpler approach. Though it has its issues too.
> > 
> > - File mode update and setxattr(system.posix_acl_access) are not atomic.
> 
> After reviewing (and testing) the patch, the only comment I have is that
> we should at least pr_warn() an eventual failure in setxattr().  But f
> that operation fails at that point, probably something went wrong on the
> other side

Hi Luis,

If setxattr failed, user will get the error. 

I guess pr_warn() could help with figuring out that there was a side affect
of failed failed setxattr operation. (mode changed). I will add something.

> and the kernel is unlikely to be able to revert the mode
> changes anyway.

Interestingly ceph code seems to revert mode changes if setxattr fails.
I think for now I am happy with just a pr_warn().
> 
> (And a nit: your patch seems to require some whitespaces clean-up.)

Will check it and fix it and post V2.

Thanks
Vivek

> 
> Cheers,
> --
> Luís
> 
> 
> > None of the approaches seem very clean to me. But sending SETATTR
> > explicitly seems to be lesser of two evils to me at this point of time.
> > Hence I am proposing this patch. 
> > 
> > I have run fstests acl tests and they pass. (./check -g acl).
> > 
> > Corresponding virtiofsd patches are here.
> > 
> > https://github.com/rhvgoyal/qemu/commits/acl-sgid-setattr
> > 
> > What do you think.
> > 
> > Vivek Goyal (1):
> >   fuse: Add a mode where fuse client sends mode changes on ACL change
> > 
> >  fs/fuse/acl.c             | 54 ++++++++++++++++++++++++++++++++++++---
> >  fs/fuse/dir.c             | 11 ++++----
> >  fs/fuse/fuse_i.h          |  9 ++++++-
> >  fs/fuse/inode.c           |  4 ++-
> >  include/uapi/linux/fuse.h |  5 ++++
> >  5 files changed, 71 insertions(+), 12 deletions(-)
> > 
> > -- 
> > 2.25.4
> > 
> 

Re: [PATCH 0/1] fuse: acl: Send file mode updates using SETATTR

[Luis Henriques]

On Wed, Mar 17, 2021 at 11:18:57AM -0400, Vivek Goyal wrote:
> On Wed, Mar 17, 2021 at 02:29:03PM +0000, Luis Henriques wrote:
> > On Tue, Mar 16, 2021 at 12:01:46PM -0400, Vivek Goyal wrote:
> > > Hi Miklos,
> > > 
> > > Please find attached a patch to fix the SGID clearing issue upon 
> > > ACL change. 
> > > 
> > > Luis reported that currently fstests generic/375 fails on virtiofs. And
> > > reason being that we don't clear SGID when it should be.
> > > 
> > > Setting ACL can lead to file mode change. And this in-turn also can
> > > lead to clearing SGID bit if.
> > > 
> > > - None of caller's groups match file owner group.
> > > AND
> > > - Caller does not have CAP_FSETID.
> > > 
> > > Current implementation relies on server updating the mode. But file
> > > server does not have enough information to do so. 
> > > 
> > > Initially I thought of sending CAP_FSETID information to server but
> > > then I realized, it is just one of the pieces. What about all the
> > > groups caller is a member of. If this has to work correctly, then
> > > all the information will have to be sent to virtiofsd somehow. Just
> > > sending CAP_FSETID information required adding V2 of fuse_setxattr_in
> > > because we don't have any space for sending extra information.
> > > 
> > > https://github.com/rhvgoyal/linux/commit/681cf5bdbba9c965c3dbd4337c16e9b17f27debe
> > > 
> > > Also this approach will not work with idmapped mounts because server
> > > does not have information about idmapped mappings.
> > > 
> > > So I started to look at the approach of sending file mode updates
> > > using SETATTR. As filesystems like 9pfs and ceph are doing. This
> > > seems simpler approach. Though it has its issues too.
> > > 
> > > - File mode update and setxattr(system.posix_acl_access) are not atomic.
> > 
> > After reviewing (and testing) the patch, the only comment I have is that
> > we should at least pr_warn() an eventual failure in setxattr().  But f
> > that operation fails at that point, probably something went wrong on the
> > other side
> 
> Hi Luis,
> 
> If setxattr failed, user will get the error. 
> 
> I guess pr_warn() could help with figuring out that there was a side affect
> of failed failed setxattr operation. (mode changed). I will add something.
> 
> > and the kernel is unlikely to be able to revert the mode
> > changes anyway.
> 
> Interestingly ceph code seems to revert mode changes if setxattr fails.
> I think for now I am happy with just a pr_warn().

Yeah, ceph does that.  And I *should* know it ;-)

Anyway, to mimic ceph's behaviour should be easy enough, although I guess
it's just a best-effort thing.

Cheers,
--
Luís


> > 
> > (And a nit: your patch seems to require some whitespaces clean-up.)
> 
> Will check it and fix it and post V2.
> 
> Thanks
> Vivek
> 
> > 
> > Cheers,
> > --
> > Luís
> > 
> > 
> > > None of the approaches seem very clean to me. But sending SETATTR
> > > explicitly seems to be lesser of two evils to me at this point of time.
> > > Hence I am proposing this patch. 
> > > 
> > > I have run fstests acl tests and they pass. (./check -g acl).
> > > 
> > > Corresponding virtiofsd patches are here.
> > > 
> > > https://github.com/rhvgoyal/qemu/commits/acl-sgid-setattr
> > > 
> > > What do you think.
> > > 
> > > Vivek Goyal (1):
> > >   fuse: Add a mode where fuse client sends mode changes on ACL change
> > > 
> > >  fs/fuse/acl.c             | 54 ++++++++++++++++++++++++++++++++++++---
> > >  fs/fuse/dir.c             | 11 ++++----
> > >  fs/fuse/fuse_i.h          |  9 ++++++-
> > >  fs/fuse/inode.c           |  4 ++-
> > >  include/uapi/linux/fuse.h |  5 ++++
> > >  5 files changed, 71 insertions(+), 12 deletions(-)
> > > 
> > > -- 
> > > 2.25.4
> > > 
> > 
>