[RFC PATCH v3 0/5] UBIFS: add ACL support

[RFC PATCH v3 0/5] UBIFS: add ACL support

[Sheng Yong]

Hi, folks,
These patches add ACL support for UBIFS.

UBIFS ACL is based on xattr, which simplifies the implementation without
considering budget and sync.

New kernel/mount options, CONFIG_UBIFS_FS_POSIX_ACL and `-o acl', are
introduced to enable or disable UBIFS ACL.

`acl.c' provides format conversion between in-memory ACL and on-flash ACL.
Each time a new inode is created, the correspondng ACL xattr is built
(except for the inode of an xattr dent). 

Testcases (adjusted to mount an UBI volume) in LTP [1] are passed.

Any comment is appreciated. More test is appreciated.

V3:
* Use POSIX generic ACL structures and functions to do the conversions
  between in-memory and on-flash ACL. Thanks Dongsheng Yang for pointing
  this out.
* Remove ACL if size is 0 when setting ACL.
* Clear cached ACL when ACL xattr is removed.

V2:
* Do not set ACL-xattr-handler, and call generic ACL handler in xattr
  functions.
http://lists.infradead.org/pipermail/linux-mtd/2015-September/061672.html

V1:
http://lists.infradead.org/pipermail/linux-mtd/2015-March/058452.html

[1] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/fs/acl/tacl_xattr.sh

thanks,
Sheng

Sheng Yong (5):
  UBIFS: ACL: introduce init/set/get functions for ACL
  UBIFS: ACL: set ACL interfaces in inode_operations
  UBIFS: ACL: handle ACL through xattr
  UBIFS: ACL: introduce ACL mount options
  UBIFS: ACL: add ACL config option

 fs/ubifs/Kconfig  |  11 +++
 fs/ubifs/Makefile |   1 +
 fs/ubifs/acl.c    | 141 +++++++++++++++++++++++++++++++
 fs/ubifs/dir.c    |  20 +++++
 fs/ubifs/file.c   |  14 ++++
 fs/ubifs/super.c  |  15 ++++
 fs/ubifs/ubifs.h  |  14 ++++
 fs/ubifs/xattr.c  | 243 ++++++++++++++++++++++++++++++++++--------------------
 8 files changed, 369 insertions(+), 90 deletions(-)
 create mode 100644 fs/ubifs/acl.c

-- 
1.9.1

[RFC PATCH v3 1/5] UBIFS: ACL: introduce init/set/get functions for ACL

[Sheng Yong]

This patch introduce a new file `acl.c', which implements:
  * initializing ACL for new file according to the directory's default ACL,
  * getting ACL which finds the ACL releated xattr and converts it to ACL,
  * setting ACL function which converts ACL to xattr and creates/changes/
    removes the xattr.

On flash ACL format is based on POSIX ACL structures, and POSIX generic
functions, posix_acl_[to|from]_xattr, are called to do the ACL conversion
between in-memory and on-flash ACL.

The ACL xattr handler is not implemented, because UBIFS does not use it.

Signed-off-by: Sheng Yong <shengyong1@huawei.com>
---
 fs/ubifs/acl.c   | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ubifs/ubifs.h |  14 ++++++
 2 files changed, 155 insertions(+)
 create mode 100644 fs/ubifs/acl.c

diff --git a/fs/ubifs/acl.c b/fs/ubifs/acl.c
new file mode 100644
index 0000000..bf37875
--- /dev/null
+++ b/fs/ubifs/acl.c
@@ -0,0 +1,141 @@
+/*
+ * This file is part of UBIFS.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ */
+
+#include <linux/fs.h>
+#include <linux/xattr.h>
+#include <linux/posix_acl_xattr.h>
+
+#include "ubifs.h"
+
+struct posix_acl *ubifs_get_acl(struct inode *inode, int type)
+{
+	struct posix_acl *acl;
+	char *name, *value = NULL;
+	int size = 0;
+
+	switch (type) {
+	case ACL_TYPE_ACCESS:
+		name = XATTR_NAME_POSIX_ACL_ACCESS;
+		break;
+	case ACL_TYPE_DEFAULT:
+		name = XATTR_NAME_POSIX_ACL_DEFAULT;
+		break;
+	default:
+		BUG();
+	}
+
+	size = ubifs_do_getxattr(inode, name, NULL, 0);
+	if (size > 0) {
+		value = kmalloc(size, GFP_KERNEL);
+		if (!value)
+			return ERR_PTR(-ENOMEM);
+		size = ubifs_do_getxattr(inode, name, value, size);
+	}
+	if (size > 0)
+		acl = posix_acl_from_xattr(&init_user_ns, value, size);
+	else if (size == -ENODATA)
+		acl = NULL;
+	else
+		acl = ERR_PTR(size);
+
+	kfree(value);
+	if (!IS_ERR(acl))
+		set_cached_acl(inode, type, acl);
+
+	return acl;
+}
+
+int ubifs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
+{
+	char *name;
+	void *value = NULL;
+	size_t size = 0;
+	int flag = 0, err;
+
+	switch (type) {
+	case ACL_TYPE_ACCESS:
+		name = XATTR_NAME_POSIX_ACL_ACCESS;
+		if (acl) {
+			err = posix_acl_equiv_mode(acl, &inode->i_mode);
+			if (err < 0)
+				return err;
+			if (err == 0)
+				acl = NULL;
+		}
+		break;
+
+	case ACL_TYPE_DEFAULT:
+		name = XATTR_NAME_POSIX_ACL_DEFAULT;
+		if (!S_ISDIR(inode->i_mode))
+			return acl ? -EACCES : 0;
+		break;
+
+	default:
+		BUG();
+	}
+
+	if (acl) {
+		size = posix_acl_xattr_size(acl->a_count);
+		value = kmalloc(size, GFP_NOFS);
+		if (!value)
+			return -ENOMEM;
+
+		err = posix_acl_to_xattr(&init_user_ns, acl, value, size);
+		if (err < 0) {
+			kfree(value);
+			return err;
+		}
+	}
+
+	if (size == 0)
+		flag = XATTR_REPLACE;
+	err = ubifs_do_setxattr(inode, name, value, size, flag);
+
+	kfree(value);
+	if (!err)
+		set_cached_acl(inode, type, acl);
+
+	return err;
+}
+
+/*
+ * Initialize the ACLs of a new inode.
+ */
+int ubifs_init_acl(struct inode *dir, struct inode *inode)
+{
+	struct posix_acl *default_acl, *acl;
+	int err;
+
+	err = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
+	if (err)
+		return err;
+
+	if (default_acl) {
+		mutex_lock(&inode->i_mutex);
+		err = ubifs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
+		mutex_unlock(&inode->i_mutex);
+		posix_acl_release(default_acl);
+	}
+
+	if (acl) {
+		if (!err) {
+			mutex_lock(&inode->i_mutex);
+			err = ubifs_set_acl(inode, acl, ACL_TYPE_ACCESS);
+			mutex_unlock(&inode->i_mutex);
+		}
+		posix_acl_release(acl);
+	}
+
+	return err;
+}
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 62aa1a5..b9ddc8d 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -1767,6 +1767,20 @@ int ubifs_removexattr(struct dentry *dentry, const char *name);
 int ubifs_init_security(struct inode *dentry, struct inode *inode,
 			const struct qstr *qstr);
 
+/* acl.c */
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+int ubifs_init_acl(struct inode *dir, struct inode *inode);
+int ubifs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
+struct posix_acl *ubifs_get_acl(struct inode *inode, int type);
+#else
+static inline int ubifs_init_acl(struct inode *inode, struct inode *dir)
+{
+	return 0;
+}
+#define ubifs_get_acl NULL
+#define ubifs_set_acl NULL
+#endif
+
 /* super.c */
 struct inode *ubifs_iget(struct super_block *sb, unsigned long inum);
 
-- 
1.9.1

Re: [RFC PATCH v3 1/5] UBIFS: ACL: introduce init/set/get functions for ACL

[Dongsheng Yang]

On 09/11/2015 05:09 PM, Sheng Yong wrote:
> This patch introduce a new file `acl.c', which implements:
>    * initializing ACL for new file according to the directory's default ACL,
>    * getting ACL which finds the ACL releated xattr and converts it to ACL,
>    * setting ACL function which converts ACL to xattr and creates/changes/
>      removes the xattr.
>
> On flash ACL format is based on POSIX ACL structures, and POSIX generic
> functions, posix_acl_[to|from]_xattr, are called to do the ACL conversion
> between in-memory and on-flash ACL.
>
> The ACL xattr handler is not implemented, because UBIFS does not use it.

Why not, I think we can use it.

Yang
>
> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
> ---
>   fs/ubifs/acl.c   | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   fs/ubifs/ubifs.h |  14 ++++++
>   2 files changed, 155 insertions(+)
>   create mode 100644 fs/ubifs/acl.c
>
> diff --git a/fs/ubifs/acl.c b/fs/ubifs/acl.c
> new file mode 100644
> index 0000000..bf37875
> --- /dev/null
> +++ b/fs/ubifs/acl.c
> @@ -0,0 +1,141 @@
> +/*
> + * This file is part of UBIFS.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/xattr.h>
> +#include <linux/posix_acl_xattr.h>
> +
> +#include "ubifs.h"
> +
> +struct posix_acl *ubifs_get_acl(struct inode *inode, int type)
> +{
> +	struct posix_acl *acl;
> +	char *name, *value = NULL;
> +	int size = 0;
> +
> +	switch (type) {
> +	case ACL_TYPE_ACCESS:
> +		name = XATTR_NAME_POSIX_ACL_ACCESS;
> +		break;
> +	case ACL_TYPE_DEFAULT:
> +		name = XATTR_NAME_POSIX_ACL_DEFAULT;
> +		break;
> +	default:
> +		BUG();
> +	}
> +
> +	size = ubifs_do_getxattr(inode, name, NULL, 0);
> +	if (size > 0) {
> +		value = kmalloc(size, GFP_KERNEL);
> +		if (!value)
> +			return ERR_PTR(-ENOMEM);
> +		size = ubifs_do_getxattr(inode, name, value, size);
> +	}
> +	if (size > 0)
> +		acl = posix_acl_from_xattr(&init_user_ns, value, size);
> +	else if (size == -ENODATA)
> +		acl = NULL;
> +	else
> +		acl = ERR_PTR(size);
> +
> +	kfree(value);
> +	if (!IS_ERR(acl))
> +		set_cached_acl(inode, type, acl);
> +
> +	return acl;
> +}
> +
> +int ubifs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> +{
> +	char *name;
> +	void *value = NULL;
> +	size_t size = 0;
> +	int flag = 0, err;
> +
> +	switch (type) {
> +	case ACL_TYPE_ACCESS:
> +		name = XATTR_NAME_POSIX_ACL_ACCESS;
> +		if (acl) {
> +			err = posix_acl_equiv_mode(acl, &inode->i_mode);
> +			if (err < 0)
> +				return err;
> +			if (err == 0)
> +				acl = NULL;
> +		}
> +		break;
> +
> +	case ACL_TYPE_DEFAULT:
> +		name = XATTR_NAME_POSIX_ACL_DEFAULT;
> +		if (!S_ISDIR(inode->i_mode))
> +			return acl ? -EACCES : 0;
> +		break;
> +
> +	default:
> +		BUG();
> +	}
> +
> +	if (acl) {
> +		size = posix_acl_xattr_size(acl->a_count);
> +		value = kmalloc(size, GFP_NOFS);
> +		if (!value)
> +			return -ENOMEM;
> +
> +		err = posix_acl_to_xattr(&init_user_ns, acl, value, size);
> +		if (err < 0) {
> +			kfree(value);
> +			return err;
> +		}
> +	}
> +
> +	if (size == 0)
> +		flag = XATTR_REPLACE;
> +	err = ubifs_do_setxattr(inode, name, value, size, flag);
> +
> +	kfree(value);
> +	if (!err)
> +		set_cached_acl(inode, type, acl);
> +
> +	return err;
> +}
> +
> +/*
> + * Initialize the ACLs of a new inode.
> + */
> +int ubifs_init_acl(struct inode *dir, struct inode *inode)
> +{
> +	struct posix_acl *default_acl, *acl;
> +	int err;
> +
> +	err = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
> +	if (err)
> +		return err;
> +
> +	if (default_acl) {
> +		mutex_lock(&inode->i_mutex);
> +		err = ubifs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
> +		mutex_unlock(&inode->i_mutex);
> +		posix_acl_release(default_acl);
> +	}
> +
> +	if (acl) {
> +		if (!err) {
> +			mutex_lock(&inode->i_mutex);
> +			err = ubifs_set_acl(inode, acl, ACL_TYPE_ACCESS);
> +			mutex_unlock(&inode->i_mutex);
> +		}
> +		posix_acl_release(acl);
> +	}
> +
> +	return err;
> +}
> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> index 62aa1a5..b9ddc8d 100644
> --- a/fs/ubifs/ubifs.h
> +++ b/fs/ubifs/ubifs.h
> @@ -1767,6 +1767,20 @@ int ubifs_removexattr(struct dentry *dentry, const char *name);
>   int ubifs_init_security(struct inode *dentry, struct inode *inode,
>   			const struct qstr *qstr);
>
> +/* acl.c */
> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
> +int ubifs_init_acl(struct inode *dir, struct inode *inode);
> +int ubifs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
> +struct posix_acl *ubifs_get_acl(struct inode *inode, int type);
> +#else
> +static inline int ubifs_init_acl(struct inode *inode, struct inode *dir)
> +{
> +	return 0;
> +}
> +#define ubifs_get_acl NULL
> +#define ubifs_set_acl NULL
> +#endif
> +
>   /* super.c */
>   struct inode *ubifs_iget(struct super_block *sb, unsigned long inum);
>
>

Re: [RFC PATCH v3 1/5] UBIFS: ACL: introduce init/set/get functions for ACL

[Sheng Yong]

Hi, Dongsheng

On 9/11/2015 1:01 PM, Dongsheng Yang wrote:
> On 09/11/2015 05:09 PM, Sheng Yong wrote:
>> This patch introduce a new file `acl.c', which implements:
>>    * initializing ACL for new file according to the directory's default ACL,
>>    * getting ACL which finds the ACL releated xattr and converts it to ACL,
>>    * setting ACL function which converts ACL to xattr and creates/changes/
>>      removes the xattr.
>>
>> On flash ACL format is based on POSIX ACL structures, and POSIX generic
>> functions, posix_acl_[to|from]_xattr, are called to do the ACL conversion
>> between in-memory and on-flash ACL.
>>
>> The ACL xattr handler is not implemented, because UBIFS does not use it.
> 
> Why not, I think we can use it.
First of all, thanks for your review :)

It seems xattr handler will be removed because of dead code of security xattr.
And if we do so, I think it's better to do that for all xattr, but not just
security and ACL, and generic_[set|get]xattr should be set in inode_operations
instead of ubifs_[get|set]xattr.

thanks,
Sheng
> 
> Yang
>>
>> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
>> ---
>>   fs/ubifs/acl.c   | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   fs/ubifs/ubifs.h |  14 ++++++
>>   2 files changed, 155 insertions(+)
>>   create mode 100644 fs/ubifs/acl.c
>>
>> diff --git a/fs/ubifs/acl.c b/fs/ubifs/acl.c
>> new file mode 100644
>> index 0000000..bf37875
>> --- /dev/null
>> +++ b/fs/ubifs/acl.c
>> @@ -0,0 +1,141 @@
>> +/*
>> + * This file is part of UBIFS.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + */
>> +
>> +#include <linux/fs.h>
>> +#include <linux/xattr.h>
>> +#include <linux/posix_acl_xattr.h>
>> +
>> +#include "ubifs.h"
>> +
>> +struct posix_acl *ubifs_get_acl(struct inode *inode, int type)
>> +{
>> +    struct posix_acl *acl;
>> +    char *name, *value = NULL;
>> +    int size = 0;
>> +
>> +    switch (type) {
>> +    case ACL_TYPE_ACCESS:
>> +        name = XATTR_NAME_POSIX_ACL_ACCESS;
>> +        break;
>> +    case ACL_TYPE_DEFAULT:
>> +        name = XATTR_NAME_POSIX_ACL_DEFAULT;
>> +        break;
>> +    default:
>> +        BUG();
>> +    }
>> +
>> +    size = ubifs_do_getxattr(inode, name, NULL, 0);
>> +    if (size > 0) {
>> +        value = kmalloc(size, GFP_KERNEL);
>> +        if (!value)
>> +            return ERR_PTR(-ENOMEM);
>> +        size = ubifs_do_getxattr(inode, name, value, size);
>> +    }
>> +    if (size > 0)
>> +        acl = posix_acl_from_xattr(&init_user_ns, value, size);
>> +    else if (size == -ENODATA)
>> +        acl = NULL;
>> +    else
>> +        acl = ERR_PTR(size);
>> +
>> +    kfree(value);
>> +    if (!IS_ERR(acl))
>> +        set_cached_acl(inode, type, acl);
>> +
>> +    return acl;
>> +}
>> +
>> +int ubifs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>> +{
>> +    char *name;
>> +    void *value = NULL;
>> +    size_t size = 0;
>> +    int flag = 0, err;
>> +
>> +    switch (type) {
>> +    case ACL_TYPE_ACCESS:
>> +        name = XATTR_NAME_POSIX_ACL_ACCESS;
>> +        if (acl) {
>> +            err = posix_acl_equiv_mode(acl, &inode->i_mode);
>> +            if (err < 0)
>> +                return err;
>> +            if (err == 0)
>> +                acl = NULL;
>> +        }
>> +        break;
>> +
>> +    case ACL_TYPE_DEFAULT:
>> +        name = XATTR_NAME_POSIX_ACL_DEFAULT;
>> +        if (!S_ISDIR(inode->i_mode))
>> +            return acl ? -EACCES : 0;
>> +        break;
>> +
>> +    default:
>> +        BUG();
>> +    }
>> +
>> +    if (acl) {
>> +        size = posix_acl_xattr_size(acl->a_count);
>> +        value = kmalloc(size, GFP_NOFS);
>> +        if (!value)
>> +            return -ENOMEM;
>> +
>> +        err = posix_acl_to_xattr(&init_user_ns, acl, value, size);
>> +        if (err < 0) {
>> +            kfree(value);
>> +            return err;
>> +        }
>> +    }
>> +
>> +    if (size == 0)
>> +        flag = XATTR_REPLACE;
>> +    err = ubifs_do_setxattr(inode, name, value, size, flag);
>> +
>> +    kfree(value);
>> +    if (!err)
>> +        set_cached_acl(inode, type, acl);
>> +
>> +    return err;
>> +}
>> +
>> +/*
>> + * Initialize the ACLs of a new inode.
>> + */
>> +int ubifs_init_acl(struct inode *dir, struct inode *inode)
>> +{
>> +    struct posix_acl *default_acl, *acl;
>> +    int err;
>> +
>> +    err = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
>> +    if (err)
>> +        return err;
>> +
>> +    if (default_acl) {
>> +        mutex_lock(&inode->i_mutex);
>> +        err = ubifs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
>> +        mutex_unlock(&inode->i_mutex);
>> +        posix_acl_release(default_acl);
>> +    }
>> +
>> +    if (acl) {
>> +        if (!err) {
>> +            mutex_lock(&inode->i_mutex);
>> +            err = ubifs_set_acl(inode, acl, ACL_TYPE_ACCESS);
>> +            mutex_unlock(&inode->i_mutex);
>> +        }
>> +        posix_acl_release(acl);
>> +    }
>> +
>> +    return err;
>> +}
>> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
>> index 62aa1a5..b9ddc8d 100644
>> --- a/fs/ubifs/ubifs.h
>> +++ b/fs/ubifs/ubifs.h
>> @@ -1767,6 +1767,20 @@ int ubifs_removexattr(struct dentry *dentry, const char *name);
>>   int ubifs_init_security(struct inode *dentry, struct inode *inode,
>>               const struct qstr *qstr);
>>
>> +/* acl.c */
>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>> +int ubifs_init_acl(struct inode *dir, struct inode *inode);
>> +int ubifs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
>> +struct posix_acl *ubifs_get_acl(struct inode *inode, int type);
>> +#else
>> +static inline int ubifs_init_acl(struct inode *inode, struct inode *dir)
>> +{
>> +    return 0;
>> +}
>> +#define ubifs_get_acl NULL
>> +#define ubifs_set_acl NULL
>> +#endif
>> +
>>   /* super.c */
>>   struct inode *ubifs_iget(struct super_block *sb, unsigned long inum);
>>
>>
> 
> 
> .
> 

Re: [RFC PATCH v3 1/5] UBIFS: ACL: introduce init/set/get functions for ACL

[Dongsheng Yang]

On 09/11/2015 02:13 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:
>>> This patch introduce a new file `acl.c', which implements:
>>>     * initializing ACL for new file according to the directory's default ACL,
>>>     * getting ACL which finds the ACL releated xattr and converts it to ACL,
>>>     * setting ACL function which converts ACL to xattr and creates/changes/
>>>       removes the xattr.
>>>
>>> On flash ACL format is based on POSIX ACL structures, and POSIX generic
>>> functions, posix_acl_[to|from]_xattr, are called to do the ACL conversion
>>> between in-memory and on-flash ACL.
>>>
>>> The ACL xattr handler is not implemented, because UBIFS does not use it.
>>
>> Why not, I think we can use it.
> First of all, thanks for your review :)
>
> It seems xattr handler will be removed because of dead code of security xattr.
> And if we do so, I think it's better to do that for all xattr, but not just
> security and ACL, and generic_[set|get]xattr should be set in inode_operations
> instead of ubifs_[get|set]xattr.

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.

That's not good idea. Please Just set the handler and call
generic_setxattr().

Yang
>
> thanks,
> Sheng
>>
>> Yang
>>>
>>> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
>>> ---
>>>    fs/ubifs/acl.c   | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>    fs/ubifs/ubifs.h |  14 ++++++
>>>    2 files changed, 155 insertions(+)
>>>    create mode 100644 fs/ubifs/acl.c
>>>
>>> diff --git a/fs/ubifs/acl.c b/fs/ubifs/acl.c
>>> new file mode 100644
>>> index 0000000..bf37875
>>> --- /dev/null
>>> +++ b/fs/ubifs/acl.c
>>> @@ -0,0 +1,141 @@
>>> +/*
>>> + * This file is part of UBIFS.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU General Public License version 2 as published by
>>> + * the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>>> + * more details.
>>> + *
>>> + */
>>> +
>>> +#include <linux/fs.h>
>>> +#include <linux/xattr.h>
>>> +#include <linux/posix_acl_xattr.h>
>>> +
>>> +#include "ubifs.h"
>>> +
>>> +struct posix_acl *ubifs_get_acl(struct inode *inode, int type)
>>> +{
>>> +    struct posix_acl *acl;
>>> +    char *name, *value = NULL;
>>> +    int size = 0;
>>> +
>>> +    switch (type) {
>>> +    case ACL_TYPE_ACCESS:
>>> +        name = XATTR_NAME_POSIX_ACL_ACCESS;
>>> +        break;
>>> +    case ACL_TYPE_DEFAULT:
>>> +        name = XATTR_NAME_POSIX_ACL_DEFAULT;
>>> +        break;
>>> +    default:
>>> +        BUG();
>>> +    }
>>> +
>>> +    size = ubifs_do_getxattr(inode, name, NULL, 0);
>>> +    if (size > 0) {
>>> +        value = kmalloc(size, GFP_KERNEL);
>>> +        if (!value)
>>> +            return ERR_PTR(-ENOMEM);
>>> +        size = ubifs_do_getxattr(inode, name, value, size);
>>> +    }
>>> +    if (size > 0)
>>> +        acl = posix_acl_from_xattr(&init_user_ns, value, size);
>>> +    else if (size == -ENODATA)
>>> +        acl = NULL;
>>> +    else
>>> +        acl = ERR_PTR(size);
>>> +
>>> +    kfree(value);
>>> +    if (!IS_ERR(acl))
>>> +        set_cached_acl(inode, type, acl);
>>> +
>>> +    return acl;
>>> +}
>>> +
>>> +int ubifs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>>> +{
>>> +    char *name;
>>> +    void *value = NULL;
>>> +    size_t size = 0;
>>> +    int flag = 0, err;
>>> +
>>> +    switch (type) {
>>> +    case ACL_TYPE_ACCESS:
>>> +        name = XATTR_NAME_POSIX_ACL_ACCESS;
>>> +        if (acl) {
>>> +            err = posix_acl_equiv_mode(acl, &inode->i_mode);
>>> +            if (err < 0)
>>> +                return err;
>>> +            if (err == 0)
>>> +                acl = NULL;
>>> +        }
>>> +        break;
>>> +
>>> +    case ACL_TYPE_DEFAULT:
>>> +        name = XATTR_NAME_POSIX_ACL_DEFAULT;
>>> +        if (!S_ISDIR(inode->i_mode))
>>> +            return acl ? -EACCES : 0;
>>> +        break;
>>> +
>>> +    default:
>>> +        BUG();
>>> +    }
>>> +
>>> +    if (acl) {
>>> +        size = posix_acl_xattr_size(acl->a_count);
>>> +        value = kmalloc(size, GFP_NOFS);
>>> +        if (!value)
>>> +            return -ENOMEM;
>>> +
>>> +        err = posix_acl_to_xattr(&init_user_ns, acl, value, size);
>>> +        if (err < 0) {
>>> +            kfree(value);
>>> +            return err;
>>> +        }
>>> +    }
>>> +
>>> +    if (size == 0)
>>> +        flag = XATTR_REPLACE;
>>> +    err = ubifs_do_setxattr(inode, name, value, size, flag);
>>> +
>>> +    kfree(value);
>>> +    if (!err)
>>> +        set_cached_acl(inode, type, acl);
>>> +
>>> +    return err;
>>> +}
>>> +
>>> +/*
>>> + * Initialize the ACLs of a new inode.
>>> + */
>>> +int ubifs_init_acl(struct inode *dir, struct inode *inode)
>>> +{
>>> +    struct posix_acl *default_acl, *acl;
>>> +    int err;
>>> +
>>> +    err = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
>>> +    if (err)
>>> +        return err;
>>> +
>>> +    if (default_acl) {
>>> +        mutex_lock(&inode->i_mutex);
>>> +        err = ubifs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
>>> +        mutex_unlock(&inode->i_mutex);
>>> +        posix_acl_release(default_acl);
>>> +    }
>>> +
>>> +    if (acl) {
>>> +        if (!err) {
>>> +            mutex_lock(&inode->i_mutex);
>>> +            err = ubifs_set_acl(inode, acl, ACL_TYPE_ACCESS);
>>> +            mutex_unlock(&inode->i_mutex);
>>> +        }
>>> +        posix_acl_release(acl);
>>> +    }
>>> +
>>> +    return err;
>>> +}
>>> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
>>> index 62aa1a5..b9ddc8d 100644
>>> --- a/fs/ubifs/ubifs.h
>>> +++ b/fs/ubifs/ubifs.h
>>> @@ -1767,6 +1767,20 @@ int ubifs_removexattr(struct dentry *dentry, const char *name);
>>>    int ubifs_init_security(struct inode *dentry, struct inode *inode,
>>>                const struct qstr *qstr);
>>>
>>> +/* acl.c */
>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>>> +int ubifs_init_acl(struct inode *dir, struct inode *inode);
>>> +int ubifs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
>>> +struct posix_acl *ubifs_get_acl(struct inode *inode, int type);
>>> +#else
>>> +static inline int ubifs_init_acl(struct inode *inode, struct inode *dir)
>>> +{
>>> +    return 0;
>>> +}
>>> +#define ubifs_get_acl NULL
>>> +#define ubifs_set_acl NULL
>>> +#endif
>>> +
>>>    /* super.c */
>>>    struct inode *ubifs_iget(struct super_block *sb, unsigned long inum);
>>>
>>>
>>
>>
>> .
>>
>
> .
>

Re: [RFC PATCH v3 1/5] UBIFS: ACL: introduce init/set/get functions for ACL

[Andreas Grünbacher]

2015-09-11 8:13 GMT+02:00 Sheng Yong <shengyong1@huawei.com>:
> It seems xattr handler will be removed because of dead code of security xattr.

The security xattr handler right now is dead code. If you can convert
ubifs to use the xattr handler infrastructure (super_block.s_xattr,
etc.), that would be great; the security xattr handler would then
start being useful. You could then implement POSIX ACLs as tmpfs does
(mm/shmem.c).

Andreas

[RFC PATCH v3 2/5] UBIFS: ACL: set ACL interfaces in inode_operations

[Sheng Yong]

Set [get|set]_acl interfaces in inode_operations for directory, file,
and symbol link.

Signed-off-by: Sheng Yong <shengyong1@huawei.com>
---
 fs/ubifs/dir.c  | 4 ++++
 fs/ubifs/file.c | 8 ++++++++
 2 files changed, 12 insertions(+)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 5c27c66..69ccc78 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -1188,6 +1188,10 @@ const struct inode_operations ubifs_dir_inode_operations = {
 	.getxattr    = ubifs_getxattr,
 	.listxattr   = ubifs_listxattr,
 	.removexattr = ubifs_removexattr,
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+	.get_acl     = ubifs_get_acl,
+	.set_acl     = ubifs_set_acl,
+#endif
 };
 
 const struct file_operations ubifs_dir_operations = {
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index a3dfe2a..58298f8 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1557,6 +1557,10 @@ const struct inode_operations ubifs_file_inode_operations = {
 	.getxattr    = ubifs_getxattr,
 	.listxattr   = ubifs_listxattr,
 	.removexattr = ubifs_removexattr,
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+	.get_acl     = ubifs_get_acl,
+	.set_acl     = ubifs_set_acl,
+#endif
 };
 
 const struct inode_operations ubifs_symlink_inode_operations = {
@@ -1568,6 +1572,10 @@ const struct inode_operations ubifs_symlink_inode_operations = {
 	.getxattr    = ubifs_getxattr,
 	.listxattr   = ubifs_listxattr,
 	.removexattr = ubifs_removexattr,
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+	.get_acl     = ubifs_get_acl,
+	.set_acl     = ubifs_set_acl,
+#endif
 };
 
 const struct file_operations ubifs_file_operations = {
-- 
1.9.1

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

[Sheng Yong]

* initialize ACL when file is created
* get ACL through ubifs_getxattr
* set ACL through ubifs_setxattr. If the size of ACL is 0, the ACL is going
  to be removed
* clear cached ACL in ubifs_removexattr if ACL xattr is removed
* modify ACL if file mode is changed

Signed-off-by: Sheng Yong <shengyong1@huawei.com>
---
 fs/ubifs/dir.c   |  16 ++++
 fs/ubifs/file.c  |   6 ++
 fs/ubifs/xattr.c | 243 ++++++++++++++++++++++++++++++++++---------------------
 3 files changed, 175 insertions(+), 90 deletions(-)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 69ccc78..db5dd45 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -270,6 +270,10 @@ static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
 		goto out_budg;
 	}
 
+	err = ubifs_init_acl(dir, inode);
+	if (err)
+		goto out_inode;
+
 	err = ubifs_init_security(dir, inode, &dentry->d_name);
 	if (err)
 		goto out_inode;
@@ -731,6 +735,10 @@ static int ubifs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 		goto out_budg;
 	}
 
+	err = ubifs_init_acl(dir, inode);
+	if (err)
+		goto out_inode;
+
 	err = ubifs_init_security(dir, inode, &dentry->d_name);
 	if (err)
 		goto out_inode;
@@ -810,6 +818,10 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry,
 		goto out_budg;
 	}
 
+	err = ubifs_init_acl(dir, inode);
+	if (err)
+		goto out_inode;
+
 	init_special_inode(inode, inode->i_mode, rdev);
 	inode->i_size = ubifs_inode(inode)->ui_size = devlen;
 	ui = ubifs_inode(inode);
@@ -898,6 +910,10 @@ static int ubifs_symlink(struct inode *dir, struct dentry *dentry,
 	ui->data_len = len;
 	inode->i_size = ubifs_inode(inode)->ui_size = len;
 
+	err = ubifs_init_acl(dir, inode);
+	if (err)
+		goto out_inode;
+
 	err = ubifs_init_security(dir, inode, &dentry->d_name);
 	if (err)
 		goto out_inode;
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 58298f8..74f4c63 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -52,6 +52,7 @@
 #include "ubifs.h"
 #include <linux/mount.h>
 #include <linux/slab.h>
+#include <linux/posix_acl.h>
 
 static int read_block(struct inode *inode, void *addr, unsigned int block,
 		      struct ubifs_data_node *dn)
@@ -1246,6 +1247,11 @@ static int do_setattr(struct ubifs_info *c, struct inode *inode,
 		mark_inode_dirty_sync(inode);
 	mutex_unlock(&ui->ui_mutex);
 
+	if (attr->ia_valid & ATTR_MODE)
+		err = posix_acl_chmod(inode, inode->i_mode);
+	if (err)
+		return err;
+
 	if (release)
 		ubifs_release_budget(c, &req);
 	if (IS_SYNC(inode))
diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index 6534b98..dad1070 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -52,7 +52,6 @@
  * in the VFS inode cache. The xentries are cached in the LNC cache (see
  * tnc.c).
  *
- * ACL support is not implemented.
  */
 
 #include "ubifs.h"
@@ -78,6 +77,10 @@ enum {
 	USER_XATTR,
 	TRUSTED_XATTR,
 	SECURITY_XATTR,
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+	POSIX_ACL_DEFAULT,
+	POSIX_ACL_ACCESS,
+#endif
 };
 
 static const struct inode_operations empty_iops;
@@ -276,6 +279,18 @@ static int check_namespace(const struct qstr *nm)
 		if (nm->name[sizeof(XATTR_SECURITY_PREFIX) - 1] == '\0')
 			return -EINVAL;
 		type = SECURITY_XATTR;
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+	} else if (!strncmp(nm->name, XATTR_NAME_POSIX_ACL_DEFAULT,
+			    sizeof(XATTR_NAME_POSIX_ACL_DEFAULT) - 1)) {
+		if (nm->name[sizeof(XATTR_NAME_POSIX_ACL_DEFAULT) - 1] != '\0')
+			return -EINVAL;
+		type = POSIX_ACL_DEFAULT;
+	} else if (!strncmp(nm->name, XATTR_NAME_POSIX_ACL_ACCESS,
+			    sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1)) {
+		if (nm->name[sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1] != '\0')
+			return -EINVAL;
+		type = POSIX_ACL_ACCESS;
+#endif
 	} else
 		return -EOPNOTSUPP;
 
@@ -299,6 +314,105 @@ static struct inode *iget_xattr(struct ubifs_info *c, ino_t inum)
 	return ERR_PTR(-EINVAL);
 }
 
+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 type, err;
+
+	dbg_gen("xattr '%s', ino %lu ('%pd')", name,
+		host->i_ino, dentry);
+	ubifs_assert(mutex_is_locked(&host->i_mutex));
+
+	type = check_namespace(&nm);
+	if (type < 0)
+		return type;
+
+	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);
+
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+	if (!err) {
+		if (type == POSIX_ACL_DEFAULT)
+			set_cached_acl(host, ACL_TYPE_DEFAULT, NULL);
+		else if (type == POSIX_ACL_ACCESS)
+			set_cached_acl(host, ACL_TYPE_ACCESS, NULL);
+	}
+#endif
+
+	/* If @i_nlink is 0, 'iput()' will delete the inode */
+	iput(inode);
+
+out_free:
+	kfree(xent);
+	return err;
+}
+
+
 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
 	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
 	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
 	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)
 {
-- 
1.9.1

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

[Dongsheng Yang]

On 09/11/2015 05:09 PM, Sheng Yong wrote:
> * initialize ACL when file is created
> * get ACL through ubifs_getxattr
> * set ACL through ubifs_setxattr. If the size of ACL is 0, the ACL is going
>    to be removed
> * clear cached ACL in ubifs_removexattr if ACL xattr is removed
> * modify ACL if file mode is changed
>
> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
> ---
>   fs/ubifs/dir.c   |  16 ++++
>   fs/ubifs/file.c  |   6 ++
>   fs/ubifs/xattr.c | 243 ++++++++++++++++++++++++++++++++++---------------------
>   3 files changed, 175 insertions(+), 90 deletions(-)
>
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 69ccc78..db5dd45 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -270,6 +270,10 @@ static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
>   		goto out_budg;
>   	}
>
> +	err = ubifs_init_acl(dir, inode);
> +	if (err)
> +		goto out_inode;
> +
>   	err = ubifs_init_security(dir, inode, &dentry->d_name);
>   	if (err)
>   		goto out_inode;
> @@ -731,6 +735,10 @@ static int ubifs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
>   		goto out_budg;
>   	}
>
> +	err = ubifs_init_acl(dir, inode);
> +	if (err)
> +		goto out_inode;
> +
>   	err = ubifs_init_security(dir, inode, &dentry->d_name);
>   	if (err)
>   		goto out_inode;
> @@ -810,6 +818,10 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry,
>   		goto out_budg;
>   	}
>
> +	err = ubifs_init_acl(dir, inode);
> +	if (err)
> +		goto out_inode;
> +
>   	init_special_inode(inode, inode->i_mode, rdev);
>   	inode->i_size = ubifs_inode(inode)->ui_size = devlen;
>   	ui = ubifs_inode(inode);
> @@ -898,6 +910,10 @@ static int ubifs_symlink(struct inode *dir, struct dentry *dentry,
>   	ui->data_len = len;
>   	inode->i_size = ubifs_inode(inode)->ui_size = len;
>
> +	err = ubifs_init_acl(dir, inode);
> +	if (err)
> +		goto out_inode;
> +
>   	err = ubifs_init_security(dir, inode, &dentry->d_name);
>   	if (err)
>   		goto out_inode;
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index 58298f8..74f4c63 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -52,6 +52,7 @@
>   #include "ubifs.h"
>   #include <linux/mount.h>
>   #include <linux/slab.h>
> +#include <linux/posix_acl.h>
>
>   static int read_block(struct inode *inode, void *addr, unsigned int block,
>   		      struct ubifs_data_node *dn)
> @@ -1246,6 +1247,11 @@ static int do_setattr(struct ubifs_info *c, struct inode *inode,
>   		mark_inode_dirty_sync(inode);
>   	mutex_unlock(&ui->ui_mutex);
>
> +	if (attr->ia_valid & ATTR_MODE)
> +		err = posix_acl_chmod(inode, inode->i_mode);
> +	if (err)
> +		return err;
> +
>   	if (release)
>   		ubifs_release_budget(c, &req);
>   	if (IS_SYNC(inode))
> diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
> index 6534b98..dad1070 100644
> --- a/fs/ubifs/xattr.c
> +++ b/fs/ubifs/xattr.c
> @@ -52,7 +52,6 @@
>    * in the VFS inode cache. The xentries are cached in the LNC cache (see
>    * tnc.c).
>    *
> - * ACL support is not implemented.
>    */
>
>   #include "ubifs.h"
> @@ -78,6 +77,10 @@ enum {
>   	USER_XATTR,
>   	TRUSTED_XATTR,
>   	SECURITY_XATTR,
> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
> +	POSIX_ACL_DEFAULT,
> +	POSIX_ACL_ACCESS,
> +#endif
>   };
>
>   static const struct inode_operations empty_iops;
> @@ -276,6 +279,18 @@ static int check_namespace(const struct qstr *nm)
>   		if (nm->name[sizeof(XATTR_SECURITY_PREFIX) - 1] == '\0')
>   			return -EINVAL;
>   		type = SECURITY_XATTR;
> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
> +	} else if (!strncmp(nm->name, XATTR_NAME_POSIX_ACL_DEFAULT,
> +			    sizeof(XATTR_NAME_POSIX_ACL_DEFAULT) - 1)) {
> +		if (nm->name[sizeof(XATTR_NAME_POSIX_ACL_DEFAULT) - 1] != '\0')
> +			return -EINVAL;
> +		type = POSIX_ACL_DEFAULT;
> +	} else if (!strncmp(nm->name, XATTR_NAME_POSIX_ACL_ACCESS,
> +			    sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1)) {
> +		if (nm->name[sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1] != '\0')
> +			return -EINVAL;
> +		type = POSIX_ACL_ACCESS;
> +#endif
>   	} else
>   		return -EOPNOTSUPP;
>
> @@ -299,6 +314,105 @@ static struct inode *iget_xattr(struct ubifs_info *c, ino_t inum)
>   	return ERR_PTR(-EINVAL);
>   }
>
> +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 type, err;
> +
> +	dbg_gen("xattr '%s', ino %lu ('%pd')", name,
> +		host->i_ino, dentry);
> +	ubifs_assert(mutex_is_locked(&host->i_mutex));
> +
> +	type = check_namespace(&nm);
> +	if (type < 0)
> +		return type;
> +
> +	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);
> +
> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
> +	if (!err) {
> +		if (type == POSIX_ACL_DEFAULT)
> +			set_cached_acl(host, ACL_TYPE_DEFAULT, NULL);
> +		else if (type == POSIX_ACL_ACCESS)
> +			set_cached_acl(host, ACL_TYPE_ACCESS, NULL);
> +	}
> +#endif
> +
> +	/* If @i_nlink is 0, 'iput()' will delete the inode */
> +	iput(inode);
> +
> +out_free:
> +	kfree(xent);
> +	return err;
> +}
> +
> +

Why move it? If you just want to use them before the definitions,
Just declare them before using.
>   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?
>   	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?
>   	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)
>   {
>

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

[Sheng Yong]

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.

>>       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.

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)
>>   {
>>
> 
> 
> .
> 

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

[Dongsheng Yang]

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?
>
>>>        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.

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);

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)
>>>    {
>>>
>>
>>
>> .
>>
>
> .
>

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

[Sheng Yong]

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 :)

> 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,
 };

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 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.

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)
>>>>    {
>>>>
>>>
>>>
>>> .
>>>
>>
>> .
>>
> 
> 
> .
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [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)
>>>>>     {
>>>>>
>>>>
>>>>
>>>> .
>>>>
>>>
>>> .
>>>
>>
>>
>> .
>>
>
> .
>

[RFC PATCH v3 4/5] UBIFS: ACL: introduce ACL mount options

[Sheng Yong]

This patch introduces `acl' and `noacl' mount options for ACL.

Signed-off-by: Sheng Yong <shengyong1@huawei.com>
---
 fs/ubifs/super.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 9547a278..52baad1 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -441,6 +441,9 @@ static int ubifs_show_options(struct seq_file *s, struct dentry *root)
 			   ubifs_compr_name(c->mount_opts.compr_type));
 	}
 
+	if (c->vfs_sb->s_flags & MS_POSIXACL)
+		seq_printf(s, ",acl");
+
 	return 0;
 }
 
@@ -926,6 +929,8 @@ enum {
 	Opt_chk_data_crc,
 	Opt_no_chk_data_crc,
 	Opt_override_compr,
+	Opt_acl,
+	Opt_noacl,
 	Opt_err,
 };
 
@@ -937,6 +942,8 @@ static const match_table_t tokens = {
 	{Opt_chk_data_crc, "chk_data_crc"},
 	{Opt_no_chk_data_crc, "no_chk_data_crc"},
 	{Opt_override_compr, "compr=%s"},
+	{Opt_acl, "acl"},
+	{Opt_noacl, "noacl"},
 	{Opt_err, NULL},
 };
 
@@ -1037,6 +1044,14 @@ static int ubifs_parse_options(struct ubifs_info *c, char *options,
 			c->default_compr = c->mount_opts.compr_type;
 			break;
 		}
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+		case Opt_acl:
+			c->vfs_sb->s_flags |= MS_POSIXACL;
+			break;
+		case Opt_noacl:
+			c->vfs_sb->s_flags &= ~MS_POSIXACL;
+			break;
+#endif
 		default:
 		{
 			unsigned long flag;
-- 
1.9.1

Re: [RFC PATCH v3 4/5] UBIFS: ACL: introduce ACL mount options

[Dongsheng Yang]

On 09/11/2015 05:09 PM, Sheng Yong wrote:
> This patch introduces `acl' and `noacl' mount options for ACL.
>
> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
> ---
>   fs/ubifs/super.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 9547a278..52baad1 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -441,6 +441,9 @@ static int ubifs_show_options(struct seq_file *s, struct dentry *root)
>   			   ubifs_compr_name(c->mount_opts.compr_type));
>   	}
>
> +	if (c->vfs_sb->s_flags & MS_POSIXACL)
> +		seq_printf(s, ",acl");
> +
>   	return 0;
>   }
>
> @@ -926,6 +929,8 @@ enum {
>   	Opt_chk_data_crc,
>   	Opt_no_chk_data_crc,
>   	Opt_override_compr,
> +	Opt_acl,
> +	Opt_noacl,
>   	Opt_err,
>   };
>
> @@ -937,6 +942,8 @@ static const match_table_t tokens = {
>   	{Opt_chk_data_crc, "chk_data_crc"},
>   	{Opt_no_chk_data_crc, "no_chk_data_crc"},
>   	{Opt_override_compr, "compr=%s"},
> +	{Opt_acl, "acl"},
> +	{Opt_noacl, "noacl"},
>   	{Opt_err, NULL},
>   };
>
> @@ -1037,6 +1044,14 @@ static int ubifs_parse_options(struct ubifs_info *c, char *options,
>   			c->default_compr = c->mount_opts.compr_type;
>   			break;
>   		}
> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
> +		case Opt_acl:
> +			c->vfs_sb->s_flags |= MS_POSIXACL;
> +			break;

I think we can error out if CONFIG_UBIFS_FS_POSIX_ACL=n && Opt_acl is 
specified. I think you missed my comment again, I mentioned it in your 
V2 patch.

Yang
> +		case Opt_noacl:
> +			c->vfs_sb->s_flags &= ~MS_POSIXACL;
> +			break;
> +#endif
>   		default:
>   		{
>   			unsigned long flag;
>

Re: [RFC PATCH v3 4/5] UBIFS: ACL: introduce ACL mount options

[Sheng Yong]

On 9/11/2015 1:03 PM, Dongsheng Yang wrote:
> On 09/11/2015 05:09 PM, Sheng Yong wrote:
>> This patch introduces `acl' and `noacl' mount options for ACL.
>>
>> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
>> ---
>>   fs/ubifs/super.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
>> index 9547a278..52baad1 100644
>> --- a/fs/ubifs/super.c
>> +++ b/fs/ubifs/super.c
>> @@ -441,6 +441,9 @@ static int ubifs_show_options(struct seq_file *s, struct dentry *root)
>>                  ubifs_compr_name(c->mount_opts.compr_type));
>>       }
>>
>> +    if (c->vfs_sb->s_flags & MS_POSIXACL)
>> +        seq_printf(s, ",acl");
>> +
>>       return 0;
>>   }
>>
>> @@ -926,6 +929,8 @@ enum {
>>       Opt_chk_data_crc,
>>       Opt_no_chk_data_crc,
>>       Opt_override_compr,
>> +    Opt_acl,
>> +    Opt_noacl,
>>       Opt_err,
>>   };
>>
>> @@ -937,6 +942,8 @@ static const match_table_t tokens = {
>>       {Opt_chk_data_crc, "chk_data_crc"},
>>       {Opt_no_chk_data_crc, "no_chk_data_crc"},
>>       {Opt_override_compr, "compr=%s"},
>> +    {Opt_acl, "acl"},
>> +    {Opt_noacl, "noacl"},
>>       {Opt_err, NULL},
>>   };
>>
>> @@ -1037,6 +1044,14 @@ static int ubifs_parse_options(struct ubifs_info *c, char *options,
>>               c->default_compr = c->mount_opts.compr_type;
>>               break;
>>           }
>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>> +        case Opt_acl:
>> +            c->vfs_sb->s_flags |= MS_POSIXACL;
>> +            break;
> 
> I think we can error out if CONFIG_UBIFS_FS_POSIX_ACL=n && Opt_acl is specified. I think you missed my comment again, I mentioned it in your V2 patch.

I don't think the else is needed. I think the `default' could handle this.

thanks,
Sheng
> 
> Yang
>> +        case Opt_noacl:
>> +            c->vfs_sb->s_flags &= ~MS_POSIXACL;
>> +            break;
>> +#endif
>>           default:
>>           {
>>               unsigned long flag;
>>
> 
> 
> .
> 

Re: [RFC PATCH v3 4/5] UBIFS: ACL: introduce ACL mount options

[Dongsheng Yang]

On 09/11/2015 04:25 PM, Sheng Yong wrote:
>
>
> On 9/11/2015 1:03 PM, Dongsheng Yang wrote:
>> On 09/11/2015 05:09 PM, Sheng Yong wrote:
>>> This patch introduces `acl' and `noacl' mount options for ACL.
>>>
>>> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
>>> ---
>>>    fs/ubifs/super.c | 15 +++++++++++++++
>>>    1 file changed, 15 insertions(+)
>>>
>>> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
>>> index 9547a278..52baad1 100644
>>> --- a/fs/ubifs/super.c
>>> +++ b/fs/ubifs/super.c
>>> @@ -441,6 +441,9 @@ static int ubifs_show_options(struct seq_file *s, struct dentry *root)
>>>                   ubifs_compr_name(c->mount_opts.compr_type));
>>>        }
>>>
>>> +    if (c->vfs_sb->s_flags & MS_POSIXACL)
>>> +        seq_printf(s, ",acl");
>>> +
>>>        return 0;
>>>    }
>>>
>>> @@ -926,6 +929,8 @@ enum {
>>>        Opt_chk_data_crc,
>>>        Opt_no_chk_data_crc,
>>>        Opt_override_compr,
>>> +    Opt_acl,
>>> +    Opt_noacl,
>>>        Opt_err,
>>>    };
>>>
>>> @@ -937,6 +942,8 @@ static const match_table_t tokens = {
>>>        {Opt_chk_data_crc, "chk_data_crc"},
>>>        {Opt_no_chk_data_crc, "no_chk_data_crc"},
>>>        {Opt_override_compr, "compr=%s"},
>>> +    {Opt_acl, "acl"},
>>> +    {Opt_noacl, "noacl"},
>>>        {Opt_err, NULL},
>>>    };
>>>
>>> @@ -1037,6 +1044,14 @@ static int ubifs_parse_options(struct ubifs_info *c, char *options,
>>>                c->default_compr = c->mount_opts.compr_type;
>>>                break;
>>>            }
>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>>> +        case Opt_acl:
>>> +            c->vfs_sb->s_flags |= MS_POSIXACL;
>>> +            break;
>>
>> I think we can error out if CONFIG_UBIFS_FS_POSIX_ACL=n && Opt_acl is specified. I think you missed my comment again, I mentioned it in your V2 patch.
>
> I don't think the else is needed. I think the `default' could handle this.

Oh, yes, good. My bad. default would handle it.

Thanx
Yang
>
> thanks,
> Sheng
>>
>> Yang
>>> +        case Opt_noacl:
>>> +            c->vfs_sb->s_flags &= ~MS_POSIXACL;
>>> +            break;
>>> +#endif
>>>            default:
>>>            {
>>>                unsigned long flag;
>>>
>>
>>
>> .
>>
>
> .
>

[RFC PATCH v3 5/5] UBIFS: ACL: add ACL config option

[Sheng Yong]

Add CONFIG_UBIFS_FS_POSIX_ACL to select ACL for UBIFS.

Signed-off-by: Sheng Yong <shengyong1@huawei.com>
---
 fs/ubifs/Kconfig  | 11 +++++++++++
 fs/ubifs/Makefile |  1 +
 2 files changed, 12 insertions(+)

diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig
index ba66d50..3ca8e8d 100644
--- a/fs/ubifs/Kconfig
+++ b/fs/ubifs/Kconfig
@@ -35,3 +35,14 @@ config UBIFS_FS_ZLIB
 	default y
 	help
 	  Zlib compresses better than LZO but it is slower. Say 'Y' if unsure.
+
+config UBIFS_FS_POSIX_ACL
+	bool "UBIFS POSIX Access Control Lists"
+	depends on UBIFS_FS
+	select FS_POSIX_ACL
+	help
+	  Posix Access Control Lists (ACLs) support permissions for users and
+	  groups beyond the owner/group/world scheme.
+
+	  To learn more about Access Control Lists, visit the Posix ACLs for
+	  Linux website <http://acl.bestbits.at/>.
diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile
index 2c6f0cb..ab8aea5 100644
--- a/fs/ubifs/Makefile
+++ b/fs/ubifs/Makefile
@@ -4,3 +4,4 @@ ubifs-y += shrinker.o journal.o file.o dir.o super.o sb.o io.o
 ubifs-y += tnc.o master.o scan.o replay.o log.o commit.o gc.o orphan.o
 ubifs-y += budget.o find.o tnc_commit.o compress.o lpt.o lprops.o
 ubifs-y += recovery.o ioctl.o lpt_commit.o tnc_misc.o xattr.o debug.o
+ubifs-$(CONFIG_UBIFS_FS_POSIX_ACL) += acl.o
-- 
1.9.1