Re: [PATCH v4 1/3] xattr: add simple initxattrs function

[David Graziano]

On Sun, Jan 8, 2017 at 3:55 AM, Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
>> +/*
>> + * Callback for security_inode_init_security() for acquiring xattrs.
>> + */
>> +int simple_xattr_initxattrs(struct inode *inode,
>> +                         const struct xattr *xattr_array,
>> +                         void *fs_info)
>> +{
>> +     struct simple_xattrs *xattrs;
>> +     const struct xattr *xattr;
>> +     struct simple_xattr *new_xattr;
>> +     size_t len;
>> +
>> +     if (!fs_info)
>> +             return -ENOMEM;
>
> This probablt should be an EINVAL, and also a WARN_ON_ONCE.

I will change the return value to -EINVAL and add the WARN_ON_ONCE.
In the next version of the patchset.

>
>> +     xattrs = (struct simple_xattrs *) fs_info;
>
> No need for the cast.  In fact we should probably just declarate it
> as struct simple_xattrs *xattrs in the protoype and thus be type safe.

I don't think the prototype can be changed to "struct simple_xattrs" as the
security_inode_init_security() function in security/security.c which calls
this is asumming an initxattrs function with following prototype
int (*initxattrs)  (struct inode *inode, const struct xattr
*xattr_array, void *fs_data)

>
>> +
>> +     for (xattr = xattr_array; xattr->name != NULL; xattr++) {
>> +             new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len);
>> +             if (!new_xattr)
>> +                     return -ENOMEM;
>
> We'll need to unwind the previous allocations here.

This patchset essentially relocates the shmem_initxattrs() function from
mm/shmem.c and uses the relocated function for both tmpfs and mqueuefs.
That inital function didn't attempt to unwind the previous allocations. If the
consensus is to unwind any allocations made by this function I can look
at adding it.