[PATCH v2 0/2] fuse: Fix clearing SGID when access ACL is set

[PATCH v2 0/2] fuse: Fix clearing SGID when access ACL is set

[Vivek Goyal]

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.

I have staged corresponding virtiofsd patches here.

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

With these patches applied "./check -g acl" passes now on virtiofs.

Thanks
Vivek

Vivek Goyal (2):
  fuse: Add support for FUSE_SETXATTR_V2
  fuse: Add a flag FUSE_SETXATTR_ACL_KILL_SGID to kill SGID

 fs/fuse/acl.c             |  8 +++++++-
 fs/fuse/fuse_i.h          |  5 ++++-
 fs/fuse/inode.c           |  4 +++-
 fs/fuse/xattr.c           | 21 +++++++++++++++------
 include/uapi/linux/fuse.h | 17 +++++++++++++++++
 5 files changed, 46 insertions(+), 9 deletions(-)

-- 
2.25.4

[PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2

[Vivek Goyal]

Fuse client needs to send additional information to file server when
it calls SETXATTR(system.posix_acl_access). Right now there is no extra
space in fuse_setxattr_in. So introduce a v2 of the structure which has
more space in it and can be used to send extra flags.

"struct fuse_setxattr_in_v2" is only used if file server opts-in for it using
flag FUSE_SETXATTR_V2 during feature negotiations.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/acl.c             |  2 +-
 fs/fuse/fuse_i.h          |  5 ++++-
 fs/fuse/inode.c           |  4 +++-
 fs/fuse/xattr.c           | 21 +++++++++++++++------
 include/uapi/linux/fuse.h | 10 ++++++++++
 5 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
index e9c0f916349d..d31260a139d4 100644
--- a/fs/fuse/acl.c
+++ b/fs/fuse/acl.c
@@ -94,7 +94,7 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
 			return ret;
 		}
 
-		ret = fuse_setxattr(inode, name, value, size, 0);
+		ret = fuse_setxattr(inode, name, value, size, 0, 0);
 		kfree(value);
 	} else {
 		ret = fuse_removexattr(inode, name);
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 63d97a15ffde..d00bf0b9a38c 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -668,6 +668,9 @@ struct fuse_conn {
 	/** Is setxattr not implemented by fs? */
 	unsigned no_setxattr:1;
 
+	/** Does file server support setxattr_v2 */
+	unsigned setxattr_v2:1;
+
 	/** Is getxattr not implemented by fs? */
 	unsigned no_getxattr:1;
 
@@ -1170,7 +1173,7 @@ void fuse_unlock_inode(struct inode *inode, bool locked);
 bool fuse_lock_inode(struct inode *inode);
 
 int fuse_setxattr(struct inode *inode, const char *name, const void *value,
-		  size_t size, int flags);
+		  size_t size, int flags, unsigned extra_flags);
 ssize_t fuse_getxattr(struct inode *inode, const char *name, void *value,
 		      size_t size);
 ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size);
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index b0e18b470e91..1c726df13f80 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_SETXATTR_V2)
+				fc->setxattr_v2 = 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_SETXATTR_V2;
 #ifdef CONFIG_FUSE_DAX
 	if (fm->fc->dax)
 		ia->in.flags |= FUSE_MAP_ALIGNMENT;
diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c
index 1a7d7ace54e1..f2aae72653dc 100644
--- a/fs/fuse/xattr.c
+++ b/fs/fuse/xattr.c
@@ -12,24 +12,33 @@
 #include <linux/posix_acl_xattr.h>
 
 int fuse_setxattr(struct inode *inode, const char *name, const void *value,
-		  size_t size, int flags)
+		  size_t size, int flags, unsigned extra_flags)
 {
 	struct fuse_mount *fm = get_fuse_mount(inode);
 	FUSE_ARGS(args);
 	struct fuse_setxattr_in inarg;
+	struct fuse_setxattr_in_v2 inarg_v2;
+	bool setxattr_v2 = fm->fc->setxattr_v2;
 	int err;
 
 	if (fm->fc->no_setxattr)
 		return -EOPNOTSUPP;
 
 	memset(&inarg, 0, sizeof(inarg));
-	inarg.size = size;
-	inarg.flags = flags;
+	memset(&inarg_v2, 0, sizeof(inarg_v2));
+	if (setxattr_v2) {
+		inarg_v2.size = size;
+		inarg_v2.flags = flags;
+		inarg_v2.setxattr_flags = extra_flags;
+	} else {
+		inarg.size = size;
+		inarg.flags = flags;
+	}
 	args.opcode = FUSE_SETXATTR;
 	args.nodeid = get_node_id(inode);
 	args.in_numargs = 3;
-	args.in_args[0].size = sizeof(inarg);
-	args.in_args[0].value = &inarg;
+	args.in_args[0].size = setxattr_v2 ? sizeof(inarg_v2) : sizeof(inarg);
+	args.in_args[0].value = setxattr_v2 ? &inarg_v2 : (void *)&inarg;
 	args.in_args[1].size = strlen(name) + 1;
 	args.in_args[1].value = name;
 	args.in_args[2].size = size;
@@ -199,7 +208,7 @@ static int fuse_xattr_set(const struct xattr_handler *handler,
 	if (!value)
 		return fuse_removexattr(inode, name);
 
-	return fuse_setxattr(inode, name, value, size, flags);
+	return fuse_setxattr(inode, name, value, size, flags, 0);
 }
 
 static bool no_xattr_list(struct dentry *dentry)
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 54442612c48b..1bb555c1c117 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_SETXATTR_V2
  */
 
 #ifndef _LINUX_FUSE_H
@@ -330,6 +331,7 @@ 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_SETXATTR_V2:	Does file server support V2 of struct fuse_setxattr_in
  */
 #define FUSE_ASYNC_READ		(1 << 0)
 #define FUSE_POSIX_LOCKS	(1 << 1)
@@ -360,6 +362,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_SETXATTR_V2	(1 << 29)
 
 /**
  * CUSE INIT request/reply flags
@@ -686,6 +689,13 @@ struct fuse_setxattr_in {
 	uint32_t	flags;
 };
 
+struct fuse_setxattr_in_v2 {
+	uint32_t	size;
+	uint32_t	flags;
+	uint32_t	setxattr_flags;
+	uint32_t	padding;
+};
+
 struct fuse_getxattr_in {
 	uint32_t	size;
 	uint32_t	padding;
-- 
2.25.4

Re: [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2

[Luis Henriques]

On Thu, Mar 25, 2021 at 11:18:22AM -0400, Vivek Goyal wrote:
> Fuse client needs to send additional information to file server when
> it calls SETXATTR(system.posix_acl_access). Right now there is no extra
> space in fuse_setxattr_in. So introduce a v2 of the structure which has
> more space in it and can be used to send extra flags.
> 
> "struct fuse_setxattr_in_v2" is only used if file server opts-in for it using
> flag FUSE_SETXATTR_V2 during feature negotiations.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/acl.c             |  2 +-
>  fs/fuse/fuse_i.h          |  5 ++++-
>  fs/fuse/inode.c           |  4 +++-
>  fs/fuse/xattr.c           | 21 +++++++++++++++------
>  include/uapi/linux/fuse.h | 10 ++++++++++
>  5 files changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
> index e9c0f916349d..d31260a139d4 100644
> --- a/fs/fuse/acl.c
> +++ b/fs/fuse/acl.c
> @@ -94,7 +94,7 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
>  			return ret;
>  		}
>  
> -		ret = fuse_setxattr(inode, name, value, size, 0);
> +		ret = fuse_setxattr(inode, name, value, size, 0, 0);
>  		kfree(value);
>  	} else {
>  		ret = fuse_removexattr(inode, name);
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 63d97a15ffde..d00bf0b9a38c 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -668,6 +668,9 @@ struct fuse_conn {
>  	/** Is setxattr not implemented by fs? */
>  	unsigned no_setxattr:1;
>  
> +	/** Does file server support setxattr_v2 */
> +	unsigned setxattr_v2:1;
> +

Minor (pedantic!) comment: most of the fields here start with 'no_*', so
maybe it's worth setting the logic to use 'no_setxattr_v2' instead?

Cheers,
--
Luís


>  	/** Is getxattr not implemented by fs? */
>  	unsigned no_getxattr:1;
>  
> @@ -1170,7 +1173,7 @@ void fuse_unlock_inode(struct inode *inode, bool locked);
>  bool fuse_lock_inode(struct inode *inode);
>  
>  int fuse_setxattr(struct inode *inode, const char *name, const void *value,
> -		  size_t size, int flags);
> +		  size_t size, int flags, unsigned extra_flags);
>  ssize_t fuse_getxattr(struct inode *inode, const char *name, void *value,
>  		      size_t size);
>  ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size);
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index b0e18b470e91..1c726df13f80 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_SETXATTR_V2)
> +				fc->setxattr_v2 = 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_SETXATTR_V2;
>  #ifdef CONFIG_FUSE_DAX
>  	if (fm->fc->dax)
>  		ia->in.flags |= FUSE_MAP_ALIGNMENT;
> diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c
> index 1a7d7ace54e1..f2aae72653dc 100644
> --- a/fs/fuse/xattr.c
> +++ b/fs/fuse/xattr.c
> @@ -12,24 +12,33 @@
>  #include <linux/posix_acl_xattr.h>
>  
>  int fuse_setxattr(struct inode *inode, const char *name, const void *value,
> -		  size_t size, int flags)
> +		  size_t size, int flags, unsigned extra_flags)
>  {
>  	struct fuse_mount *fm = get_fuse_mount(inode);
>  	FUSE_ARGS(args);
>  	struct fuse_setxattr_in inarg;
> +	struct fuse_setxattr_in_v2 inarg_v2;
> +	bool setxattr_v2 = fm->fc->setxattr_v2;
>  	int err;
>  
>  	if (fm->fc->no_setxattr)
>  		return -EOPNOTSUPP;
>  
>  	memset(&inarg, 0, sizeof(inarg));
> -	inarg.size = size;
> -	inarg.flags = flags;
> +	memset(&inarg_v2, 0, sizeof(inarg_v2));
> +	if (setxattr_v2) {
> +		inarg_v2.size = size;
> +		inarg_v2.flags = flags;
> +		inarg_v2.setxattr_flags = extra_flags;
> +	} else {
> +		inarg.size = size;
> +		inarg.flags = flags;
> +	}
>  	args.opcode = FUSE_SETXATTR;
>  	args.nodeid = get_node_id(inode);
>  	args.in_numargs = 3;
> -	args.in_args[0].size = sizeof(inarg);
> -	args.in_args[0].value = &inarg;
> +	args.in_args[0].size = setxattr_v2 ? sizeof(inarg_v2) : sizeof(inarg);
> +	args.in_args[0].value = setxattr_v2 ? &inarg_v2 : (void *)&inarg;
>  	args.in_args[1].size = strlen(name) + 1;
>  	args.in_args[1].value = name;
>  	args.in_args[2].size = size;
> @@ -199,7 +208,7 @@ static int fuse_xattr_set(const struct xattr_handler *handler,
>  	if (!value)
>  		return fuse_removexattr(inode, name);
>  
> -	return fuse_setxattr(inode, name, value, size, flags);
> +	return fuse_setxattr(inode, name, value, size, flags, 0);
>  }
>  
>  static bool no_xattr_list(struct dentry *dentry)
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 54442612c48b..1bb555c1c117 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_SETXATTR_V2
>   */
>  
>  #ifndef _LINUX_FUSE_H
> @@ -330,6 +331,7 @@ 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_SETXATTR_V2:	Does file server support V2 of struct fuse_setxattr_in
>   */
>  #define FUSE_ASYNC_READ		(1 << 0)
>  #define FUSE_POSIX_LOCKS	(1 << 1)
> @@ -360,6 +362,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_SETXATTR_V2	(1 << 29)
>  
>  /**
>   * CUSE INIT request/reply flags
> @@ -686,6 +689,13 @@ struct fuse_setxattr_in {
>  	uint32_t	flags;
>  };
>  
> +struct fuse_setxattr_in_v2 {
> +	uint32_t	size;
> +	uint32_t	flags;
> +	uint32_t	setxattr_flags;
> +	uint32_t	padding;
> +};
> +
>  struct fuse_getxattr_in {
>  	uint32_t	size;
>  	uint32_t	padding;
> -- 
> 2.25.4
> 

Re: [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2

[Vivek Goyal]

On Mon, Mar 29, 2021 at 03:50:37PM +0100, Luis Henriques wrote:
> On Thu, Mar 25, 2021 at 11:18:22AM -0400, Vivek Goyal wrote:
> > Fuse client needs to send additional information to file server when
> > it calls SETXATTR(system.posix_acl_access). Right now there is no extra
> > space in fuse_setxattr_in. So introduce a v2 of the structure which has
> > more space in it and can be used to send extra flags.
> > 
> > "struct fuse_setxattr_in_v2" is only used if file server opts-in for it using
> > flag FUSE_SETXATTR_V2 during feature negotiations.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/fuse/acl.c             |  2 +-
> >  fs/fuse/fuse_i.h          |  5 ++++-
> >  fs/fuse/inode.c           |  4 +++-
> >  fs/fuse/xattr.c           | 21 +++++++++++++++------
> >  include/uapi/linux/fuse.h | 10 ++++++++++
> >  5 files changed, 33 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
> > index e9c0f916349d..d31260a139d4 100644
> > --- a/fs/fuse/acl.c
> > +++ b/fs/fuse/acl.c
> > @@ -94,7 +94,7 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
> >  			return ret;
> >  		}
> >  
> > -		ret = fuse_setxattr(inode, name, value, size, 0);
> > +		ret = fuse_setxattr(inode, name, value, size, 0, 0);
> >  		kfree(value);
> >  	} else {
> >  		ret = fuse_removexattr(inode, name);
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 63d97a15ffde..d00bf0b9a38c 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -668,6 +668,9 @@ struct fuse_conn {
> >  	/** Is setxattr not implemented by fs? */
> >  	unsigned no_setxattr:1;
> >  
> > +	/** Does file server support setxattr_v2 */
> > +	unsigned setxattr_v2:1;
> > +
> 
> Minor (pedantic!) comment: most of the fields here start with 'no_*', so
> maybe it's worth setting the logic to use 'no_setxattr_v2' instead?

Hi Luis,

"setxattr_v2" kind of makes more sense to me because it is disabled
by default untile and unless client opts in. If I use no_setxattr_v2,
then it means by default I will have to initialize it to 1. Right
now, following automatically takes care of it.

fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL);

Also, there are other examples which don't use "no_" prefix.

auto_inval_data, explicit_inval_data, do_readdirplus, readdirplus_auto, 
async_dio..... and list goes on.

Vivek

Re: [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2

[Luis Henriques]

On Thu, Mar 25, 2021 at 11:18:22AM -0400, Vivek Goyal wrote:
> Fuse client needs to send additional information to file server when
> it calls SETXATTR(system.posix_acl_access). Right now there is no extra
> space in fuse_setxattr_in. So introduce a v2 of the structure which has
> more space in it and can be used to send extra flags.
> 
> "struct fuse_setxattr_in_v2" is only used if file server opts-in for it using
> flag FUSE_SETXATTR_V2 during feature negotiations.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/acl.c             |  2 +-
>  fs/fuse/fuse_i.h          |  5 ++++-
>  fs/fuse/inode.c           |  4 +++-
>  fs/fuse/xattr.c           | 21 +++++++++++++++------
>  include/uapi/linux/fuse.h | 10 ++++++++++
>  5 files changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
> index e9c0f916349d..d31260a139d4 100644
> --- a/fs/fuse/acl.c
> +++ b/fs/fuse/acl.c
> @@ -94,7 +94,7 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
>  			return ret;
>  		}
>  
> -		ret = fuse_setxattr(inode, name, value, size, 0);
> +		ret = fuse_setxattr(inode, name, value, size, 0, 0);
>  		kfree(value);
>  	} else {
>  		ret = fuse_removexattr(inode, name);
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 63d97a15ffde..d00bf0b9a38c 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -668,6 +668,9 @@ struct fuse_conn {
>  	/** Is setxattr not implemented by fs? */
>  	unsigned no_setxattr:1;
>  
> +	/** Does file server support setxattr_v2 */
> +	unsigned setxattr_v2:1;
> +
>  	/** Is getxattr not implemented by fs? */
>  	unsigned no_getxattr:1;
>  
> @@ -1170,7 +1173,7 @@ void fuse_unlock_inode(struct inode *inode, bool locked);
>  bool fuse_lock_inode(struct inode *inode);
>  
>  int fuse_setxattr(struct inode *inode, const char *name, const void *value,
> -		  size_t size, int flags);
> +		  size_t size, int flags, unsigned extra_flags);
>  ssize_t fuse_getxattr(struct inode *inode, const char *name, void *value,
>  		      size_t size);
>  ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size);
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index b0e18b470e91..1c726df13f80 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_SETXATTR_V2)
> +				fc->setxattr_v2 = 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_SETXATTR_V2;
>  #ifdef CONFIG_FUSE_DAX
>  	if (fm->fc->dax)
>  		ia->in.flags |= FUSE_MAP_ALIGNMENT;
> diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c
> index 1a7d7ace54e1..f2aae72653dc 100644
> --- a/fs/fuse/xattr.c
> +++ b/fs/fuse/xattr.c
> @@ -12,24 +12,33 @@
>  #include <linux/posix_acl_xattr.h>
>  
>  int fuse_setxattr(struct inode *inode, const char *name, const void *value,
> -		  size_t size, int flags)
> +		  size_t size, int flags, unsigned extra_flags)
>  {
>  	struct fuse_mount *fm = get_fuse_mount(inode);
>  	FUSE_ARGS(args);
>  	struct fuse_setxattr_in inarg;
> +	struct fuse_setxattr_in_v2 inarg_v2;
> +	bool setxattr_v2 = fm->fc->setxattr_v2;
>  	int err;
>  
>  	if (fm->fc->no_setxattr)
>  		return -EOPNOTSUPP;
>  
>  	memset(&inarg, 0, sizeof(inarg));
> -	inarg.size = size;
> -	inarg.flags = flags;
> +	memset(&inarg_v2, 0, sizeof(inarg_v2));
> +	if (setxattr_v2) {
> +		inarg_v2.size = size;
> +		inarg_v2.flags = flags;
> +		inarg_v2.setxattr_flags = extra_flags;
> +	} else {
> +		inarg.size = size;
> +		inarg.flags = flags;
> +	}
>  	args.opcode = FUSE_SETXATTR;
>  	args.nodeid = get_node_id(inode);
>  	args.in_numargs = 3;
> -	args.in_args[0].size = sizeof(inarg);
> -	args.in_args[0].value = &inarg;
> +	args.in_args[0].size = setxattr_v2 ? sizeof(inarg_v2) : sizeof(inarg);
> +	args.in_args[0].value = setxattr_v2 ? &inarg_v2 : (void *)&inarg;

And yet another minor:

It's a bit awkward to have to cast '&inarg' to 'void *' just because
you're using the ternary operator.  Why not use an 'if' statement instead
for initializing .size and .value?

Cheers,
--
Luís

>  	args.in_args[1].size = strlen(name) + 1;
>  	args.in_args[1].value = name;
>  	args.in_args[2].size = size;
> @@ -199,7 +208,7 @@ static int fuse_xattr_set(const struct xattr_handler *handler,
>  	if (!value)
>  		return fuse_removexattr(inode, name);
>  
> -	return fuse_setxattr(inode, name, value, size, flags);
> +	return fuse_setxattr(inode, name, value, size, flags, 0);
>  }
>  
>  static bool no_xattr_list(struct dentry *dentry)
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 54442612c48b..1bb555c1c117 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_SETXATTR_V2
>   */
>  
>  #ifndef _LINUX_FUSE_H
> @@ -330,6 +331,7 @@ 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_SETXATTR_V2:	Does file server support V2 of struct fuse_setxattr_in
>   */
>  #define FUSE_ASYNC_READ		(1 << 0)
>  #define FUSE_POSIX_LOCKS	(1 << 1)
> @@ -360,6 +362,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_SETXATTR_V2	(1 << 29)
>  
>  /**
>   * CUSE INIT request/reply flags
> @@ -686,6 +689,13 @@ struct fuse_setxattr_in {
>  	uint32_t	flags;
>  };
>  
> +struct fuse_setxattr_in_v2 {
> +	uint32_t	size;
> +	uint32_t	flags;
> +	uint32_t	setxattr_flags;
> +	uint32_t	padding;
> +};
> +
>  struct fuse_getxattr_in {
>  	uint32_t	size;
>  	uint32_t	padding;
> -- 
> 2.25.4
> 

Re: [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2

[Vivek Goyal]

On Mon, Mar 29, 2021 at 03:54:03PM +0100, Luis Henriques wrote:
> On Thu, Mar 25, 2021 at 11:18:22AM -0400, Vivek Goyal wrote:
> > Fuse client needs to send additional information to file server when
> > it calls SETXATTR(system.posix_acl_access). Right now there is no extra
> > space in fuse_setxattr_in. So introduce a v2 of the structure which has
> > more space in it and can be used to send extra flags.
> > 
> > "struct fuse_setxattr_in_v2" is only used if file server opts-in for it using
> > flag FUSE_SETXATTR_V2 during feature negotiations.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/fuse/acl.c             |  2 +-
> >  fs/fuse/fuse_i.h          |  5 ++++-
> >  fs/fuse/inode.c           |  4 +++-
> >  fs/fuse/xattr.c           | 21 +++++++++++++++------
> >  include/uapi/linux/fuse.h | 10 ++++++++++
> >  5 files changed, 33 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
> > index e9c0f916349d..d31260a139d4 100644
> > --- a/fs/fuse/acl.c
> > +++ b/fs/fuse/acl.c
> > @@ -94,7 +94,7 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
> >  			return ret;
> >  		}
> >  
> > -		ret = fuse_setxattr(inode, name, value, size, 0);
> > +		ret = fuse_setxattr(inode, name, value, size, 0, 0);
> >  		kfree(value);
> >  	} else {
> >  		ret = fuse_removexattr(inode, name);
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 63d97a15ffde..d00bf0b9a38c 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -668,6 +668,9 @@ struct fuse_conn {
> >  	/** Is setxattr not implemented by fs? */
> >  	unsigned no_setxattr:1;
> >  
> > +	/** Does file server support setxattr_v2 */
> > +	unsigned setxattr_v2:1;
> > +
> >  	/** Is getxattr not implemented by fs? */
> >  	unsigned no_getxattr:1;
> >  
> > @@ -1170,7 +1173,7 @@ void fuse_unlock_inode(struct inode *inode, bool locked);
> >  bool fuse_lock_inode(struct inode *inode);
> >  
> >  int fuse_setxattr(struct inode *inode, const char *name, const void *value,
> > -		  size_t size, int flags);
> > +		  size_t size, int flags, unsigned extra_flags);
> >  ssize_t fuse_getxattr(struct inode *inode, const char *name, void *value,
> >  		      size_t size);
> >  ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size);
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index b0e18b470e91..1c726df13f80 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_SETXATTR_V2)
> > +				fc->setxattr_v2 = 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_SETXATTR_V2;
> >  #ifdef CONFIG_FUSE_DAX
> >  	if (fm->fc->dax)
> >  		ia->in.flags |= FUSE_MAP_ALIGNMENT;
> > diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c
> > index 1a7d7ace54e1..f2aae72653dc 100644
> > --- a/fs/fuse/xattr.c
> > +++ b/fs/fuse/xattr.c
> > @@ -12,24 +12,33 @@
> >  #include <linux/posix_acl_xattr.h>
> >  
> >  int fuse_setxattr(struct inode *inode, const char *name, const void *value,
> > -		  size_t size, int flags)
> > +		  size_t size, int flags, unsigned extra_flags)
> >  {
> >  	struct fuse_mount *fm = get_fuse_mount(inode);
> >  	FUSE_ARGS(args);
> >  	struct fuse_setxattr_in inarg;
> > +	struct fuse_setxattr_in_v2 inarg_v2;
> > +	bool setxattr_v2 = fm->fc->setxattr_v2;
> >  	int err;
> >  
> >  	if (fm->fc->no_setxattr)
> >  		return -EOPNOTSUPP;
> >  
> >  	memset(&inarg, 0, sizeof(inarg));
> > -	inarg.size = size;
> > -	inarg.flags = flags;
> > +	memset(&inarg_v2, 0, sizeof(inarg_v2));
> > +	if (setxattr_v2) {
> > +		inarg_v2.size = size;
> > +		inarg_v2.flags = flags;
> > +		inarg_v2.setxattr_flags = extra_flags;
> > +	} else {
> > +		inarg.size = size;
> > +		inarg.flags = flags;
> > +	}
> >  	args.opcode = FUSE_SETXATTR;
> >  	args.nodeid = get_node_id(inode);
> >  	args.in_numargs = 3;
> > -	args.in_args[0].size = sizeof(inarg);
> > -	args.in_args[0].value = &inarg;
> > +	args.in_args[0].size = setxattr_v2 ? sizeof(inarg_v2) : sizeof(inarg);
> > +	args.in_args[0].value = setxattr_v2 ? &inarg_v2 : (void *)&inarg;
> 
> And yet another minor:
> 
> It's a bit awkward to have to cast '&inarg' to 'void *' just because
> you're using the ternary operator.  Why not use an 'if' statement instead
> for initializing .size and .value?

Yes, I had to use (void *), otherwise compiler was complaining about
returning different types of pointers. Interesting that compiler
expects to return same type of pointer.

I think I am fine with this as well as adding explicit if statement. I
guess just a matter of taste. 

Miklos, what do you think? If you also prefer if statement instead,
I will make changes and post again.

Vivek

> 
> Cheers,
> --
> Luís
> 
> >  	args.in_args[1].size = strlen(name) + 1;
> >  	args.in_args[1].value = name;
> >  	args.in_args[2].size = size;
> > @@ -199,7 +208,7 @@ static int fuse_xattr_set(const struct xattr_handler *handler,
> >  	if (!value)
> >  		return fuse_removexattr(inode, name);
> >  
> > -	return fuse_setxattr(inode, name, value, size, flags);
> > +	return fuse_setxattr(inode, name, value, size, flags, 0);
> >  }
> >  
> >  static bool no_xattr_list(struct dentry *dentry)
> > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> > index 54442612c48b..1bb555c1c117 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_SETXATTR_V2
> >   */
> >  
> >  #ifndef _LINUX_FUSE_H
> > @@ -330,6 +331,7 @@ 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_SETXATTR_V2:	Does file server support V2 of struct fuse_setxattr_in
> >   */
> >  #define FUSE_ASYNC_READ		(1 << 0)
> >  #define FUSE_POSIX_LOCKS	(1 << 1)
> > @@ -360,6 +362,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_SETXATTR_V2	(1 << 29)
> >  
> >  /**
> >   * CUSE INIT request/reply flags
> > @@ -686,6 +689,13 @@ struct fuse_setxattr_in {
> >  	uint32_t	flags;
> >  };
> >  
> > +struct fuse_setxattr_in_v2 {
> > +	uint32_t	size;
> > +	uint32_t	flags;
> > +	uint32_t	setxattr_flags;
> > +	uint32_t	padding;
> > +};
> > +
> >  struct fuse_getxattr_in {
> >  	uint32_t	size;
> >  	uint32_t	padding;
> > -- 
> > 2.25.4
> > 
> 

Re: [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2

[Luis Henriques]

Vivek Goyal <vgoyal@redhat.com> writes:

> On Mon, Mar 29, 2021 at 03:54:03PM +0100, Luis Henriques wrote:
>> On Thu, Mar 25, 2021 at 11:18:22AM -0400, Vivek Goyal wrote:
>> > Fuse client needs to send additional information to file server when
>> > it calls SETXATTR(system.posix_acl_access). Right now there is no extra
>> > space in fuse_setxattr_in. So introduce a v2 of the structure which has
>> > more space in it and can be used to send extra flags.
>> > 
>> > "struct fuse_setxattr_in_v2" is only used if file server opts-in for it using
>> > flag FUSE_SETXATTR_V2 during feature negotiations.
>> > 
>> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> > ---
>> >  fs/fuse/acl.c             |  2 +-
>> >  fs/fuse/fuse_i.h          |  5 ++++-
>> >  fs/fuse/inode.c           |  4 +++-
>> >  fs/fuse/xattr.c           | 21 +++++++++++++++------
>> >  include/uapi/linux/fuse.h | 10 ++++++++++
>> >  5 files changed, 33 insertions(+), 9 deletions(-)
>> > 
>> > diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
>> > index e9c0f916349d..d31260a139d4 100644
>> > --- a/fs/fuse/acl.c
>> > +++ b/fs/fuse/acl.c
>> > @@ -94,7 +94,7 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
>> >  			return ret;
>> >  		}
>> >  
>> > -		ret = fuse_setxattr(inode, name, value, size, 0);
>> > +		ret = fuse_setxattr(inode, name, value, size, 0, 0);
>> >  		kfree(value);
>> >  	} else {
>> >  		ret = fuse_removexattr(inode, name);
>> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>> > index 63d97a15ffde..d00bf0b9a38c 100644
>> > --- a/fs/fuse/fuse_i.h
>> > +++ b/fs/fuse/fuse_i.h
>> > @@ -668,6 +668,9 @@ struct fuse_conn {
>> >  	/** Is setxattr not implemented by fs? */
>> >  	unsigned no_setxattr:1;
>> >  
>> > +	/** Does file server support setxattr_v2 */
>> > +	unsigned setxattr_v2:1;
>> > +
>> >  	/** Is getxattr not implemented by fs? */
>> >  	unsigned no_getxattr:1;
>> >  
>> > @@ -1170,7 +1173,7 @@ void fuse_unlock_inode(struct inode *inode, bool locked);
>> >  bool fuse_lock_inode(struct inode *inode);
>> >  
>> >  int fuse_setxattr(struct inode *inode, const char *name, const void *value,
>> > -		  size_t size, int flags);
>> > +		  size_t size, int flags, unsigned extra_flags);
>> >  ssize_t fuse_getxattr(struct inode *inode, const char *name, void *value,
>> >  		      size_t size);
>> >  ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size);
>> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>> > index b0e18b470e91..1c726df13f80 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_SETXATTR_V2)
>> > +				fc->setxattr_v2 = 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_SETXATTR_V2;
>> >  #ifdef CONFIG_FUSE_DAX
>> >  	if (fm->fc->dax)
>> >  		ia->in.flags |= FUSE_MAP_ALIGNMENT;
>> > diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c
>> > index 1a7d7ace54e1..f2aae72653dc 100644
>> > --- a/fs/fuse/xattr.c
>> > +++ b/fs/fuse/xattr.c
>> > @@ -12,24 +12,33 @@
>> >  #include <linux/posix_acl_xattr.h>
>> >  
>> >  int fuse_setxattr(struct inode *inode, const char *name, const void *value,
>> > -		  size_t size, int flags)
>> > +		  size_t size, int flags, unsigned extra_flags)
>> >  {
>> >  	struct fuse_mount *fm = get_fuse_mount(inode);
>> >  	FUSE_ARGS(args);
>> >  	struct fuse_setxattr_in inarg;
>> > +	struct fuse_setxattr_in_v2 inarg_v2;
>> > +	bool setxattr_v2 = fm->fc->setxattr_v2;
>> >  	int err;
>> >  
>> >  	if (fm->fc->no_setxattr)
>> >  		return -EOPNOTSUPP;
>> >  
>> >  	memset(&inarg, 0, sizeof(inarg));
>> > -	inarg.size = size;
>> > -	inarg.flags = flags;
>> > +	memset(&inarg_v2, 0, sizeof(inarg_v2));
>> > +	if (setxattr_v2) {
>> > +		inarg_v2.size = size;
>> > +		inarg_v2.flags = flags;
>> > +		inarg_v2.setxattr_flags = extra_flags;
>> > +	} else {
>> > +		inarg.size = size;
>> > +		inarg.flags = flags;
>> > +	}
>> >  	args.opcode = FUSE_SETXATTR;
>> >  	args.nodeid = get_node_id(inode);
>> >  	args.in_numargs = 3;
>> > -	args.in_args[0].size = sizeof(inarg);
>> > -	args.in_args[0].value = &inarg;
>> > +	args.in_args[0].size = setxattr_v2 ? sizeof(inarg_v2) : sizeof(inarg);
>> > +	args.in_args[0].value = setxattr_v2 ? &inarg_v2 : (void *)&inarg;
>> 
>> And yet another minor:
>> 
>> It's a bit awkward to have to cast '&inarg' to 'void *' just because
>> you're using the ternary operator.  Why not use an 'if' statement instead
>> for initializing .size and .value?
>
> Yes, I had to use (void *), otherwise compiler was complaining about
> returning different types of pointers. Interesting that compiler
> expects to return same type of pointer.

IIRC, K&R (which I unfortunately don't have at hand right now) says that
the types of both expressions need to match, so probably a different
compiler would show the same warning.

Cheers,
-- 
Luis

> I think I am fine with this as well as adding explicit if statement. I
> guess just a matter of taste. 
>
> Miklos, what do you think? If you also prefer if statement instead,
> I will make changes and post again.
>
> Vivek

[PATCH v2 2/2] fuse: Add a flag FUSE_SETXATTR_ACL_KILL_SGID to kill SGID

[Vivek Goyal]

When posix access ACL is set, it can have an effect on file mode and
it can also need to clear SGID if.

- None of caller's group/supplementary groups match file owner group.
AND
- Caller is not priviliged (No CAP_FSETID).

As of now fuser server is responsible for changing the file mode as well. But
it does not know whether to clear SGID or not.

So add a flag FUSE_SETXATTR_ACL_KILL_SGID and send this info with
SETXATTR to let file server know that sgid needs to be cleared as well.

Reported-by: Luis Henriques <lhenriques@suse.de>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/acl.c             | 8 +++++++-
 include/uapi/linux/fuse.h | 7 +++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
index d31260a139d4..8819ceb0a4e5 100644
--- a/fs/fuse/acl.c
+++ b/fs/fuse/acl.c
@@ -71,6 +71,7 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
 		return -EINVAL;
 
 	if (acl) {
+		unsigned extra_flags = 0;
 		/*
 		 * Fuse userspace is responsible for updating access
 		 * permissions in the inode, if needed. fuse_setxattr
@@ -94,7 +95,12 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
 			return ret;
 		}
 
-		ret = fuse_setxattr(inode, name, value, size, 0, 0);
+		if (fc->setxattr_v2 &&
+		    !in_group_p(i_gid_into_mnt(&init_user_ns, inode)) &&
+		    !capable_wrt_inode_uidgid(&init_user_ns, inode, CAP_FSETID))
+			extra_flags |= FUSE_SETXATTR_ACL_KILL_SGID;
+
+		ret = fuse_setxattr(inode, name, value, size, 0, extra_flags);
 		kfree(value);
 	} else {
 		ret = fuse_removexattr(inode, name);
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 1bb555c1c117..08c11a7beaa7 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -180,6 +180,7 @@
  *  - add FUSE_HANDLE_KILLPRIV_V2, FUSE_WRITE_KILL_SUIDGID, FATTR_KILL_SUIDGID
  *  - add FUSE_OPEN_KILL_SUIDGID
  *  - add FUSE_SETXATTR_V2
+ *  - add FUSE_SETXATTR_ACL_KILL_SGID
  */
 
 #ifndef _LINUX_FUSE_H
@@ -454,6 +455,12 @@ struct fuse_file_lock {
  */
 #define FUSE_OPEN_KILL_SUIDGID	(1 << 0)
 
+/**
+ * setxattr flags
+ * FUSE_SETXATTR_ACL_KILL_SGID: Clear SGID when system.posix_acl_access is set
+ */
+#define FUSE_SETXATTR_ACL_KILL_SGID	(1 << 0)
+
 enum fuse_opcode {
 	FUSE_LOOKUP		= 1,
 	FUSE_FORGET		= 2,  /* no reply */
-- 
2.25.4

Re: [Virtio-fs] [PATCH v2 0/2] fuse: Fix clearing SGID when access ACL is set

[Vivek Goyal]

Hi Miklos,

Ping for this patch series.

Vivek

On Thu, Mar 25, 2021 at 11:18:21AM -0400, Vivek Goyal 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.
> 
> I have staged corresponding virtiofsd patches here.
> 
> https://github.com/rhvgoyal/qemu/commits/acl-sgid-setxattr-flag
> 
> With these patches applied "./check -g acl" passes now on virtiofs.
> 
> Thanks
> Vivek
> 
> Vivek Goyal (2):
>   fuse: Add support for FUSE_SETXATTR_V2
>   fuse: Add a flag FUSE_SETXATTR_ACL_KILL_SGID to kill SGID
> 
>  fs/fuse/acl.c             |  8 +++++++-
>  fs/fuse/fuse_i.h          |  5 ++++-
>  fs/fuse/inode.c           |  4 +++-
>  fs/fuse/xattr.c           | 21 +++++++++++++++------
>  include/uapi/linux/fuse.h | 17 +++++++++++++++++
>  5 files changed, 46 insertions(+), 9 deletions(-)
> 
> -- 
> 2.25.4
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://listman.redhat.com/mailman/listinfo/virtio-fs

Re: [PATCH v2 0/2] fuse: Fix clearing SGID when access ACL is set

[Miklos Szeredi]

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.

Applied and pushed to fuse.git#for-next.

Thanks,
Miklos

Re: [PATCH v2 0/2] fuse: Fix clearing SGID when access ACL is set

[Vivek Goyal]

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,

Thanks. Patches look good. I will update userspace part and repost.

Vivek

> 
> Applied and pushed to fuse.git#for-next.
> 
> Thanks,
> Miklos
> 

Re: [PATCH v2 0/2] fuse: Fix clearing SGID when access ACL is set

[Vivek Goyal]

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