[RFC PATCH v3 3/5] UBIFS: ACL: handle ACL through xattr

[Dongsheng Yang]

On 09/12/2015 09:15 AM, Sheng Yong wrote:
>
>
> On 9/11/2015 2:25 PM, Dongsheng Yang wrote:
>> On 09/11/2015 02:18 PM, Sheng Yong wrote:
>>> Hi, Dongsheng
>>>
>>> On 9/11/2015 1:01 PM, Dongsheng Yang wrote:
>>>> On 09/11/2015 05:09 PM, Sheng Yong wrote:
>>> [...]
>>>>
>>>> Why move it? If you just want to use them before the definitions,
>>>> Just declare them before using.
>>>
>>> OK.
>>>>>     int ubifs_do_setxattr(struct inode *host, const char *name,
>>>>>                   const void *value, size_t size, int flags)
>>>>>     {
>>>>> @@ -348,7 +462,19 @@ int ubifs_do_setxattr(struct inode *host, const char *name,
>>>>>             goto out_free;
>>>>>         }
>>>>>
>>>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>>>>> +    if (size == 0) {
>>>>> +        ubifs_assert(inode->i_nlink == 1);
>>>>> +        clear_nlink(inode);
>>>>> +        err = remove_xattr(c, host, inode, &nm);
>>>>> +        if (err)
>>>>> +            set_nlink(inode, 1);
>>>>> +        iput(inode);
>>>>> +        goto out_free;
>>>>> +    }
>>>>> +#endif
>>>>
>>>> Is there a testcase for it?
>>>
>>> I test `setfacl -b/-k'. I don't know how setfacl is implemented. But for ACL_TYPE_ACCESS,
>>> ubifs_setxattr() is called with size = 0 and value = NULL; while for ACL_TYPE_DEFAULT,
>>> ubifs_removexattr() is called.
>>
>> I mean, if we don't add the code above, what kind of problem
>> we would meet?
>
> Then, a new xattr wich length 0 and value NULL, is written to flash, which is useless.
> I don't think we should keep this xattr.
>
>>>
>>>>>         err = change_xattr(c, host, inode, value, size);
>>>>> +
>>>>>         iput(inode);
>>>>>
>>>>>     out_free:
>>>>> @@ -359,6 +485,9 @@ out_free:
>>>>>     int ubifs_setxattr(struct dentry *dentry, const char *name,
>>>>>                const void *value, size_t size, int flags)
>>>>>     {
>>>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>>>>> +    const struct xattr_handler *handler;
>>>>> +#endif
>>>>>         struct qstr nm = QSTR_INIT(name, strlen(name));
>>>>>         int type;
>>>>>
>>>>> @@ -369,6 +498,16 @@ int ubifs_setxattr(struct dentry *dentry, const char *name,
>>>>>         if (type < 0)
>>>>>             return type;
>>>>>
>>>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>>>>> +    if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) {
>>>>> +        if (type == POSIX_ACL_DEFAULT)
>>>>> +            handler = &posix_acl_default_xattr_handler;
>>>>> +        if (type == POSIX_ACL_ACCESS)
>>>>> +            handler = &posix_acl_access_xattr_handler;
>>>>> +        return handler->set(dentry, name, value, size, flags,
>>>>> +                    handler->flags);
>>>>> +    }
>>>>> +#endif
>>>>
>>>> What about setting sb->s_xattr and calling generic_setxattr() here?
>>>
>>> I have no idea if we should do this :(
>>> If we do, I think, we should call generic functions for all xattr.
>>
>
> I copied the xattr handler discussion from the other email, so that
> we could have it at one place :)

That's great :)
>
>> No, only for POSIX_ACL_DEFAULT|POSIX_ACL_ACCESS currently. Then
>> Something like that:
>>
>> if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS)
>>      return generic_setxattr(dentry, name, value, size, flags);
>
> Acutally, in my v1 patchset, I did like this. The URL is:
> http://lists.infradead.org/pipermail/linux-mtd/2015-March/058451.html
>
>> Security handler is different with acl handler. Please read the
>> security_getxattr(), it just return ubifs_getxattr(). That means
>> we can use ubifs_getxattr() immediately. So we can remove the
>> security_handler for ubifs.
>>
>> But acl is different, we need to call a special ubifs_get_acl()
>> here. Then I found you get the handler by youself in the
>> ubifs_get|set_xattr() and call handler->set() by youself.
>
> Right, they are different. I mean using the generic way means
> ubifs_[get|set]xattr should be modified.
>
>>
>> That's not good idea. Please Just set the handler and call
>> generic_setxattr().
>
> But IMO, calling generic_setxattr means we have to add
> posix_acl_[access|default]_xattr_handler in ubifs_xattr_handlers, like
>
>   const struct xattr_handler *ubifs_xattr_handlers[] = {
>   	&ubifs_xattr_security_handler,
> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
> +	&posix_acl_access_xattr_handler,
> +	&posix_acl_default_xattr_handler,
> +#endif
>   	NULL,
>   };

Yes, obviously.
>
> then, generic_setxattr() could call posix_acl_***_xattr_handler.set(),
> which will call ubifs_set_acl() to do the real setting. Since security
> xattr dead code is going to be removed, ubifs_xattr_handlers may go
> away too.

So there would be a conflict with the other patch from Richard to
remove ubifs_xattr_handlers. But that's not a problem at all, just
address it when merging.We do not use security_handler in ubifs, but
that does not mean we can not use sb->xattr in ubifs.

> So I agree with Andreas that if we could add all xattr handlers
> in ubifs_xattr_handlers (which means ubifs_[get|set]xattr should be
> modified and generic_[get|set]xattr should be set in inode_operations,
> e.g ext4, jffs2),it's better to use generic_setxattr. I'm not sure
> whether we should convert ubifs to use the xattr handler infrastructure,
> like what Andreas said.

Okey, I think I got your meaning now. You are proposing something like
jffs2. Let me try to make our discussion more clear. There are some 
proposals here.

(1). What you did in this patch, get the handler for acl and call
handler->get() in ubifs_getxattr(). I disagree with this idea.

(2). What you suggest above to implement all handlers and assign
inode_ops->get_xattr with generic_getxattr().I think there would be a 
lot of duplication in this implementation. E.g, Why we need a 
check_namespace() in ubifs? That would solve the duplicated checking.
So we need much more code to implement this idea and these code are
almost same.

(3). What you did in V1. Use xattr handler for necessary, currently
only for acl handlers. Same with what Btrfs did.

Hi Sheng, I vote (3).

Thanx
Yang
>
> thanks,
> Sheng
>
>>
>> Yang
>>>
>>> thanks,
>>> Sheng
>>>
>>>>>         return ubifs_do_setxattr(d_inode(dentry), name, value, size, flags);
>>>>>     }
>>>>>
>>>>> @@ -428,6 +567,9 @@ out_unlock:
>>>>>     ssize_t ubifs_getxattr(struct dentry *dentry, const char *name,
>>>>>                    void *value, size_t size)
>>>>>     {
>>>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>>>>> +    const struct xattr_handler *handler;
>>>>> +#endif
>>>>>         struct qstr nm = QSTR_INIT(name, strlen(name));
>>>>>         int type;
>>>>>
>>>>> @@ -438,6 +580,16 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name,
>>>>>         if (type < 0)
>>>>>             return type;
>>>>>
>>>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>>>>> +    if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) {
>>>>> +        if (type == POSIX_ACL_DEFAULT)
>>>>> +            handler = &posix_acl_default_xattr_handler;
>>>>> +        if (type == POSIX_ACL_ACCESS)
>>>>> +            handler = &posix_acl_access_xattr_handler;
>>>>> +        return handler->get(dentry, name, value, size,
>>>>> +                    handler->flags);
>>>>> +    }
>>>>> +#endif
>>>>
>>>> Ditto
>>>>
>>>> Thanx
>>>> Yang
>>>>>         return ubifs_do_getxattr(d_inode(dentry), name, value, size);
>>>>>     }
>>>>>
>>>>> @@ -505,95 +657,6 @@ ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size)
>>>>>         return written;
>>>>>     }
>>>>>
>>>>> -static int remove_xattr(struct ubifs_info *c, struct inode *host,
>>>>> -            struct inode *inode, const struct qstr *nm)
>>>>> -{
>>>>> -    int err;
>>>>> -    struct ubifs_inode *host_ui = ubifs_inode(host);
>>>>> -    struct ubifs_inode *ui = ubifs_inode(inode);
>>>>> -    struct ubifs_budget_req req = { .dirtied_ino = 2, .mod_dent = 1,
>>>>> -                .dirtied_ino_d = ALIGN(host_ui->data_len, 8) };
>>>>> -
>>>>> -    ubifs_assert(ui->data_len == inode->i_size);
>>>>> -
>>>>> -    err = ubifs_budget_space(c, &req);
>>>>> -    if (err)
>>>>> -        return err;
>>>>> -
>>>>> -    mutex_lock(&host_ui->ui_mutex);
>>>>> -    host->i_ctime = ubifs_current_time(host);
>>>>> -    host_ui->xattr_cnt -= 1;
>>>>> -    host_ui->xattr_size -= CALC_DENT_SIZE(nm->len);
>>>>> -    host_ui->xattr_size -= CALC_XATTR_BYTES(ui->data_len);
>>>>> -    host_ui->xattr_names -= nm->len;
>>>>> -
>>>>> -    err = ubifs_jnl_delete_xattr(c, host, inode, nm);
>>>>> -    if (err)
>>>>> -        goto out_cancel;
>>>>> -    mutex_unlock(&host_ui->ui_mutex);
>>>>> -
>>>>> -    ubifs_release_budget(c, &req);
>>>>> -    return 0;
>>>>> -
>>>>> -out_cancel:
>>>>> -    host_ui->xattr_cnt += 1;
>>>>> -    host_ui->xattr_size += CALC_DENT_SIZE(nm->len);
>>>>> -    host_ui->xattr_size += CALC_XATTR_BYTES(ui->data_len);
>>>>> -    mutex_unlock(&host_ui->ui_mutex);
>>>>> -    ubifs_release_budget(c, &req);
>>>>> -    make_bad_inode(inode);
>>>>> -    return err;
>>>>> -}
>>>>> -
>>>>> -int ubifs_removexattr(struct dentry *dentry, const char *name)
>>>>> -{
>>>>> -    struct inode *inode, *host = d_inode(dentry);
>>>>> -    struct ubifs_info *c = host->i_sb->s_fs_info;
>>>>> -    struct qstr nm = QSTR_INIT(name, strlen(name));
>>>>> -    struct ubifs_dent_node *xent;
>>>>> -    union ubifs_key key;
>>>>> -    int err;
>>>>> -
>>>>> -    dbg_gen("xattr '%s', ino %lu ('%pd')", name,
>>>>> -        host->i_ino, dentry);
>>>>> -    ubifs_assert(mutex_is_locked(&host->i_mutex));
>>>>> -
>>>>> -    err = check_namespace(&nm);
>>>>> -    if (err < 0)
>>>>> -        return err;
>>>>> -
>>>>> -    xent = kmalloc(UBIFS_MAX_XENT_NODE_SZ, GFP_NOFS);
>>>>> -    if (!xent)
>>>>> -        return -ENOMEM;
>>>>> -
>>>>> -    xent_key_init(c, &key, host->i_ino, &nm);
>>>>> -    err = ubifs_tnc_lookup_nm(c, &key, xent, &nm);
>>>>> -    if (err) {
>>>>> -        if (err == -ENOENT)
>>>>> -            err = -ENODATA;
>>>>> -        goto out_free;
>>>>> -    }
>>>>> -
>>>>> -    inode = iget_xattr(c, le64_to_cpu(xent->inum));
>>>>> -    if (IS_ERR(inode)) {
>>>>> -        err = PTR_ERR(inode);
>>>>> -        goto out_free;
>>>>> -    }
>>>>> -
>>>>> -    ubifs_assert(inode->i_nlink == 1);
>>>>> -    clear_nlink(inode);
>>>>> -    err = remove_xattr(c, host, inode, &nm);
>>>>> -    if (err)
>>>>> -        set_nlink(inode, 1);
>>>>> -
>>>>> -    /* If @i_nlink is 0, 'iput()' will delete the inode */
>>>>> -    iput(inode);
>>>>> -
>>>>> -out_free:
>>>>> -    kfree(xent);
>>>>> -    return err;
>>>>> -}
>>>>> -
>>>>>     static size_t security_listxattr(struct dentry *d, char *list, size_t list_size,
>>>>>                      const char *name, size_t name_len, int flags)
>>>>>     {
>>>>>
>>>>
>>>>
>>>> .
>>>>
>>>
>>> .
>>>
>>
>>
>> .
>>
>
> .
>