Re: [PATCH v4] Add security.* XATTR support for the UBIFS

[Artem Bityutskiy <dedekind1@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2910 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]