Re: [Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
On 8/15/19 3:27 PM, James Morris wrote: On Thu, 15 Aug 2019, Mark Salyzyn wrote: Good Idea, but using the same argument structure for set and get I would be concerned about the loss of compiler protection for the buffer argument; Agreed, I missed that. Sadly, the pattern of struct getxattr_args args; memset(&args, 0, sizeof(args)); args. = ... __vfs_getxattr(&args}; ... __vfs_setxattr(&args); would be nice, so maybe we need to cool our jets and instead: struct xattr_gs_args { struct dentry *dentry; struct inode *inode; const char *name; union { void *buffer; const void *value; }; size_t size; int flags; }; value _must_ be referenced for all setxattr operations, buffer for getxattr operations (how can we enforce that?). struct getxattr_args { struct dentry *dentry; struct inode *inode; const char *name; void *buffer; size_t size; int flags; Does 'get' need flags? :-) That was the _whole_ point of the patch, flags is how we pass in the recursion so that a security/internal getxattr call has the rights to acquire the data in the lower layer(s). -- Mark
Re: [Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
On Thu, 15 Aug 2019, Mark Salyzyn wrote: > Good Idea, but using the same argument structure for set and get I would be > concerned about the loss of compiler protection for the buffer argument; Agreed, I missed that. > struct getxattr_args { > struct dentry *dentry; > struct inode *inode; > const char *name; > void *buffer; > size_t size; > int flags; Does 'get' need flags? -- James Morris
Re: [Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
On 8/15/19 12:20 PM, James Morris wrote: On Tue, 13 Aug 2019, Greg Kroah-Hartman wrote: On Mon, Aug 12, 2019 at 12:32:49PM -0700, Mark Salyzyn wrote: --- a/include/linux/xattr.h +++ b/include/linux/xattr.h @@ -30,10 +30,10 @@ struct xattr_handler { const char *prefix; int flags; /* fs private flags */ bool (*list)(struct dentry *dentry); - int (*get)(const struct xattr_handler *, struct dentry *dentry, + int (*get)(const struct xattr_handler *handler, struct dentry *dentry, struct inode *inode, const char *name, void *buffer, - size_t size); - int (*set)(const struct xattr_handler *, struct dentry *dentry, + size_t size, int flags); + int (*set)(const struct xattr_handler *handler, struct dentry *dentry, struct inode *inode, const char *name, const void *buffer, size_t size, int flags); Wow, 7 arguments. Isn't there some nice rule of thumb that says once you get more then 5, a function becomes impossible to understand? Surely this could be a structure passed in here somehow, that way when you add the 8th argument in the future, you don't have to change everything yet again? :) I don't have anything concrete to offer as a replacement fix for this, but to me this just feels really wrong... How about something like: struct xattr_gs_args { struct dentry *dentry; struct inode *inode; const char *name; const void *buffer; size_t size; int flags; }; int (*get)(const struct xattr_handler *handler, struct xattr_gs_args *args); int (*set)(const struct xattr_handler *handler, struct xattr_gs_args *args); Good Idea, but using the same argument structure for set and get I would be concerned about the loss of compiler protection for the buffer argument; it is void* for get, and const void* for set. And if we pulled out buffer (and size since it is paired with it) from the structure to solve, the 'mixed' argument approach (resulting in 4 args) adds to the difficulty/complexity. Good news is the same structure(s) can get passed to __vfs_getxattr and __vfs_setxattr, so one less issue with getting the argument order correct from the caller. From an optimization standpoint, passing an argument to a pointer to a structure assembled on the stack constrains the compiler. Whereas individual arguments allow for the optimization to place all the arguments into registers. All modern processors have no issue with tens of arguments. So, I will look into what the patch set will look like by splitting into set and get, and trying to reuse the structure down the call chain. struct getxattr_args { struct dentry *dentry; struct inode *inode; const char *name; void *buffer; size_t size; int flags; }; struct setxattr_args { struct dentry *dentry; struct inode *inode; const char *name; const void *buffer; size_t size; int flags; }; -- Mark
Re: [Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
On Fri, Aug 16, 2019 at 05:20:36AM +1000, James Morris wrote: > On Tue, 13 Aug 2019, Greg Kroah-Hartman wrote: > > > On Mon, Aug 12, 2019 at 12:32:49PM -0700, Mark Salyzyn wrote: > > > --- a/include/linux/xattr.h > > > +++ b/include/linux/xattr.h > > > @@ -30,10 +30,10 @@ struct xattr_handler { > > > const char *prefix; > > > int flags; /* fs private flags */ > > > bool (*list)(struct dentry *dentry); > > > - int (*get)(const struct xattr_handler *, struct dentry *dentry, > > > + int (*get)(const struct xattr_handler *handler, struct dentry *dentry, > > > struct inode *inode, const char *name, void *buffer, > > > -size_t size); > > > - int (*set)(const struct xattr_handler *, struct dentry *dentry, > > > +size_t size, int flags); > > > + int (*set)(const struct xattr_handler *handler, struct dentry *dentry, > > > struct inode *inode, const char *name, const void *buffer, > > > size_t size, int flags); > > > > Wow, 7 arguments. Isn't there some nice rule of thumb that says once > > you get more then 5, a function becomes impossible to understand? > > > > Surely this could be a structure passed in here somehow, that way when > > you add the 8th argument in the future, you don't have to change > > everything yet again? :) > > > > I don't have anything concrete to offer as a replacement fix for this, > > but to me this just feels really wrong... > > How about something like: > > struct xattr_gs_args { > struct dentry *dentry; > struct inode *inode; As he said in a later message, dentry and inode is redundant, only 1 is needed (dentry I think?) > const char *name; > const void *buffer; > size_t size; > int flags; > }; > > int (*get)(const struct xattr_handler *handler, struct xattr_gs_args *args); > int (*set)(const struct xattr_handler *handler, struct xattr_gs_args *args); But yes, that would be much much better. thanks, greg k-h
Re: [Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
On Tue, 13 Aug 2019, Greg Kroah-Hartman wrote: > On Mon, Aug 12, 2019 at 12:32:49PM -0700, Mark Salyzyn wrote: > > --- a/include/linux/xattr.h > > +++ b/include/linux/xattr.h > > @@ -30,10 +30,10 @@ struct xattr_handler { > > const char *prefix; > > int flags; /* fs private flags */ > > bool (*list)(struct dentry *dentry); > > - int (*get)(const struct xattr_handler *, struct dentry *dentry, > > + int (*get)(const struct xattr_handler *handler, struct dentry *dentry, > >struct inode *inode, const char *name, void *buffer, > > - size_t size); > > - int (*set)(const struct xattr_handler *, struct dentry *dentry, > > + size_t size, int flags); > > + int (*set)(const struct xattr_handler *handler, struct dentry *dentry, > >struct inode *inode, const char *name, const void *buffer, > >size_t size, int flags); > > Wow, 7 arguments. Isn't there some nice rule of thumb that says once > you get more then 5, a function becomes impossible to understand? > > Surely this could be a structure passed in here somehow, that way when > you add the 8th argument in the future, you don't have to change > everything yet again? :) > > I don't have anything concrete to offer as a replacement fix for this, > but to me this just feels really wrong... How about something like: struct xattr_gs_args { struct dentry *dentry; struct inode *inode; const char *name; const void *buffer; size_t size; int flags; }; int (*get)(const struct xattr_handler *handler, struct xattr_gs_args *args); int (*set)(const struct xattr_handler *handler, struct xattr_gs_args *args); -- James Morris
Re: [Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
On 8/13/19 1:48 AM, Greg Kroah-Hartman wrote: On Mon, Aug 12, 2019 at 12:32:49PM -0700, Mark Salyzyn wrote: --- a/include/linux/xattr.h +++ b/include/linux/xattr.h @@ -30,10 +30,10 @@ struct xattr_handler { const char *prefix; int flags; /* fs private flags */ bool (*list)(struct dentry *dentry); - int (*get)(const struct xattr_handler *, struct dentry *dentry, + int (*get)(const struct xattr_handler *handler, struct dentry *dentry, struct inode *inode, const char *name, void *buffer, - size_t size); - int (*set)(const struct xattr_handler *, struct dentry *dentry, + size_t size, int flags); + int (*set)(const struct xattr_handler *handler, struct dentry *dentry, struct inode *inode, const char *name, const void *buffer, size_t size, int flags); Wow, 7 arguments. Isn't there some nice rule of thumb that says once you get more then 5, a function becomes impossible to understand? This is a method with a pot-pourri of somewhat intuitive useful, but not always necessary, arguments, the additional argument does not complicate the function(s) AFAIK, but maybe its usage. Most functions do not even reference handler, the inode is typically a derivative of dentry, The arguments most used are the name of the attribute and the buffer/size the results are to be placed into. The addition of flags is actually a pattern borrowed from the [.]set method, which provides at least 32 bits of 'control' (of which we added only one). Before, it was an anti-pattern. Surely this could be a structure passed in here somehow, that way when you add the 8th argument in the future, you don't have to change everything yet again? :) Just be happy I provided int flags, instead of bool no_security ;-> there are a few bits there that can be used in the future. I don't have anything concrete to offer as a replacement fix for this, but to me this just feels really wrong... I went through 6 different alternatives (in the overlayfs security fix patch set) until I found this one that resonated with the security and filesystem stakeholders. The one was a direct result of trying to reduce the security attack surface. This code was created by threading a needle, and evolution. I am game for a 7th alternative to solve the unionfs set of recursive calls into acquiring the extended attributes. -- Mark
Re: [Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
On Mon, Aug 12, 2019 at 12:32:49PM -0700, Mark Salyzyn wrote: > --- a/include/linux/xattr.h > +++ b/include/linux/xattr.h > @@ -30,10 +30,10 @@ struct xattr_handler { > const char *prefix; > int flags; /* fs private flags */ > bool (*list)(struct dentry *dentry); > - int (*get)(const struct xattr_handler *, struct dentry *dentry, > + int (*get)(const struct xattr_handler *handler, struct dentry *dentry, > struct inode *inode, const char *name, void *buffer, > -size_t size); > - int (*set)(const struct xattr_handler *, struct dentry *dentry, > +size_t size, int flags); > + int (*set)(const struct xattr_handler *handler, struct dentry *dentry, > struct inode *inode, const char *name, const void *buffer, > size_t size, int flags); Wow, 7 arguments. Isn't there some nice rule of thumb that says once you get more then 5, a function becomes impossible to understand? Surely this could be a structure passed in here somehow, that way when you add the 8th argument in the future, you don't have to change everything yet again? :) I don't have anything concrete to offer as a replacement fix for this, but to me this just feels really wrong... thanks, greg k-h
Re: [Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
Hi Mark, Thank you for the patch! Yet something to improve: [auto build test ERROR on linus/master] [cannot apply to v5.3-rc4] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Mark-Salyzyn/Add-flags-option-to-get-xattr-method-paired-to-__vfs_getxattr/20190813-124612 config: c6x-allyesconfig (attached as .config) compiler: c6x-elf-gcc (GCC) 7.4.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.4.0 make.cross ARCH=c6x If you fix the issue, kindly add following tag Reported-by: kbuild test robot All errors (new ones prefixed by >>): >> fs/ubifs/xattr.c:699:9: error: initialization from incompatible pointer type >> [-Werror=incompatible-pointer-types] .get = xattr_get, ^ fs/ubifs/xattr.c:699:9: note: (near initialization for 'ubifs_user_xattr_handler.get') fs/ubifs/xattr.c:705:9: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types] .get = xattr_get, ^ fs/ubifs/xattr.c:705:9: note: (near initialization for 'ubifs_trusted_xattr_handler.get') fs/ubifs/xattr.c:712:9: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types] .get = xattr_get, ^ fs/ubifs/xattr.c:712:9: note: (near initialization for 'ubifs_security_xattr_handler.get') cc1: some warnings being treated as errors -- >> drivers/staging//erofs/xattr.c:492:9: error: initialization from >> incompatible pointer type [-Werror=incompatible-pointer-types] .get = erofs_xattr_generic_get, ^~~ drivers/staging//erofs/xattr.c:492:9: note: (near initialization for 'erofs_xattr_user_handler.get') drivers/staging//erofs/xattr.c:499:9: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types] .get = erofs_xattr_generic_get, ^~~ drivers/staging//erofs/xattr.c:499:9: note: (near initialization for 'erofs_xattr_trusted_handler.get') drivers/staging//erofs/xattr.c:506:9: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types] .get = erofs_xattr_generic_get, ^~~ drivers/staging//erofs/xattr.c:506:9: note: (near initialization for 'erofs_xattr_security_handler.get') cc1: some warnings being treated as errors -- >> fs//afs/xattr.c:156:12: error: initialization from incompatible pointer type >> [-Werror=incompatible-pointer-types] .get= afs_xattr_get_acl, ^ fs//afs/xattr.c:156:12: note: (near initialization for 'afs_xattr_afs_acl_handler.get') fs//afs/xattr.c:327:9: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types] .get = afs_xattr_get_yfs, ^ fs//afs/xattr.c:327:9: note: (near initialization for 'afs_xattr_yfs_handler.get') cc1: some warnings being treated as errors vim +699 fs/ubifs/xattr.c 2b88fc21cae91e Andreas Gruenbacher 2016-04-22 696 dfaf8d2aeca482 Ben Dooks 2016-06-21 697 static const struct xattr_handler ubifs_user_xattr_handler = { 2b88fc21cae91e Andreas Gruenbacher 2016-04-22 698 .prefix = XATTR_USER_PREFIX, ade46c3a6029de Richard Weinberger 2016-09-19 @699 .get = xattr_get, ade46c3a6029de Richard Weinberger 2016-09-19 700 .set = xattr_set, 2b88fc21cae91e Andreas Gruenbacher 2016-04-22 701 }; 2b88fc21cae91e Andreas Gruenbacher 2016-04-22 702 :: The code at line 699 was first introduced by commit :: ade46c3a6029dea49dbc6c7734b0f6a78d3f104c ubifs: Export xattr get and set functions :: TO: Richard Weinberger :: CC: Richard Weinberger --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip