| Message ID | 20210325151823.572089-2-vgoyal@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: Fix clearing SGID when access ACL is set | expand |
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 >
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 >
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
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 > > >
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,
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;
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(-)