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

[Subodh Nijsure <snijsure@grid-net.com>]

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.
However the above implementation is consistent with ext2/ext3/ext4/jffs 
implementation.
> 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.
> Did you check whether the generic code already performs this check?
I didn't see a generic code that performs this check.

>> +	return __ubifs_setxattr(d->d_inode, name, value,
>> +				size, flags);
>> +}
>> +
>> +struct xattr_handler ubifs_xattr_security_handler = {
>> +	.prefix = XATTR_SECURITY_PREFIX,
>> +	.list   = ubifs_security_listxattr,
>> +	.get    = ubifs_security_getxattr,
>> +	.set    = ubifs_security_setxattr,
>> +};
>> +
>> +const struct xattr_handler *ubifs_xattr_handlers[] = {
>> +	&ubifs_xattr_security_handler,
>> +	NULL
>> +};
>> +
>> +static int ubifs_initxattrs(struct inode *inode,
>> +			    const struct xattr *xattr_array, void *fs_info)
>> +{
>> +	const struct xattr *xattr;
>> +	char *name;
>> +	int err = 0;
>> +
>> +	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.
>
>> +		}
>> +		strcpy(name, XATTR_SECURITY_PREFIX);
>> +		strcpy(name + XATTR_SECURITY_PREFIX_LEN, xattr->name);
>> +		err = __ubifs_setxattr(inode, name, xattr->value,
>> +				       xattr->value_len, 0);
>> +		kfree(name);
>> +		if (err<  0)
>> +			break;
>> +	}
>> +	return err;
>> +}
>> +
>> +int
>> +ubifs_init_security(struct inode *dentry, struct inode *inode,
>> +		   const struct qstr *qstr)
>> +{
>> +	struct ubifs_inode *dir_ui = ubifs_inode(inode);
>> +	int err = 0;
>> +
>> +	mutex_lock(&inode->i_mutex);
>> +	mutex_lock(&dir_ui->ui_mutex);
>> +
> 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().
I will wait couple days to see if there are other comments trickle in 
before submitting v5.
It would great if someone else can run UBIFS with extended attributes 
enabled and provide an ACK! ;-)

-Subodh