lkml.org 
[lkml]   [2012]   [May]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4] Add security.* XATTR support for the UBIFS
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


\
 
 \ /
  Last update: 2012-05-15 00:02    [from the cache]
©2003-2011 Jasper Spaans