ubifs: implement POSIX Access Control Lists (ACLs)

Message ID	1476046382-19185-1-git-send-email-pascal.eberhard@gmail.com
State	Deferred, archived
Delegated to:	Richard Weinberger
Headers	show Return-Path: <linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org> From: Pascal Eberhard <pascal.eberhard@gmail.com> To: richard@nod.at Subject: [PATCH] ubifs: implement POSIX Access Control Lists (ACLs) Date: Sun, 9 Oct 2016 22:53:02 +0200 Message-Id: <1476046382-19185-1-git-send-email-pascal.eberhard@gmail.com> summary: Content analysis details: (-2.7 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [2a00:1450:4010:c07:0:0:0:242 listed in] [list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (pascal.eberhard[at]gmail.com) -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid Precedence: list Cc: Pascal Eberhard <pascal.eberhard@gmail.com>, linux-mtd@lists.infradead.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-mtd" <linux-mtd-bounces@lists.infradead.org> Errors-To: linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org

Pascal Eberhard Oct. 9, 2016, 8:53 p.m. UTC

This patch implements POSIX Access Control Lists (ACLs) in UBIFS.
It uses posix_acl_(access|default)_xattr_handler ACL generic handlers
as xattr handlers are now used in UBIFS.

Signed-off-by: Pascal Eberhard <pascal.eberhard@gmail.com>
---
 fs/ubifs/Kconfig  |  13 +++++
 fs/ubifs/Makefile |   1 +
 fs/ubifs/acl.c    | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ubifs/dir.c    |  22 +++++++++
 fs/ubifs/file.c   |   9 ++++
 fs/ubifs/super.c  |  15 ++++++
 fs/ubifs/ubifs.h  |  18 +++++++
 fs/ubifs/xattr.c  |  11 +++--
 8 files changed, 229 insertions(+), 3 deletions(-)
 create mode 100644 fs/ubifs/acl.c

Pascal Eberhard Oct. 9, 2016, 9:03 p.m. UTC | #1

Hi,


On 2016-10-09 22:53, Pascal Eberhard wrote:
> This patch implements POSIX Access Control Lists (ACLs) in UBIFS.

The history of this patch might be of interest:
- I am running UBIFS with ACL for 2 years with kernel 3.2. The goal
   now is to -finally- get in sync with mainline.
   APIs evolved a lot since 3.2, so porting this initial code to
   mainline did not really make sense as 80% of the code is now
   implemented in a generic manner.
- As well, I was pleased to see Sheng Yong patch proposal in 2015 in
   ML archives, but it seems it has not been finalized.

So this work is based on actual btrfs implementation and 'inspired'
from Sheng Yong proposal, related discussions, as well as my 3.2
initial code.

This patch has been tested with:
- LTP fs/acl/tacl_xattr.sh (modified version),
- LTP fs/acls/acl_test01 (modified version),
- an ACL test suite made during initial 3.2 development.


Best regards,

Pascal

Richard Weinberger Oct. 18, 2016, 9:32 p.m. UTC | #2

Pascal,

On 09.10.2016 22:53, Pascal Eberhard wrote:
> This patch implements POSIX Access Control Lists (ACLs) in UBIFS.
> It uses posix_acl_(access|default)_xattr_handler ACL generic handlers
> as xattr handlers are now used in UBIFS.

Cool! :)
CC'ing Andreas, maybe he also has comments.

> Signed-off-by: Pascal Eberhard <pascal.eberhard@gmail.com>
> ---
>  fs/ubifs/Kconfig  |  13 +++++
>  fs/ubifs/Makefile |   1 +
>  fs/ubifs/acl.c    | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/ubifs/dir.c    |  22 +++++++++
>  fs/ubifs/file.c   |   9 ++++
>  fs/ubifs/super.c  |  15 ++++++
>  fs/ubifs/ubifs.h  |  18 +++++++
>  fs/ubifs/xattr.c  |  11 +++--
>  8 files changed, 229 insertions(+), 3 deletions(-)
>  create mode 100644 fs/ubifs/acl.c
> 
> diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig
> index 7ff7712..39119e5 100644
> --- a/fs/ubifs/Kconfig
> +++ b/fs/ubifs/Kconfig
> @@ -50,3 +50,16 @@ config UBIFS_ATIME_SUPPORT
>  	  strictatime is the "heavy", relatime is "lighter", etc.
>  
>  	  If unsure, say 'N'
> +
> +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/>.
> +
> +	  If you don't know what Access Control Lists are, say N
> diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile
> index c54a243..af7a83f 100644
> --- a/fs/ubifs/Makefile
> +++ b/fs/ubifs/Makefile
> @@ -5,3 +5,4 @@ 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-y += misc.o
> +ubifs-$(CONFIG_UBIFS_FS_POSIX_ACL) += acl.o
> \ No newline at end of file
> diff --git a/fs/ubifs/acl.c b/fs/ubifs/acl.c
> new file mode 100644
> index 0000000..a4d4734
> --- /dev/null
> +++ b/fs/ubifs/acl.c
> @@ -0,0 +1,143 @@
> +/*
> + * 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>

ubifs.h includes this already.

> +#include <linux/xattr.h>

Ditto.

> +#include <linux/posix_acl_xattr.h>
> +
> +#include "ubifs.h"
> +
> +struct posix_acl *ubifs_get_acl(struct inode *inode, int type)
> +{
> +	int size;
> +	const char *name;
> +	char *value = NULL;
> +	struct posix_acl *acl;
> +
> +	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:
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	size = __ubifs_getxattr(inode, name, NULL, 0);
> +	if (size > 0) {
> +		value = kzalloc(size, GFP_KERNEL);
> +		if (!value)
> +			return ERR_PTR(-ENOMEM);
> +		size = __ubifs_getxattr(inode, name, value, size);
> +	}
> +	if (size > 0)
> +		acl = posix_acl_from_xattr(&init_user_ns, value, size);
> +	else if (size == -ERANGE || size == -ENODATA || size == 0)
> +		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)
> +{
> +	int ret;
> +	int size = 0;
> +	const char *name;
> +	char *value = NULL;
> +	int flags = 0;
> +
> +	switch (type) {
> +	case ACL_TYPE_ACCESS:
> +		name = XATTR_NAME_POSIX_ACL_ACCESS;
> +		if (acl) {
> +			ret = posix_acl_equiv_mode(acl, &inode->i_mode);
> +			if (ret < 0)
> +				return ret;
> +			if (ret == 0)
> +				acl = NULL;
> +		}
> +		ret = 0;
> +		break;
> +	case ACL_TYPE_DEFAULT:
> +		if (!S_ISDIR(inode->i_mode))
> +			return acl ? -EINVAL : 0;
> +		name = XATTR_NAME_POSIX_ACL_DEFAULT;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (acl) {
> +		size = posix_acl_xattr_size(acl->a_count);
> +		value = kmalloc(size, GFP_KERNEL);
> +		if (!value)
> +			return -ENOMEM;
> +
> +		ret = posix_acl_to_xattr(&init_user_ns, acl, value, size);
> +		if (ret < 0)
> +			goto out;
> +	}
> +
> +	if (size == 0)
> +		flags = XATTR_REPLACE;
> +	ret = __ubifs_setxattr(inode, name, value, size, flags);
> +
> +out:
> +	kfree(value);
> +	if (!ret)
> +		set_cached_acl(inode, type, acl);
> +
> +	return ret;
> +}
> +
> +/*
> + * Initialize the ACLs of a new inode.

Please use the Javadoc style we use for all UBIFS functions.
(applies to other functions too in this patch)-

> + */
> +int ubifs_init_acl(struct inode *dir, struct inode *inode)
> +{
> +	struct posix_acl *default_acl, *acl;
> +	int ret = 0;
> +
> +	ret = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
> +	if (ret)
> +		return ret;
> +
> +	if (default_acl) {
> +		inode_lock(inode);
> +		ret = ubifs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
> +		inode_unlock(inode);
> +		posix_acl_release(default_acl);
> +	}
> +
> +	if (acl) {
> +		if (!ret) {
> +			inode_lock(inode);
> +			ret = ubifs_set_acl(inode, acl, ACL_TYPE_ACCESS);
> +			inode_unlock(inode);
> +		}
> +		posix_acl_release(acl);
> +	}
> +
> +	if (!default_acl && !acl)
> +		cache_no_acl(inode);
> +	return ret;
> +}
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index ccd9128..a2e5609 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;
> @@ -341,6 +345,10 @@ static int do_tmpfile(struct inode *dir, struct dentry *dentry,
>  		ubifs_assert(inode->i_op == &ubifs_file_inode_operations);
>  	}
>  
> +	err = ubifs_init_acl(dir, inode);
> +	if (err)
> +		goto out_inode;

Why do we need ACLs for O_TMPFILE?

> +
>  	err = ubifs_init_security(dir, inode, &dentry->d_name);
>  	if (err)
>  		goto out_inode;
> @@ -821,6 +829,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;
> @@ -903,6 +915,10 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry,
>  	ui->data = dev;
>  	ui->data_len = devlen;
>  
> +	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;
> @@ -985,6 +1001,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;

Do we really need ACLs for symlinks too? I thought permissions for symlinks are irrelevant.

>  	err = ubifs_init_security(dir, inode, &dentry->d_name);
>  	if (err)
>  		goto out_inode;
> @@ -1395,6 +1415,8 @@ const struct inode_operations ubifs_dir_inode_operations = {
>  	.update_time = ubifs_update_time,
>  #endif
>  	.tmpfile     = ubifs_tmpfile,
> +	.get_acl     = ubifs_get_acl,
> +	.set_acl     = ubifs_set_acl,
>  };
>  
>  const struct file_operations ubifs_dir_operations = {
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index 4ed270b..17f6098 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -53,6 +53,7 @@
>  #include <linux/mount.h>
>  #include <linux/slab.h>
>  #include <linux/migrate.h>
> +#include <linux/posix_acl.h>
>  
>  static int read_block(struct inode *inode, void *addr, unsigned int block,
>  		      struct ubifs_data_node *dn)
> @@ -1251,6 +1252,10 @@ static int do_setattr(struct ubifs_info *c, struct inode *inode,
>  		ubifs_release_budget(c, &req);
>  	if (IS_SYNC(inode))
>  		err = inode->i_sb->s_op->write_inode(inode, NULL);
> +
> +	if (!err && (attr->ia_valid & ATTR_MODE))
> +		err = posix_acl_chmod(inode, inode->i_mode);
> +
>  	return err;
>  }
>  
> @@ -1628,6 +1633,8 @@ const struct inode_operations ubifs_file_inode_operations = {
>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>  	.update_time = ubifs_update_time,
>  #endif
> +	.get_acl     = ubifs_get_acl,
> +	.set_acl     = ubifs_set_acl,
>  };
>  
>  const struct inode_operations ubifs_symlink_inode_operations = {
> @@ -1642,6 +1649,8 @@ const struct inode_operations ubifs_symlink_inode_operations = {
>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>  	.update_time = ubifs_update_time,
>  #endif
> +	.get_acl     = ubifs_get_acl,
> +	.set_acl     = ubifs_set_acl,
>  };
>  
>  const struct file_operations ubifs_file_operations = {
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 4ec0510..8239dfd 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -444,6 +444,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_puts(s, ",acl");
> +
>  	return 0;
>  }
>  
> @@ -929,6 +932,8 @@ enum {
>  	Opt_chk_data_crc,
>  	Opt_no_chk_data_crc,
>  	Opt_override_compr,
> +	Opt_acl,
> +	Opt_noacl,
>  	Opt_err,
>  };
>  
> @@ -940,6 +945,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},
>  };
>  
> @@ -1040,6 +1047,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;
> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> index 096035e..7935449 100644
> --- a/fs/ubifs/ubifs.h
> +++ b/fs/ubifs/ubifs.h
> @@ -1743,6 +1743,24 @@ extern const struct xattr_handler *ubifs_xattr_handlers[];
>  ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size);
>  int ubifs_init_security(struct inode *dentry, struct inode *inode,
>  			const struct qstr *qstr);
> +ssize_t __ubifs_getxattr(struct inode *host, const char *name,
> +				void *buf, size_t size);
> +int __ubifs_setxattr(struct inode *host, const char *name,
> +			    const void *value, size_t size, int flags);
> +
> +/* 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);
> diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
> index 6c2f4d4..0a596a1 100644
> --- a/fs/ubifs/xattr.c
> +++ b/fs/ubifs/xattr.c
> @@ -52,13 +52,14 @@
>   * in the VFS inode cache. The xentries are cached in the LNC cache (see
>   * tnc.c).
>   *
> - * ACL support is not implemented.
> + * ACL support is now implemented.

I think we can just drop that line now. :-)

>   */
>  
>  #include "ubifs.h"
>  #include <linux/fs.h>
>  #include <linux/slab.h>
>  #include <linux/xattr.h>
> +#include <linux/posix_acl_xattr.h>

While we are here, these includes should also be cleaned up.

>  /*
>   * Limit the number of extended attributes per inode so that the total size
> @@ -268,7 +269,7 @@ static struct inode *iget_xattr(struct ubifs_info *c, ino_t inum)
>  	return ERR_PTR(-EINVAL);
>  }
>  
> -static int __ubifs_setxattr(struct inode *host, const char *name,
> +int __ubifs_setxattr(struct inode *host, const char *name,
>  			    const void *value, size_t size, int flags)
>  {
>  	struct inode *inode;
> @@ -328,7 +329,7 @@ out_free:
>  	return err;
>  }
>  
> -static ssize_t __ubifs_getxattr(struct inode *host, const char *name,
> +ssize_t __ubifs_getxattr(struct inode *host, const char *name,
>  				void *buf, size_t size)
>  {
>  	struct inode *inode;
> @@ -619,5 +620,9 @@ const struct xattr_handler *ubifs_xattr_handlers[] = {
>  	&ubifs_user_xattr_handler,
>  	&ubifs_trusted_xattr_handler,
>  	&ubifs_security_xattr_handler,
> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
> +	&posix_acl_access_xattr_handler,
> +	&posix_acl_default_xattr_handler,
> +#endif
>  	NULL
>  };
> 

Overall the patch does not look bad and I'm sure we can add it for v4.10 soon.
I'm a bit confused since most stuff is not really UBIFS specific.
Andreas, is just "normal" for ACL support? I thought most code is generic and already in VFS.

Thanks,
//richard

Pascal Eberhard Oct. 18, 2016, 11:54 p.m. UTC | #3

Richard,

Thanks for your review.

On 2016-10-18 23:32, Richard Weinberger wrote:
> Pascal,
> 
> On 09.10.2016 22:53, Pascal Eberhard wrote:
>> This patch implements POSIX Access Control Lists (ACLs) in UBIFS.
>> It uses posix_acl_(access|default)_xattr_handler ACL generic handlers
>> as xattr handlers are now used in UBIFS.
> 
> Cool! :)
> CC'ing Andreas, maybe he also has comments.
> 
>> Signed-off-by: Pascal Eberhard <pascal.eberhard@gmail.com>
>> ---
>>  fs/ubifs/Kconfig  |  13 +++++
>>  fs/ubifs/Makefile |   1 +
>>  fs/ubifs/acl.c    | 143 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  fs/ubifs/dir.c    |  22 +++++++++
>>  fs/ubifs/file.c   |   9 ++++
>>  fs/ubifs/super.c  |  15 ++++++
>>  fs/ubifs/ubifs.h  |  18 +++++++
>>  fs/ubifs/xattr.c  |  11 +++--
>>  8 files changed, 229 insertions(+), 3 deletions(-)
>>  create mode 100644 fs/ubifs/acl.c
>> 

[...]

>> +#include <linux/fs.h>
> 
> ubifs.h includes this already.
> 

ok

>> +#include <linux/xattr.h>
> 
> Ditto.
> 

ok

[...]

>> +/*
>> + * Initialize the ACLs of a new inode.
> 
> Please use the Javadoc style we use for all UBIFS functions.
> (applies to other functions too in this patch)-

ok

[...]

>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>> index ccd9128..a2e5609 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;
>> @@ -341,6 +345,10 @@ static int do_tmpfile(struct inode *dir, struct 
>> dentry *dentry,
>>  		ubifs_assert(inode->i_op == &ubifs_file_inode_operations);
>>  	}
>> 
>> +	err = ubifs_init_acl(dir, inode);
>> +	if (err)
>> +		goto out_inode;
> 
> Why do we need ACLs for O_TMPFILE?

O_TMPFILE files are unnamed... until linkat() is called.
open manpage says that when linkat() is called to a O_TMPFILE file:
"In this case, the open() mode argument determines the file permission 
mode, as with O_CREAT."
And mode with O_CREAT is applied with respect of the default ACLs:
"in the absence of a default ACL, the mode of the created file is (mode 
& ~umask)"

So my interpretation is that default ACLs shall be propagated to 
O_TMPFILE at creation.

O_TMPFILE implementation for btrfs and xfs apply default ACLs as well.

[...]

>> @@ -985,6 +1001,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;
> 
> Do we really need ACLs for symlinks too? I thought permissions for
> symlinks are irrelevant.

Indeed, they are irrelevant. It will be removed.

>> diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
>> index 6c2f4d4..0a596a1 100644
>> --- a/fs/ubifs/xattr.c
>> +++ b/fs/ubifs/xattr.c
>> @@ -52,13 +52,14 @@
>>   * in the VFS inode cache. The xentries are cached in the LNC cache 
>> (see
>>   * tnc.c).
>>   *
>> - * ACL support is not implemented.
>> + * ACL support is now implemented.
> 
> I think we can just drop that line now. :-)

:-)

> 
>>   */
>> 
>>  #include "ubifs.h"
>>  #include <linux/fs.h>
>>  #include <linux/slab.h>
>>  #include <linux/xattr.h>
>> +#include <linux/posix_acl_xattr.h>
> 
> While we are here, these includes should also be cleaned up.
> 

Ok, and I'll remove the now obsolete enum just below too then:

/*
  * Extended attribute type constants.
  *
  * USER_XATTR: user extended attribute ("user.*")
  * TRUSTED_XATTR: trusted extended attribute ("trusted.*)
  * SECURITY_XATTR: security extended attribute ("security.*")
  */

enum {
         USER_XATTR,
         TRUSTED_XATTR,
         SECURITY_XATTR,
};


> 
> Overall the patch does not look bad and I'm sure we can add it for 
> v4.10 soon.

ok, great!

> I'm a bit confused since most stuff is not really UBIFS specific.
> Andreas, is just "normal" for ACL support? I thought most code is
> generic and already in VFS.
> 
> Thanks,
> //richard
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Andreas Gruenbacher Oct. 19, 2016, 2:34 p.m. UTC | #4

Pascal,

On Tue, Oct 18, 2016 at 11:32 PM, Richard Weinberger <richard@nod.at> wrote:
> Pascal,
>
> On 09.10.2016 22:53, Pascal Eberhard wrote:
>> This patch implements POSIX Access Control Lists (ACLs) in UBIFS.
>> It uses posix_acl_(access|default)_xattr_handler ACL generic handlers
>> as xattr handlers are now used in UBIFS.
>
> Cool! :)
> CC'ing Andreas, maybe he also has comments.

please run this through xfstests as well. Note the following fixes /
improvements I've made in the course of reviewing this; they've
obviously not been merged, yet:

  https://github.com/andreas-gruenbacher/xfsprogs-dev/commits/io
  https://github.com/andreas-gruenbacher/xfstests/commits/tmpfile

>> Signed-off-by: Pascal Eberhard <pascal.eberhard@gmail.com>
>> ---
>>  fs/ubifs/Kconfig  |  13 +++++
>>  fs/ubifs/Makefile |   1 +
>>  fs/ubifs/acl.c    | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  fs/ubifs/dir.c    |  22 +++++++++
>>  fs/ubifs/file.c   |   9 ++++
>>  fs/ubifs/super.c  |  15 ++++++
>>  fs/ubifs/ubifs.h  |  18 +++++++
>>  fs/ubifs/xattr.c  |  11 +++--
>>  8 files changed, 229 insertions(+), 3 deletions(-)
>>  create mode 100644 fs/ubifs/acl.c
>>
>> diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig
>> index 7ff7712..39119e5 100644
>> --- a/fs/ubifs/Kconfig
>> +++ b/fs/ubifs/Kconfig
>> @@ -50,3 +50,16 @@ config UBIFS_ATIME_SUPPORT
>>         strictatime is the "heavy", relatime is "lighter", etc.
>>
>>         If unsure, say 'N'
>> +
>> +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/>.
>> +
>> +       If you don't know what Access Control Lists are, say N
>> diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile
>> index c54a243..af7a83f 100644
>> --- a/fs/ubifs/Makefile
>> +++ b/fs/ubifs/Makefile
>> @@ -5,3 +5,4 @@ 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-y += misc.o
>> +ubifs-$(CONFIG_UBIFS_FS_POSIX_ACL) += acl.o
>> \ No newline at end of file
>> diff --git a/fs/ubifs/acl.c b/fs/ubifs/acl.c
>> new file mode 100644
>> index 0000000..a4d4734
>> --- /dev/null
>> +++ b/fs/ubifs/acl.c
>> @@ -0,0 +1,143 @@
>> +/*
>> + * 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>
>
> ubifs.h includes this already.
>
>> +#include <linux/xattr.h>
>
> Ditto.
>
>> +#include <linux/posix_acl_xattr.h>
>> +
>> +#include "ubifs.h"
>> +
>> +struct posix_acl *ubifs_get_acl(struct inode *inode, int type)
>> +{
>> +     int size;
>> +     const char *name;
>> +     char *value = NULL;
>> +     struct posix_acl *acl;
>> +
>> +     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:
>> +             return ERR_PTR(-EINVAL);
>> +     }
>> +
>> +     size = __ubifs_getxattr(inode, name, NULL, 0);
>> +     if (size > 0) {
>> +             value = kzalloc(size, GFP_KERNEL);
>> +             if (!value)
>> +                     return ERR_PTR(-ENOMEM);
>> +             size = __ubifs_getxattr(inode, name, value, size);
>> +     }
>> +     if (size > 0)
>> +             acl = posix_acl_from_xattr(&init_user_ns, value, size);
>> +     else if (size == -ERANGE || size == -ENODATA || size == 0)
>> +             acl = NULL;
>> +     else
>> +             acl = ERR_PTR(size);
>> +
>> +     kfree(value);
>> +     if (!IS_ERR(acl))
>> +             set_cached_acl(inode, type, acl);

The get_acl inode operation should no longer call set_cached_acl; the
cached ACL is updated automatically. (It's correct to use
set_cached_acl in the set_acl inode operation though.)

>> +
>> +     return acl;
>> +}
>> +
>> +int ubifs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>> +{
>> +     int ret;
>> +     int size = 0;
>> +     const char *name;
>> +     char *value = NULL;
>> +     int flags = 0;
>> +
>> +     switch (type) {
>> +     case ACL_TYPE_ACCESS:
>> +             name = XATTR_NAME_POSIX_ACL_ACCESS;
>> +             if (acl) {
>> +                     ret = posix_acl_equiv_mode(acl, &inode->i_mode);
>> +                     if (ret < 0)
>> +                             return ret;
>> +                     if (ret == 0)
>> +                             acl = NULL;
>> +             }
>> +             ret = 0;
>> +             break;
>> +     case ACL_TYPE_DEFAULT:
>> +             if (!S_ISDIR(inode->i_mode))
>> +                     return acl ? -EINVAL : 0;
>> +             name = XATTR_NAME_POSIX_ACL_DEFAULT;
>> +             break;
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +
>> +     if (acl) {
>> +             size = posix_acl_xattr_size(acl->a_count);
>> +             value = kmalloc(size, GFP_KERNEL);
>> +             if (!value)
>> +                     return -ENOMEM;
>> +
>> +             ret = posix_acl_to_xattr(&init_user_ns, acl, value, size);
>> +             if (ret < 0)
>> +                     goto out;
>> +     }
>> +
>> +     if (size == 0)
>> +             flags = XATTR_REPLACE;
>> +     ret = __ubifs_setxattr(inode, name, value, size, flags);
>> +
>> +out:
>> +     kfree(value);
>> +     if (!ret)
>> +             set_cached_acl(inode, type, acl);
>> +
>> +     return ret;
>> +}
>> +
>> +/*
>> + * Initialize the ACLs of a new inode.
>
> Please use the Javadoc style we use for all UBIFS functions.
> (applies to other functions too in this patch)-
>
>> + */
>> +int ubifs_init_acl(struct inode *dir, struct inode *inode)
>> +{
>> +     struct posix_acl *default_acl, *acl;
>> +     int ret = 0;
>> +
>> +     ret = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
>> +     if (ret)
>> +             return ret;
>> +
>> +     if (default_acl) {
>> +             inode_lock(inode);

Why the inode locking here?

>> +             ret = ubifs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
>> +             inode_unlock(inode);
>> +             posix_acl_release(default_acl);
>> +     }
>> +
>> +     if (acl) {
>> +             if (!ret) {
>> +                     inode_lock(inode);

Same here.

>> +                     ret = ubifs_set_acl(inode, acl, ACL_TYPE_ACCESS);
>> +                     inode_unlock(inode);
>> +             }
>> +             posix_acl_release(acl);
>> +     }
>> +
>> +     if (!default_acl && !acl)
>> +             cache_no_acl(inode);

This call to cache_no_acl doesn't make sense; please check. It may be
wrong in btrfs as well; copying Josef for that.

>> +     return ret;
>> +}
>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>> index ccd9128..a2e5609 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;
>> +

The pattern ubifs_budget_space / ubifs_new_inode / ubifs_init_acl /
ubifs_init_security repeats a number of times in the code, so it
should probably be put in a function.

>>       err = ubifs_init_security(dir, inode, &dentry->d_name);
>>       if (err)
>>               goto out_inode;
>> @@ -341,6 +345,10 @@ static int do_tmpfile(struct inode *dir, struct dentry *dentry,
>>               ubifs_assert(inode->i_op == &ubifs_file_inode_operations);
>>       }
>>
>> +     err = ubifs_init_acl(dir, inode);
>> +     if (err)
>> +             goto out_inode;
>
> Why do we need ACLs for O_TMPFILE?
>
>> +
>>       err = ubifs_init_security(dir, inode, &dentry->d_name);
>>       if (err)
>>               goto out_inode;
>> @@ -821,6 +829,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;
>> @@ -903,6 +915,10 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry,
>>       ui->data = dev;
>>       ui->data_len = devlen;
>>
>> +     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;
>> @@ -985,6 +1001,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;
>
> Do we really need ACLs for symlinks too? I thought permissions for symlinks are irrelevant.
>
>>       err = ubifs_init_security(dir, inode, &dentry->d_name);
>>       if (err)
>>               goto out_inode;
>> @@ -1395,6 +1415,8 @@ const struct inode_operations ubifs_dir_inode_operations = {
>>       .update_time = ubifs_update_time,
>>  #endif
>>       .tmpfile     = ubifs_tmpfile,
>> +     .get_acl     = ubifs_get_acl,
>> +     .set_acl     = ubifs_set_acl,
>>  };
>>
>>  const struct file_operations ubifs_dir_operations = {
>> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
>> index 4ed270b..17f6098 100644
>> --- a/fs/ubifs/file.c
>> +++ b/fs/ubifs/file.c
>> @@ -53,6 +53,7 @@
>>  #include <linux/mount.h>
>>  #include <linux/slab.h>
>>  #include <linux/migrate.h>
>> +#include <linux/posix_acl.h>
>>
>>  static int read_block(struct inode *inode, void *addr, unsigned int block,
>>                     struct ubifs_data_node *dn)
>> @@ -1251,6 +1252,10 @@ static int do_setattr(struct ubifs_info *c, struct inode *inode,
>>               ubifs_release_budget(c, &req);
>>       if (IS_SYNC(inode))
>>               err = inode->i_sb->s_op->write_inode(inode, NULL);
>> +
>> +     if (!err && (attr->ia_valid & ATTR_MODE))
>> +             err = posix_acl_chmod(inode, inode->i_mode);
>> +
>>       return err;
>>  }
>>
>> @@ -1628,6 +1633,8 @@ const struct inode_operations ubifs_file_inode_operations = {
>>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>>       .update_time = ubifs_update_time,
>>  #endif
>> +     .get_acl     = ubifs_get_acl,
>> +     .set_acl     = ubifs_set_acl,
>>  };
>>
>>  const struct inode_operations ubifs_symlink_inode_operations = {
>> @@ -1642,6 +1649,8 @@ const struct inode_operations ubifs_symlink_inode_operations = {
>>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>>       .update_time = ubifs_update_time,
>>  #endif
>> +     .get_acl     = ubifs_get_acl,
>> +     .set_acl     = ubifs_set_acl,
>>  };
>>
>>  const struct file_operations ubifs_file_operations = {
>> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
>> index 4ec0510..8239dfd 100644
>> --- a/fs/ubifs/super.c
>> +++ b/fs/ubifs/super.c
>> @@ -444,6 +444,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_puts(s, ",acl");
>> +
>>       return 0;
>>  }
>>
>> @@ -929,6 +932,8 @@ enum {
>>       Opt_chk_data_crc,
>>       Opt_no_chk_data_crc,
>>       Opt_override_compr,
>> +     Opt_acl,
>> +     Opt_noacl,
>>       Opt_err,
>>  };
>>
>> @@ -940,6 +945,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},
>>  };
>>
>> @@ -1040,6 +1047,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;
>> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
>> index 096035e..7935449 100644
>> --- a/fs/ubifs/ubifs.h
>> +++ b/fs/ubifs/ubifs.h
>> @@ -1743,6 +1743,24 @@ extern const struct xattr_handler *ubifs_xattr_handlers[];
>>  ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size);
>>  int ubifs_init_security(struct inode *dentry, struct inode *inode,
>>                       const struct qstr *qstr);
>> +ssize_t __ubifs_getxattr(struct inode *host, const char *name,
>> +                             void *buf, size_t size);
>> +int __ubifs_setxattr(struct inode *host, const char *name,
>> +                         const void *value, size_t size, int flags);
>> +
>> +/* 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);
>> diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
>> index 6c2f4d4..0a596a1 100644
>> --- a/fs/ubifs/xattr.c
>> +++ b/fs/ubifs/xattr.c
>> @@ -52,13 +52,14 @@
>>   * in the VFS inode cache. The xentries are cached in the LNC cache (see
>>   * tnc.c).
>>   *
>> - * ACL support is not implemented.
>> + * ACL support is now implemented.
>
> I think we can just drop that line now. :-)
>
>>   */
>>
>>  #include "ubifs.h"
>>  #include <linux/fs.h>
>>  #include <linux/slab.h>
>>  #include <linux/xattr.h>
>> +#include <linux/posix_acl_xattr.h>
>
> While we are here, these includes should also be cleaned up.
>
>>  /*
>>   * Limit the number of extended attributes per inode so that the total size
>> @@ -268,7 +269,7 @@ static struct inode *iget_xattr(struct ubifs_info *c, ino_t inum)
>>       return ERR_PTR(-EINVAL);
>>  }
>>
>> -static int __ubifs_setxattr(struct inode *host, const char *name,
>> +int __ubifs_setxattr(struct inode *host, const char *name,
>>                           const void *value, size_t size, int flags)
>>  {
>>       struct inode *inode;
>> @@ -328,7 +329,7 @@ out_free:
>>       return err;
>>  }
>>
>> -static ssize_t __ubifs_getxattr(struct inode *host, const char *name,
>> +ssize_t __ubifs_getxattr(struct inode *host, const char *name,
>>                               void *buf, size_t size)
>>  {
>>       struct inode *inode;
>> @@ -619,5 +620,9 @@ const struct xattr_handler *ubifs_xattr_handlers[] = {
>>       &ubifs_user_xattr_handler,
>>       &ubifs_trusted_xattr_handler,
>>       &ubifs_security_xattr_handler,
>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>> +     &posix_acl_access_xattr_handler,
>> +     &posix_acl_default_xattr_handler,
>> +#endif
>>       NULL
>>  };
>>
>
> Overall the patch does not look bad and I'm sure we can add it for v4.10 soon.
> I'm a bit confused since most stuff is not really UBIFS specific.
> Andreas, is just "normal" for ACL support? I thought most code is generic and already in VFS.

There are several filesystems like ubifs that use the xattr
representation as the on-disk format, so ubifs_get_acl and
ubifs_set_acl could be abstracted and shared across those filesystems.
The rest looks good.

Thanks,
Andreas

Andreas Gruenbacher Oct. 19, 2016, 2:37 p.m. UTC | #5

On Wed, Oct 19, 2016 at 4:34 PM, Andreas Gruenbacher
<agruenba@redhat.com> wrote:
> Pascal,
>
> On Tue, Oct 18, 2016 at 11:32 PM, Richard Weinberger <richard@nod.at> wrote:
>> Pascal,
>>
>> On 09.10.2016 22:53, Pascal Eberhard wrote:
>>> This patch implements POSIX Access Control Lists (ACLs) in UBIFS.
>>> It uses posix_acl_(access|default)_xattr_handler ACL generic handlers
>>> as xattr handlers are now used in UBIFS.
>>
>> Cool! :)
>> CC'ing Andreas, maybe he also has comments.
>
> please run this through xfstests as well. Note the following fixes /
> improvements I've made in the course of reviewing this; they've
> obviously not been merged, yet:
>
>   https://github.com/andreas-gruenbacher/xfsprogs-dev/commits/io
>   https://github.com/andreas-gruenbacher/xfstests/commits/tmpfile
>
>>> Signed-off-by: Pascal Eberhard <pascal.eberhard@gmail.com>
>>> ---
>>>  fs/ubifs/Kconfig  |  13 +++++
>>>  fs/ubifs/Makefile |   1 +
>>>  fs/ubifs/acl.c    | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  fs/ubifs/dir.c    |  22 +++++++++
>>>  fs/ubifs/file.c   |   9 ++++
>>>  fs/ubifs/super.c  |  15 ++++++
>>>  fs/ubifs/ubifs.h  |  18 +++++++
>>>  fs/ubifs/xattr.c  |  11 +++--
>>>  8 files changed, 229 insertions(+), 3 deletions(-)
>>>  create mode 100644 fs/ubifs/acl.c
>>>
>>> diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig
>>> index 7ff7712..39119e5 100644
>>> --- a/fs/ubifs/Kconfig
>>> +++ b/fs/ubifs/Kconfig
>>> @@ -50,3 +50,16 @@ config UBIFS_ATIME_SUPPORT
>>>         strictatime is the "heavy", relatime is "lighter", etc.
>>>
>>>         If unsure, say 'N'
>>> +
>>> +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/>.
>>> +
>>> +       If you don't know what Access Control Lists are, say N
>>> diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile
>>> index c54a243..af7a83f 100644
>>> --- a/fs/ubifs/Makefile
>>> +++ b/fs/ubifs/Makefile
>>> @@ -5,3 +5,4 @@ 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-y += misc.o
>>> +ubifs-$(CONFIG_UBIFS_FS_POSIX_ACL) += acl.o
>>> \ No newline at end of file
>>> diff --git a/fs/ubifs/acl.c b/fs/ubifs/acl.c
>>> new file mode 100644
>>> index 0000000..a4d4734
>>> --- /dev/null
>>> +++ b/fs/ubifs/acl.c
>>> @@ -0,0 +1,143 @@
>>> +/*
>>> + * 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>
>>
>> ubifs.h includes this already.
>>
>>> +#include <linux/xattr.h>
>>
>> Ditto.
>>
>>> +#include <linux/posix_acl_xattr.h>
>>> +
>>> +#include "ubifs.h"
>>> +
>>> +struct posix_acl *ubifs_get_acl(struct inode *inode, int type)
>>> +{
>>> +     int size;
>>> +     const char *name;
>>> +     char *value = NULL;
>>> +     struct posix_acl *acl;
>>> +
>>> +     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:
>>> +             return ERR_PTR(-EINVAL);
>>> +     }
>>> +
>>> +     size = __ubifs_getxattr(inode, name, NULL, 0);
>>> +     if (size > 0) {
>>> +             value = kzalloc(size, GFP_KERNEL);
>>> +             if (!value)
>>> +                     return ERR_PTR(-ENOMEM);
>>> +             size = __ubifs_getxattr(inode, name, value, size);
>>> +     }
>>> +     if (size > 0)
>>> +             acl = posix_acl_from_xattr(&init_user_ns, value, size);
>>> +     else if (size == -ERANGE || size == -ENODATA || size == 0)
>>> +             acl = NULL;
>>> +     else
>>> +             acl = ERR_PTR(size);
>>> +
>>> +     kfree(value);
>>> +     if (!IS_ERR(acl))
>>> +             set_cached_acl(inode, type, acl);
>
> The get_acl inode operation should no longer call set_cached_acl; the
> cached ACL is updated automatically. (It's correct to use
> set_cached_acl in the set_acl inode operation though.)
>
>>> +
>>> +     return acl;
>>> +}
>>> +
>>> +int ubifs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>>> +{
>>> +     int ret;
>>> +     int size = 0;
>>> +     const char *name;
>>> +     char *value = NULL;
>>> +     int flags = 0;
>>> +
>>> +     switch (type) {
>>> +     case ACL_TYPE_ACCESS:
>>> +             name = XATTR_NAME_POSIX_ACL_ACCESS;
>>> +             if (acl) {
>>> +                     ret = posix_acl_equiv_mode(acl, &inode->i_mode);
>>> +                     if (ret < 0)
>>> +                             return ret;
>>> +                     if (ret == 0)
>>> +                             acl = NULL;
>>> +             }
>>> +             ret = 0;
>>> +             break;
>>> +     case ACL_TYPE_DEFAULT:
>>> +             if (!S_ISDIR(inode->i_mode))
>>> +                     return acl ? -EINVAL : 0;
>>> +             name = XATTR_NAME_POSIX_ACL_DEFAULT;
>>> +             break;
>>> +     default:
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     if (acl) {
>>> +             size = posix_acl_xattr_size(acl->a_count);
>>> +             value = kmalloc(size, GFP_KERNEL);
>>> +             if (!value)
>>> +                     return -ENOMEM;
>>> +
>>> +             ret = posix_acl_to_xattr(&init_user_ns, acl, value, size);
>>> +             if (ret < 0)
>>> +                     goto out;
>>> +     }
>>> +
>>> +     if (size == 0)
>>> +             flags = XATTR_REPLACE;
>>> +     ret = __ubifs_setxattr(inode, name, value, size, flags);
>>> +
>>> +out:
>>> +     kfree(value);
>>> +     if (!ret)
>>> +             set_cached_acl(inode, type, acl);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +/*
>>> + * Initialize the ACLs of a new inode.
>>
>> Please use the Javadoc style we use for all UBIFS functions.
>> (applies to other functions too in this patch)-
>>
>>> + */
>>> +int ubifs_init_acl(struct inode *dir, struct inode *inode)
>>> +{
>>> +     struct posix_acl *default_acl, *acl;
>>> +     int ret = 0;
>>> +
>>> +     ret = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     if (default_acl) {
>>> +             inode_lock(inode);
>
> Why the inode locking here?
>
>>> +             ret = ubifs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
>>> +             inode_unlock(inode);
>>> +             posix_acl_release(default_acl);
>>> +     }
>>> +
>>> +     if (acl) {
>>> +             if (!ret) {
>>> +                     inode_lock(inode);
>
> Same here.
>
>>> +                     ret = ubifs_set_acl(inode, acl, ACL_TYPE_ACCESS);
>>> +                     inode_unlock(inode);
>>> +             }
>>> +             posix_acl_release(acl);
>>> +     }
>>> +
>>> +     if (!default_acl && !acl)
>>> +             cache_no_acl(inode);
>
> This call to cache_no_acl doesn't make sense; please check. It may be
> wrong in btrfs as well; copying Josef for that.

Now with Josef's current email address, hopefully.

>>> +     return ret;
>>> +}
>>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>>> index ccd9128..a2e5609 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;
>>> +
>
> The pattern ubifs_budget_space / ubifs_new_inode / ubifs_init_acl /
> ubifs_init_security repeats a number of times in the code, so it
> should probably be put in a function.
>
>>>       err = ubifs_init_security(dir, inode, &dentry->d_name);
>>>       if (err)
>>>               goto out_inode;
>>> @@ -341,6 +345,10 @@ static int do_tmpfile(struct inode *dir, struct dentry *dentry,
>>>               ubifs_assert(inode->i_op == &ubifs_file_inode_operations);
>>>       }
>>>
>>> +     err = ubifs_init_acl(dir, inode);
>>> +     if (err)
>>> +             goto out_inode;
>>
>> Why do we need ACLs for O_TMPFILE?
>>
>>> +
>>>       err = ubifs_init_security(dir, inode, &dentry->d_name);
>>>       if (err)
>>>               goto out_inode;
>>> @@ -821,6 +829,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;
>>> @@ -903,6 +915,10 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry,
>>>       ui->data = dev;
>>>       ui->data_len = devlen;
>>>
>>> +     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;
>>> @@ -985,6 +1001,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;
>>
>> Do we really need ACLs for symlinks too? I thought permissions for symlinks are irrelevant.
>>
>>>       err = ubifs_init_security(dir, inode, &dentry->d_name);
>>>       if (err)
>>>               goto out_inode;
>>> @@ -1395,6 +1415,8 @@ const struct inode_operations ubifs_dir_inode_operations = {
>>>       .update_time = ubifs_update_time,
>>>  #endif
>>>       .tmpfile     = ubifs_tmpfile,
>>> +     .get_acl     = ubifs_get_acl,
>>> +     .set_acl     = ubifs_set_acl,
>>>  };
>>>
>>>  const struct file_operations ubifs_dir_operations = {
>>> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
>>> index 4ed270b..17f6098 100644
>>> --- a/fs/ubifs/file.c
>>> +++ b/fs/ubifs/file.c
>>> @@ -53,6 +53,7 @@
>>>  #include <linux/mount.h>
>>>  #include <linux/slab.h>
>>>  #include <linux/migrate.h>
>>> +#include <linux/posix_acl.h>
>>>
>>>  static int read_block(struct inode *inode, void *addr, unsigned int block,
>>>                     struct ubifs_data_node *dn)
>>> @@ -1251,6 +1252,10 @@ static int do_setattr(struct ubifs_info *c, struct inode *inode,
>>>               ubifs_release_budget(c, &req);
>>>       if (IS_SYNC(inode))
>>>               err = inode->i_sb->s_op->write_inode(inode, NULL);
>>> +
>>> +     if (!err && (attr->ia_valid & ATTR_MODE))
>>> +             err = posix_acl_chmod(inode, inode->i_mode);
>>> +
>>>       return err;
>>>  }
>>>
>>> @@ -1628,6 +1633,8 @@ const struct inode_operations ubifs_file_inode_operations = {
>>>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>>>       .update_time = ubifs_update_time,
>>>  #endif
>>> +     .get_acl     = ubifs_get_acl,
>>> +     .set_acl     = ubifs_set_acl,
>>>  };
>>>
>>>  const struct inode_operations ubifs_symlink_inode_operations = {
>>> @@ -1642,6 +1649,8 @@ const struct inode_operations ubifs_symlink_inode_operations = {
>>>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>>>       .update_time = ubifs_update_time,
>>>  #endif
>>> +     .get_acl     = ubifs_get_acl,
>>> +     .set_acl     = ubifs_set_acl,
>>>  };
>>>
>>>  const struct file_operations ubifs_file_operations = {
>>> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
>>> index 4ec0510..8239dfd 100644
>>> --- a/fs/ubifs/super.c
>>> +++ b/fs/ubifs/super.c
>>> @@ -444,6 +444,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_puts(s, ",acl");
>>> +
>>>       return 0;
>>>  }
>>>
>>> @@ -929,6 +932,8 @@ enum {
>>>       Opt_chk_data_crc,
>>>       Opt_no_chk_data_crc,
>>>       Opt_override_compr,
>>> +     Opt_acl,
>>> +     Opt_noacl,
>>>       Opt_err,
>>>  };
>>>
>>> @@ -940,6 +945,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},
>>>  };
>>>
>>> @@ -1040,6 +1047,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;
>>> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
>>> index 096035e..7935449 100644
>>> --- a/fs/ubifs/ubifs.h
>>> +++ b/fs/ubifs/ubifs.h
>>> @@ -1743,6 +1743,24 @@ extern const struct xattr_handler *ubifs_xattr_handlers[];
>>>  ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size);
>>>  int ubifs_init_security(struct inode *dentry, struct inode *inode,
>>>                       const struct qstr *qstr);
>>> +ssize_t __ubifs_getxattr(struct inode *host, const char *name,
>>> +                             void *buf, size_t size);
>>> +int __ubifs_setxattr(struct inode *host, const char *name,
>>> +                         const void *value, size_t size, int flags);
>>> +
>>> +/* 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);
>>> diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
>>> index 6c2f4d4..0a596a1 100644
>>> --- a/fs/ubifs/xattr.c
>>> +++ b/fs/ubifs/xattr.c
>>> @@ -52,13 +52,14 @@
>>>   * in the VFS inode cache. The xentries are cached in the LNC cache (see
>>>   * tnc.c).
>>>   *
>>> - * ACL support is not implemented.
>>> + * ACL support is now implemented.
>>
>> I think we can just drop that line now. :-)
>>
>>>   */
>>>
>>>  #include "ubifs.h"
>>>  #include <linux/fs.h>
>>>  #include <linux/slab.h>
>>>  #include <linux/xattr.h>
>>> +#include <linux/posix_acl_xattr.h>
>>
>> While we are here, these includes should also be cleaned up.
>>
>>>  /*
>>>   * Limit the number of extended attributes per inode so that the total size
>>> @@ -268,7 +269,7 @@ static struct inode *iget_xattr(struct ubifs_info *c, ino_t inum)
>>>       return ERR_PTR(-EINVAL);
>>>  }
>>>
>>> -static int __ubifs_setxattr(struct inode *host, const char *name,
>>> +int __ubifs_setxattr(struct inode *host, const char *name,
>>>                           const void *value, size_t size, int flags)
>>>  {
>>>       struct inode *inode;
>>> @@ -328,7 +329,7 @@ out_free:
>>>       return err;
>>>  }
>>>
>>> -static ssize_t __ubifs_getxattr(struct inode *host, const char *name,
>>> +ssize_t __ubifs_getxattr(struct inode *host, const char *name,
>>>                               void *buf, size_t size)
>>>  {
>>>       struct inode *inode;
>>> @@ -619,5 +620,9 @@ const struct xattr_handler *ubifs_xattr_handlers[] = {
>>>       &ubifs_user_xattr_handler,
>>>       &ubifs_trusted_xattr_handler,
>>>       &ubifs_security_xattr_handler,
>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>>> +     &posix_acl_access_xattr_handler,
>>> +     &posix_acl_default_xattr_handler,
>>> +#endif
>>>       NULL
>>>  };
>>>
>>
>> Overall the patch does not look bad and I'm sure we can add it for v4.10 soon.
>> I'm a bit confused since most stuff is not really UBIFS specific.
>> Andreas, is just "normal" for ACL support? I thought most code is generic and already in VFS.
>
> There are several filesystems like ubifs that use the xattr
> representation as the on-disk format, so ubifs_get_acl and
> ubifs_set_acl could be abstracted and shared across those filesystems.
> The rest looks good.
>
> Thanks,
> Andreas

Josef Bacik Oct. 19, 2016, 2:51 p.m. UTC | #6

On 10/19/2016 10:37 AM, Andreas Gruenbacher wrote:
> On Wed, Oct 19, 2016 at 4:34 PM, Andreas Gruenbacher
> <agruenba@redhat.com> wrote:
>> Pascal,
>>
>> On Tue, Oct 18, 2016 at 11:32 PM, Richard Weinberger <richard@nod.at> wrote:
>>> Pascal,
>>>
>>> On 09.10.2016 22:53, Pascal Eberhard wrote:
>>>> This patch implements POSIX Access Control Lists (ACLs) in UBIFS.
>>>> It uses posix_acl_(access|default)_xattr_handler ACL generic handlers
>>>> as xattr handlers are now used in UBIFS.
>>>
>>> Cool! :)
>>> CC'ing Andreas, maybe he also has comments.
>>
>> please run this through xfstests as well. Note the following fixes /
>> improvements I've made in the course of reviewing this; they've
>> obviously not been merged, yet:
>>
>>   https://github.com/andreas-gruenbacher/xfsprogs-dev/commits/io
>>   https://github.com/andreas-gruenbacher/xfstests/commits/tmpfile
>>
>>>> Signed-off-by: Pascal Eberhard <pascal.eberhard@gmail.com>
>>>> ---
>>>>  fs/ubifs/Kconfig  |  13 +++++
>>>>  fs/ubifs/Makefile |   1 +
>>>>  fs/ubifs/acl.c    | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  fs/ubifs/dir.c    |  22 +++++++++
>>>>  fs/ubifs/file.c   |   9 ++++
>>>>  fs/ubifs/super.c  |  15 ++++++
>>>>  fs/ubifs/ubifs.h  |  18 +++++++
>>>>  fs/ubifs/xattr.c  |  11 +++--
>>>>  8 files changed, 229 insertions(+), 3 deletions(-)
>>>>  create mode 100644 fs/ubifs/acl.c
>>>>
>>>> diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig
>>>> index 7ff7712..39119e5 100644
>>>> --- a/fs/ubifs/Kconfig
>>>> +++ b/fs/ubifs/Kconfig
>>>> @@ -50,3 +50,16 @@ config UBIFS_ATIME_SUPPORT
>>>>         strictatime is the "heavy", relatime is "lighter", etc.
>>>>
>>>>         If unsure, say 'N'
>>>> +
>>>> +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 <https://urldefense.proofpoint.com/v2/url?u=http-3A__acl.bestbits.at_&d=DQIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=sDzg6MvHymKOUgI8SFIm4Q&m=kMcqxODxmAbECYcLW6bRou3Q-jJYfAL4HPw5ErTumUU&s=De0Qb4gRiGITbgivfS5NKW92fL7-HKCGNvCnAMU6CwM&e= >.
>>>> +
>>>> +       If you don't know what Access Control Lists are, say N
>>>> diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile
>>>> index c54a243..af7a83f 100644
>>>> --- a/fs/ubifs/Makefile
>>>> +++ b/fs/ubifs/Makefile
>>>> @@ -5,3 +5,4 @@ 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-y += misc.o
>>>> +ubifs-$(CONFIG_UBIFS_FS_POSIX_ACL) += acl.o
>>>> \ No newline at end of file
>>>> diff --git a/fs/ubifs/acl.c b/fs/ubifs/acl.c
>>>> new file mode 100644
>>>> index 0000000..a4d4734
>>>> --- /dev/null
>>>> +++ b/fs/ubifs/acl.c
>>>> @@ -0,0 +1,143 @@
>>>> +/*
>>>> + * 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>
>>>
>>> ubifs.h includes this already.
>>>
>>>> +#include <linux/xattr.h>
>>>
>>> Ditto.
>>>
>>>> +#include <linux/posix_acl_xattr.h>
>>>> +
>>>> +#include "ubifs.h"
>>>> +
>>>> +struct posix_acl *ubifs_get_acl(struct inode *inode, int type)
>>>> +{
>>>> +     int size;
>>>> +     const char *name;
>>>> +     char *value = NULL;
>>>> +     struct posix_acl *acl;
>>>> +
>>>> +     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:
>>>> +             return ERR_PTR(-EINVAL);
>>>> +     }
>>>> +
>>>> +     size = __ubifs_getxattr(inode, name, NULL, 0);
>>>> +     if (size > 0) {
>>>> +             value = kzalloc(size, GFP_KERNEL);
>>>> +             if (!value)
>>>> +                     return ERR_PTR(-ENOMEM);
>>>> +             size = __ubifs_getxattr(inode, name, value, size);
>>>> +     }
>>>> +     if (size > 0)
>>>> +             acl = posix_acl_from_xattr(&init_user_ns, value, size);
>>>> +     else if (size == -ERANGE || size == -ENODATA || size == 0)
>>>> +             acl = NULL;
>>>> +     else
>>>> +             acl = ERR_PTR(size);
>>>> +
>>>> +     kfree(value);
>>>> +     if (!IS_ERR(acl))
>>>> +             set_cached_acl(inode, type, acl);
>>
>> The get_acl inode operation should no longer call set_cached_acl; the
>> cached ACL is updated automatically. (It's correct to use
>> set_cached_acl in the set_acl inode operation though.)
>>
>>>> +
>>>> +     return acl;
>>>> +}
>>>> +
>>>> +int ubifs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>>>> +{
>>>> +     int ret;
>>>> +     int size = 0;
>>>> +     const char *name;
>>>> +     char *value = NULL;
>>>> +     int flags = 0;
>>>> +
>>>> +     switch (type) {
>>>> +     case ACL_TYPE_ACCESS:
>>>> +             name = XATTR_NAME_POSIX_ACL_ACCESS;
>>>> +             if (acl) {
>>>> +                     ret = posix_acl_equiv_mode(acl, &inode->i_mode);
>>>> +                     if (ret < 0)
>>>> +                             return ret;
>>>> +                     if (ret == 0)
>>>> +                             acl = NULL;
>>>> +             }
>>>> +             ret = 0;
>>>> +             break;
>>>> +     case ACL_TYPE_DEFAULT:
>>>> +             if (!S_ISDIR(inode->i_mode))
>>>> +                     return acl ? -EINVAL : 0;
>>>> +             name = XATTR_NAME_POSIX_ACL_DEFAULT;
>>>> +             break;
>>>> +     default:
>>>> +             return -EINVAL;
>>>> +     }
>>>> +
>>>> +     if (acl) {
>>>> +             size = posix_acl_xattr_size(acl->a_count);
>>>> +             value = kmalloc(size, GFP_KERNEL);
>>>> +             if (!value)
>>>> +                     return -ENOMEM;
>>>> +
>>>> +             ret = posix_acl_to_xattr(&init_user_ns, acl, value, size);
>>>> +             if (ret < 0)
>>>> +                     goto out;
>>>> +     }
>>>> +
>>>> +     if (size == 0)
>>>> +             flags = XATTR_REPLACE;
>>>> +     ret = __ubifs_setxattr(inode, name, value, size, flags);
>>>> +
>>>> +out:
>>>> +     kfree(value);
>>>> +     if (!ret)
>>>> +             set_cached_acl(inode, type, acl);
>>>> +
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Initialize the ACLs of a new inode.
>>>
>>> Please use the Javadoc style we use for all UBIFS functions.
>>> (applies to other functions too in this patch)-
>>>
>>>> + */
>>>> +int ubifs_init_acl(struct inode *dir, struct inode *inode)
>>>> +{
>>>> +     struct posix_acl *default_acl, *acl;
>>>> +     int ret = 0;
>>>> +
>>>> +     ret = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>> +     if (default_acl) {
>>>> +             inode_lock(inode);
>>
>> Why the inode locking here?
>>
>>>> +             ret = ubifs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
>>>> +             inode_unlock(inode);
>>>> +             posix_acl_release(default_acl);
>>>> +     }
>>>> +
>>>> +     if (acl) {
>>>> +             if (!ret) {
>>>> +                     inode_lock(inode);
>>
>> Same here.
>>
>>>> +                     ret = ubifs_set_acl(inode, acl, ACL_TYPE_ACCESS);
>>>> +                     inode_unlock(inode);
>>>> +             }
>>>> +             posix_acl_release(acl);
>>>> +     }
>>>> +
>>>> +     if (!default_acl && !acl)
>>>> +             cache_no_acl(inode);
>>
>> This call to cache_no_acl doesn't make sense; please check. It may be
>> wrong in btrfs as well; copying Josef for that.
>
> Now with Josef's current email address, hopefully.
>

Looks right to me, we call this during inode creation, so if there is no ACL to 
set then we cache no acl so we don't waste the lookup later.  Thanks,

Josef

Andreas Gruenbacher Oct. 19, 2016, 3:05 p.m. UTC | #7

On Wed, Oct 19, 2016 at 4:51 PM, Josef Bacik <jbacik@fb.com> wrote:
> Looks right to me, we call this during inode creation, so if there is no ACL
> to set then we cache no acl so we don't waste the lookup later.

This is when we create an new inode, so neither i_acl nor
i_default_acl should be ACL_NOT_CACHED when this function succeeds. So
either we should make sure that i_acl and i_default_acl are already
not set to ACL_NOT_CACHED anymore when entering this function, or
cache_no_acl should be called unconditionally before setting the
actual ACLs.

Thanks,
Andreas

Pascal Eberhard Oct. 20, 2016, 12:04 a.m. UTC | #8

Hi Andreas,

Thanks for your comments.

On 2016-10-19 16:34, Andreas Gruenbacher wrote:
> Pascal,
> 
> On Tue, Oct 18, 2016 at 11:32 PM, Richard Weinberger <richard@nod.at> 
> wrote:
>> Pascal,
>> 
>> On 09.10.2016 22:53, Pascal Eberhard wrote:
>>> This patch implements POSIX Access Control Lists (ACLs) in UBIFS.
>>> It uses posix_acl_(access|default)_xattr_handler ACL generic handlers
>>> as xattr handlers are now used in UBIFS.
>> 
>> Cool! :)
>> CC'ing Andreas, maybe he also has comments.
> 
> please run this through xfstests as well. Note the following fixes /
> improvements I've made in the course of reviewing this; they've
> obviously not been merged, yet:
> 
>   https://github.com/andreas-gruenbacher/xfsprogs-dev/commits/io
>   https://github.com/andreas-gruenbacher/xfstests/commits/tmpfile

Ok, nice :), I will run them.

>>> Signed-off-by: Pascal Eberhard <pascal.eberhard@gmail.com>
>>> ---
>>>  fs/ubifs/Kconfig  |  13 +++++
>>>  fs/ubifs/Makefile |   1 +
>>>  fs/ubifs/acl.c    | 143 
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  fs/ubifs/dir.c    |  22 +++++++++
>>>  fs/ubifs/file.c   |   9 ++++
>>>  fs/ubifs/super.c  |  15 ++++++
>>>  fs/ubifs/ubifs.h  |  18 +++++++
>>>  fs/ubifs/xattr.c  |  11 +++--
>>>  8 files changed, 229 insertions(+), 3 deletions(-)
>>>  create mode 100644 fs/ubifs/acl.c
>>> 
>>> diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig
>>> index 7ff7712..39119e5 100644
>>> --- a/fs/ubifs/Kconfig
>>> +++ b/fs/ubifs/Kconfig
>>> @@ -50,3 +50,16 @@ config UBIFS_ATIME_SUPPORT
>>>         strictatime is the "heavy", relatime is "lighter", etc.
>>> 
>>>         If unsure, say 'N'
>>> +
>>> +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/>.
>>> +
>>> +       If you don't know what Access Control Lists are, say N
>>> diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile
>>> index c54a243..af7a83f 100644
>>> --- a/fs/ubifs/Makefile
>>> +++ b/fs/ubifs/Makefile
>>> @@ -5,3 +5,4 @@ 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-y += misc.o
>>> +ubifs-$(CONFIG_UBIFS_FS_POSIX_ACL) += acl.o
>>> \ No newline at end of file
>>> diff --git a/fs/ubifs/acl.c b/fs/ubifs/acl.c
>>> new file mode 100644
>>> index 0000000..a4d4734
>>> --- /dev/null
>>> +++ b/fs/ubifs/acl.c
>>> @@ -0,0 +1,143 @@
>>> +/*
>>> + * 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>
>> 
>> ubifs.h includes this already.
>> 
>>> +#include <linux/xattr.h>
>> 
>> Ditto.
>> 
>>> +#include <linux/posix_acl_xattr.h>
>>> +
>>> +#include "ubifs.h"
>>> +
>>> +struct posix_acl *ubifs_get_acl(struct inode *inode, int type)
>>> +{
>>> +     int size;
>>> +     const char *name;
>>> +     char *value = NULL;
>>> +     struct posix_acl *acl;
>>> +
>>> +     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:
>>> +             return ERR_PTR(-EINVAL);
>>> +     }
>>> +
>>> +     size = __ubifs_getxattr(inode, name, NULL, 0);
>>> +     if (size > 0) {
>>> +             value = kzalloc(size, GFP_KERNEL);
>>> +             if (!value)
>>> +                     return ERR_PTR(-ENOMEM);
>>> +             size = __ubifs_getxattr(inode, name, value, size);
>>> +     }
>>> +     if (size > 0)
>>> +             acl = posix_acl_from_xattr(&init_user_ns, value, size);
>>> +     else if (size == -ERANGE || size == -ENODATA || size == 0)
>>> +             acl = NULL;
>>> +     else
>>> +             acl = ERR_PTR(size);
>>> +
>>> +     kfree(value);
>>> +     if (!IS_ERR(acl))
>>> +             set_cached_acl(inode, type, acl);
> 
> The get_acl inode operation should no longer call set_cached_acl; the
> cached ACL is updated automatically. (It's correct to use
> set_cached_acl in the set_acl inode operation though.)

ok

>>> +
>>> +     return acl;
>>> +}
>>> +
>>> +int ubifs_set_acl(struct inode *inode, struct posix_acl *acl, int 
>>> type)
>>> +{
>>> +     int ret;
>>> +     int size = 0;
>>> +     const char *name;
>>> +     char *value = NULL;
>>> +     int flags = 0;
>>> +
>>> +     switch (type) {
>>> +     case ACL_TYPE_ACCESS:
>>> +             name = XATTR_NAME_POSIX_ACL_ACCESS;
>>> +             if (acl) {
>>> +                     ret = posix_acl_equiv_mode(acl, 
>>> &inode->i_mode);
>>> +                     if (ret < 0)
>>> +                             return ret;
>>> +                     if (ret == 0)
>>> +                             acl = NULL;
>>> +             }
>>> +             ret = 0;
>>> +             break;
>>> +     case ACL_TYPE_DEFAULT:
>>> +             if (!S_ISDIR(inode->i_mode))
>>> +                     return acl ? -EINVAL : 0;
>>> +             name = XATTR_NAME_POSIX_ACL_DEFAULT;
>>> +             break;
>>> +     default:
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     if (acl) {
>>> +             size = posix_acl_xattr_size(acl->a_count);
>>> +             value = kmalloc(size, GFP_KERNEL);
>>> +             if (!value)
>>> +                     return -ENOMEM;
>>> +
>>> +             ret = posix_acl_to_xattr(&init_user_ns, acl, value, 
>>> size);
>>> +             if (ret < 0)
>>> +                     goto out;
>>> +     }
>>> +
>>> +     if (size == 0)
>>> +             flags = XATTR_REPLACE;
>>> +     ret = __ubifs_setxattr(inode, name, value, size, flags);
>>> +
>>> +out:
>>> +     kfree(value);
>>> +     if (!ret)
>>> +             set_cached_acl(inode, type, acl);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +/*
>>> + * Initialize the ACLs of a new inode.
>> 
>> Please use the Javadoc style we use for all UBIFS functions.
>> (applies to other functions too in this patch)-
>> 
>>> + */
>>> +int ubifs_init_acl(struct inode *dir, struct inode *inode)
>>> +{
>>> +     struct posix_acl *default_acl, *acl;
>>> +     int ret = 0;
>>> +
>>> +     ret = posix_acl_create(dir, &inode->i_mode, &default_acl, 
>>> &acl);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     if (default_acl) {
>>> +             inode_lock(inode);
> 
> Why the inode locking here?

I see no reason why we should keep it but I will double check.

>>> +             ret = ubifs_set_acl(inode, default_acl, 
>>> ACL_TYPE_DEFAULT);
>>> +             inode_unlock(inode);
>>> +             posix_acl_release(default_acl);
>>> +     }
>>> +
>>> +     if (acl) {
>>> +             if (!ret) {
>>> +                     inode_lock(inode);
> 
> Same here.

Same here :)

>>> +                     ret = ubifs_set_acl(inode, acl, 
>>> ACL_TYPE_ACCESS);
>>> +                     inode_unlock(inode);
>>> +             }
>>> +             posix_acl_release(acl);
>>> +     }
>>> +
>>> +     if (!default_acl && !acl)
>>> +             cache_no_acl(inode);
> 
> This call to cache_no_acl doesn't make sense; please check. It may be
> wrong in btrfs as well; copying Josef for that.

Both i_acl and i_default_acl are set to ACL_NOT_CACHED when entering
ubifs_init_acl().
As proposed, the function will be updated so cache_no_acl() will be
called unconditionally before setting the actual ACLs (if any).

>>> +     return ret;
>>> +}
>>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>>> index ccd9128..a2e5609 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;
>>> +
> 
> The pattern ubifs_budget_space / ubifs_new_inode / ubifs_init_acl /
> ubifs_init_security repeats a number of times in the code, so it
> should probably be put in a function.

I will look into it.

>>>       err = ubifs_init_security(dir, inode, &dentry->d_name);
>>>       if (err)
>>>               goto out_inode;
>>> @@ -341,6 +345,10 @@ static int do_tmpfile(struct inode *dir, struct 
>>> dentry *dentry,
>>>               ubifs_assert(inode->i_op == 
>>> &ubifs_file_inode_operations);
>>>       }
>>> 
>>> +     err = ubifs_init_acl(dir, inode);
>>> +     if (err)
>>> +             goto out_inode;
>> 
>> Why do we need ACLs for O_TMPFILE?
>> 
>>> +
>>>       err = ubifs_init_security(dir, inode, &dentry->d_name);
>>>       if (err)
>>>               goto out_inode;
>>> @@ -821,6 +829,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;
>>> @@ -903,6 +915,10 @@ static int ubifs_mknod(struct inode *dir, struct 
>>> dentry *dentry,
>>>       ui->data = dev;
>>>       ui->data_len = devlen;
>>> 
>>> +     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;
>>> @@ -985,6 +1001,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;
>> 
>> Do we really need ACLs for symlinks too? I thought permissions for 
>> symlinks are irrelevant.
>> 
>>>       err = ubifs_init_security(dir, inode, &dentry->d_name);
>>>       if (err)
>>>               goto out_inode;
>>> @@ -1395,6 +1415,8 @@ const struct inode_operations 
>>> ubifs_dir_inode_operations = {
>>>       .update_time = ubifs_update_time,
>>>  #endif
>>>       .tmpfile     = ubifs_tmpfile,
>>> +     .get_acl     = ubifs_get_acl,
>>> +     .set_acl     = ubifs_set_acl,
>>>  };
>>> 
>>>  const struct file_operations ubifs_dir_operations = {
>>> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
>>> index 4ed270b..17f6098 100644
>>> --- a/fs/ubifs/file.c
>>> +++ b/fs/ubifs/file.c
>>> @@ -53,6 +53,7 @@
>>>  #include <linux/mount.h>
>>>  #include <linux/slab.h>
>>>  #include <linux/migrate.h>
>>> +#include <linux/posix_acl.h>
>>> 
>>>  static int read_block(struct inode *inode, void *addr, unsigned int 
>>> block,
>>>                     struct ubifs_data_node *dn)
>>> @@ -1251,6 +1252,10 @@ static int do_setattr(struct ubifs_info *c, 
>>> struct inode *inode,
>>>               ubifs_release_budget(c, &req);
>>>       if (IS_SYNC(inode))
>>>               err = inode->i_sb->s_op->write_inode(inode, NULL);
>>> +
>>> +     if (!err && (attr->ia_valid & ATTR_MODE))
>>> +             err = posix_acl_chmod(inode, inode->i_mode);
>>> +
>>>       return err;
>>>  }
>>> 
>>> @@ -1628,6 +1633,8 @@ const struct inode_operations 
>>> ubifs_file_inode_operations = {
>>>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>>>       .update_time = ubifs_update_time,
>>>  #endif
>>> +     .get_acl     = ubifs_get_acl,
>>> +     .set_acl     = ubifs_set_acl,
>>>  };
>>> 
>>>  const struct inode_operations ubifs_symlink_inode_operations = {
>>> @@ -1642,6 +1649,8 @@ const struct inode_operations 
>>> ubifs_symlink_inode_operations = {
>>>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>>>       .update_time = ubifs_update_time,
>>>  #endif
>>> +     .get_acl     = ubifs_get_acl,
>>> +     .set_acl     = ubifs_set_acl,
>>>  };
>>> 
>>>  const struct file_operations ubifs_file_operations = {
>>> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
>>> index 4ec0510..8239dfd 100644
>>> --- a/fs/ubifs/super.c
>>> +++ b/fs/ubifs/super.c
>>> @@ -444,6 +444,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_puts(s, ",acl");
>>> +
>>>       return 0;
>>>  }
>>> 
>>> @@ -929,6 +932,8 @@ enum {
>>>       Opt_chk_data_crc,
>>>       Opt_no_chk_data_crc,
>>>       Opt_override_compr,
>>> +     Opt_acl,
>>> +     Opt_noacl,
>>>       Opt_err,
>>>  };
>>> 
>>> @@ -940,6 +945,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},
>>>  };
>>> 
>>> @@ -1040,6 +1047,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;
>>> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
>>> index 096035e..7935449 100644
>>> --- a/fs/ubifs/ubifs.h
>>> +++ b/fs/ubifs/ubifs.h
>>> @@ -1743,6 +1743,24 @@ extern const struct xattr_handler 
>>> *ubifs_xattr_handlers[];
>>>  ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t 
>>> size);
>>>  int ubifs_init_security(struct inode *dentry, struct inode *inode,
>>>                       const struct qstr *qstr);
>>> +ssize_t __ubifs_getxattr(struct inode *host, const char *name,
>>> +                             void *buf, size_t size);
>>> +int __ubifs_setxattr(struct inode *host, const char *name,
>>> +                         const void *value, size_t size, int flags);
>>> +
>>> +/* 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);
>>> diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
>>> index 6c2f4d4..0a596a1 100644
>>> --- a/fs/ubifs/xattr.c
>>> +++ b/fs/ubifs/xattr.c
>>> @@ -52,13 +52,14 @@
>>>   * in the VFS inode cache. The xentries are cached in the LNC cache 
>>> (see
>>>   * tnc.c).
>>>   *
>>> - * ACL support is not implemented.
>>> + * ACL support is now implemented.
>> 
>> I think we can just drop that line now. :-)
>> 
>>>   */
>>> 
>>>  #include "ubifs.h"
>>>  #include <linux/fs.h>
>>>  #include <linux/slab.h>
>>>  #include <linux/xattr.h>
>>> +#include <linux/posix_acl_xattr.h>
>> 
>> While we are here, these includes should also be cleaned up.
>> 
>>>  /*
>>>   * Limit the number of extended attributes per inode so that the 
>>> total size
>>> @@ -268,7 +269,7 @@ static struct inode *iget_xattr(struct ubifs_info 
>>> *c, ino_t inum)
>>>       return ERR_PTR(-EINVAL);
>>>  }
>>> 
>>> -static int __ubifs_setxattr(struct inode *host, const char *name,
>>> +int __ubifs_setxattr(struct inode *host, const char *name,
>>>                           const void *value, size_t size, int flags)
>>>  {
>>>       struct inode *inode;
>>> @@ -328,7 +329,7 @@ out_free:
>>>       return err;
>>>  }
>>> 
>>> -static ssize_t __ubifs_getxattr(struct inode *host, const char 
>>> *name,
>>> +ssize_t __ubifs_getxattr(struct inode *host, const char *name,
>>>                               void *buf, size_t size)
>>>  {
>>>       struct inode *inode;
>>> @@ -619,5 +620,9 @@ const struct xattr_handler 
>>> *ubifs_xattr_handlers[] = {
>>>       &ubifs_user_xattr_handler,
>>>       &ubifs_trusted_xattr_handler,
>>>       &ubifs_security_xattr_handler,
>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>>> +     &posix_acl_access_xattr_handler,
>>> +     &posix_acl_default_xattr_handler,
>>> +#endif
>>>       NULL
>>>  };
>>> 
>> 
>> Overall the patch does not look bad and I'm sure we can add it for 
>> v4.10 soon.
>> I'm a bit confused since most stuff is not really UBIFS specific.
>> Andreas, is just "normal" for ACL support? I thought most code is 
>> generic and already in VFS.
> 
> There are several filesystems like ubifs that use the xattr
> representation as the on-disk format, so ubifs_get_acl and
> ubifs_set_acl could be abstracted and shared across those filesystems.

I'd like that, unfortunately ubifs_[gs]et_acl() calls 
__ubifs_[gs]etxattr() which
are internal ubifs functions. I am not sure yet how this could be 
abstracted.
I will look into it too.

> The rest looks good.
> 
> Thanks,
> Andreas

ubifs: implement POSIX Access Control Lists (ACLs)

Commit Message

Comments

Patch