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    [W:0.034 / U:1.368 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site