Messages in this thread | | | Subject | Re: [PATCH v4] Add security.* XATTR support for the UBIFS | From | Artem Bityutskiy <> | Date | Tue, 15 May 2012 13:29:13 +0300 |
| |
On Mon, 2012-05-14 at 14:09 -0700, Subodh Nijsure wrote: > On 05/14/2012 06:02 AM, Artem Bityutskiy wrote: > > On Sun, 2012-05-13 at 06:24 -0700, snijsure@grid-net.com wrote: > >> +int ubifs_security_getxattr(struct dentry *d, const char *name, > >> + void *buffer, size_t size, int flags) > >> +{ > >> + if (strcmp(name, "") == 0) > >> + return -EINVAL; > >> + return __ubifs_getxattr(d->d_inode, name, buffer, size); > >> +} > >> + > >> + > >> +int ubifs_security_setxattr(struct dentry *d, const char *name, > >> + const void *value, size_t size, > >> + int flags, int handler_flags) > >> +{ > >> + if (strcmp(name, "") == 0) > >> + return -EINVAL; > > Should this check pushed town to __ubifs_getxattr/__ubifs_setxattr ? > If you really want to move that check into __ubifs_get/setxattr we can > do that.
Yes, if other FSes have this check - please add it there.
> However the above implementation is consistent with ext2/ext3/ext4/jffs > implementation.
OK, but on the other hand - how much sense does it make to have these trivial wrappers? Should we have a wrapper per-check? :-)
BTW, to they have to be non-static?
> > Does an extended attribute in general with zero name length legitimate? > My preference would be to remain consistent with interpretation of other > file systems, in terms of what constitutes an > invalid parameter. ext* filesystems seem to be declaring a blank > extended attribute as invalid parameter. Man page for setxattr/getxattr > don't explicitly state as such though.
Sure, let's add this check - I guess I was not careful enough and missed it.
> > Did you check whether the generic code already performs this check? > I didn't see a generic code that performs this check.
OK, thanks.
> >> + for (xattr = xattr_array; xattr->name != NULL; xattr++) { > >> + name = kmalloc(XATTR_SECURITY_PREFIX_LEN + > >> + strlen(xattr->name) + 1, GFP_NOFS); > >> + if (!name) { > >> + err = -ENOMEM; > >> + break; > > Where is the already allocated memory freed in this case? > In this particular case kmalloc() failed and we are returning ENOMEM > error, and in case of success, we do free the allocated memory.
Indeed, sorry for silly question.
> > You do not actually need these mutexes, because "inode" is new, it is > > not added to any lists yet, so you own it entirely. Which means that you > > do not even need to introduce this helper function - just call > > 'security_inode_init_security()' directly. > Okay, I can change the code to directly call the > security_inode_init_security().
OK, thanks!
> It would great if someone else can run UBIFS with extended attributes > enabled and provide an ACK! ;-)
I will run it once you send the patch I cannot nit-pick on anymore (aka perfect patch) :-)))
Thanks!
-- Best Regards, Artem Bityutskiy [unhandled content-type:application/pgp-signature] | |