[Cluster-devel] [PATCH 00/18] Consolidate Posix ACL implementation V2

[Cluster-devel] [PATCH 00/18] Consolidate Posix ACL implementation V2

[Christoph Hellwig]

This series consolidates the various cut'n'pasted Posix ACL implementations
into a single common one based on the ->get_acl method Linus added a while
ago and a new ->set_acl counterpart.

This remove ~1800 lines of code and provides a single place to implement
various nasty little gems of the semantics.

Unfortunately the 9p code is still left out - it implements the ACLs
in two very weird ways, one using the common code but on the client only,
and one pasing things straight through to the server.  We could easily
convert it to the new code on the write side if ->set_acl took a dentry,
but there's no cance to do that on the ->get_acl side.  Ideas how to
handle it welcome.

After that we'd be ready to never go into the fs for the ACL attributes
and branch straight to the ACL code below the syscall, repairing the
old API braindamage of overloading ACLs onto the xattrs.


Changes from V1:
 - check for symlinks in the ACL code and remove checks in the lower
   level functions.
 - remove get_acl instances for symlinks in a few filesystems
 - pass a umode_t mode argument to posix_acl_chmod to accomodate f2fs
 - various cosemtic bits from the reviews.

Note that I still haven't heard from ocfs2 folks, so the patch is left
unchanged.

[Cluster-devel] [PATCH 01/18] reiserfs: prefix ACL symbols with reiserfs_

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0001-reiserfs-prefix-ACL-symbols-with-reiserfs_.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/35841bc7/attachment.ksh>

[Cluster-devel] [PATCH 02/18] fs: add get_acl helper

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0002-fs-add-get_acl-helper.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/2311bc1c/attachment.ksh>

[Cluster-devel] [PATCH 02/18] fs: add get_acl helper

[Andreas Gruenbacher]

Christoph,

> +struct posix_acl *get_acl(struct inode *inode, int type)
> +{
> +	struct posix_acl *acl;
> +
> +	acl = get_cached_acl(inode, type);
> +	if (acl != ACL_NOT_CACHED)
> +		return acl;
> +
> +	if (!IS_POSIXACL(inode))
> +		return NULL;
> +
> +	/*
> +	 * A filesystem can force a ACL callback by just never filling the
> +	 * ACL cache. But normally you'd fill the cache either at inode
> +	 * instantiation time, or on the first ->get_acl call.
> +	 *
> +	 * If the filesystem doesn't have a get_acl() function at all, we'll
> +	 * just create the negative cache entry.
> +	 */
> +	if (!inode->i_op->get_acl) {
> +		set_cached_acl(inode, type, NULL);
> +		return ERR_PTR(-EAGAIN);

The function should return NULL here.

Andreas

[Cluster-devel] [PATCH 02/18] fs: add get_acl helper

[Christoph Hellwig]

On Thu, Dec 12, 2013 at 08:06:09PM +0100, Andreas Gruenbacher wrote:
> > +	/*
> > +	 * A filesystem can force a ACL callback by just never filling the
> > +	 * ACL cache. But normally you'd fill the cache either at inode
> > +	 * instantiation time, or on the first ->get_acl call.
> > +	 *
> > +	 * If the filesystem doesn't have a get_acl() function at all, we'll
> > +	 * just create the negative cache entry.
> > +	 */
> > +	if (!inode->i_op->get_acl) {
> > +		set_cached_acl(inode, type, NULL);
> > +		return ERR_PTR(-EAGAIN);
> 
> The function should return NULL here.

Indeed.  EAGAIN is the convention check_acl() in fs/namei.c uses,
but it will return that automatically if we just return NULL here.

[Cluster-devel] [PATCH 03/18] fs: add a set_acl inode operation

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0003-fs-add-a-set_acl-inode-operation.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/12450327/attachment.ksh>

[Cluster-devel] [PATCH 04/18] fs: add generic xattr_acl handlers

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0004-fs-add-generic-xattr_acl-handlers.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/91db43bc/attachment.ksh>

[Cluster-devel] [PATCH 04/18] fs: add generic xattr_acl handlers

[Andreas Gruenbacher]

Christoph,

> +static int
> +posix_acl_xattr_set(struct dentry *dentry, const char *name,
> +		const void *value, size_t size, int flags, int type)
> +{
> +	struct inode *inode = dentry->d_inode;
> +	struct posix_acl *acl = NULL;
> +	int ret;
> +
> +	if (!IS_POSIXACL(inode))
> +		return -EOPNOTSUPP;
> +	if (S_ISLNK(inode->i_mode) || !inode->i_op->set_acl)
> +		return -EOPNOTSUPP;

Sama here, I would remove the S_ISLNK() check.

Andreas

[Cluster-devel] [PATCH 05/18] fs: make posix_acl_chmod more useful

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0005-fs-make-posix_acl_chmod-more-useful.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/176a07d3/attachment.ksh>

[Cluster-devel] [PATCH 05/18] fs: make posix_acl_chmod more useful

[Andreas Gruenbacher]

Christoph,

> +int
> +posix_acl_chmod(struct inode *inode)
> +{
> +	struct posix_acl *acl;
> +	int ret = 0;
> +
> +	if (S_ISLNK(inode->i_mode) || !inode->i_op->set_acl)
> +		return -EOPNOTSUPP;

Symlinks never have get_acl callbacks, so I would remove the S_ISLNK() check here.

Andreas

[Cluster-devel] [PATCH 05/18] fs: make posix_acl_chmod more useful

[Christoph Hellwig]

On Thu, Dec 12, 2013 at 08:07:20PM +0100, Andreas Gruenbacher wrote:
> Christoph,
> 
> > +int
> > +posix_acl_chmod(struct inode *inode)
> > +{
> > +	struct posix_acl *acl;
> > +	int ret = 0;
> > +
> > +	if (S_ISLNK(inode->i_mode) || !inode->i_op->set_acl)
> > +		return -EOPNOTSUPP;
> 
> Symlinks never have get_acl callbacks, so I would remove the S_ISLNK() check here.

Yeah, will simplify it.  Same for the other places where we have both
checks.

[Cluster-devel] [PATCH 06/18] fs: make posix_acl_create more useful

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0006-fs-make-posix_acl_create-more-useful.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/eb09193e/attachment.ksh>

[Cluster-devel] [PATCH 07/18] btrfs: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0007-btrfs-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/876d757d/attachment.ksh>

[Cluster-devel] [PATCH 08/18] ext2/3/4: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0008-ext2-3-4-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/6d2a563e/attachment.ksh>

[Cluster-devel] [PATCH 09/18] f2fs: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0009-f2fs-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/2cec2935/attachment.ksh>

[Cluster-devel] [PATCH 10/18] hfsplus: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0010-hfsplus-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/d51bfb09/attachment.ksh>

[Cluster-devel] [PATCH 11/18] jffs2: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0011-jffs2-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/a3efba51/attachment.ksh>

[Cluster-devel] [PATCH 12/18] ocfs2: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0012-ocfs2-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/88e1778c/attachment.ksh>

[Cluster-devel] [PATCH 13/18] reiserfs: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0013-reiserfs-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/6aa889e6/attachment.ksh>

[Cluster-devel] [PATCH 14/18] xfs: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0014-xfs-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/9a6a7104/attachment.ksh>

[Cluster-devel] [PATCH 15/18] jfs: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0015-jfs-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/c6ddfc3b/attachment.ksh>

[Cluster-devel] [PATCH 16/18] gfs2: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0016-gfs2-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/aafe34fb/attachment.ksh>

[Cluster-devel] [PATCH 16/18] gfs2: use generic posix ACL infrastructure

[Steven Whitehouse]

Hi,

On Wed, 2013-12-11 at 02:42 -0800, Christoph Hellwig wrote:
> plain text document attachment
> (0016-gfs2-use-generic-posix-ACL-infrastructure.patch)
> This contains some major refactoring for the create path so that
> inodes are created with the right mode to start with instead of
> fixing it up later.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Steven Whitehouse <swhiteho@redhat.com>

A really nice clean up - this is a very useful step forward in
simplifying the create path. Thanks for sorting this out,

Steve.

> ---
>  fs/gfs2/acl.c   |  234 +++++++------------------------------------------------
>  fs/gfs2/acl.h   |    4 +-
>  fs/gfs2/inode.c |   33 ++++++--
>  fs/gfs2/xattr.c |    4 +-
>  4 files changed, 62 insertions(+), 213 deletions(-)
> 
> diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
> index e82e4ac..ba94566 100644
> --- a/fs/gfs2/acl.c
> +++ b/fs/gfs2/acl.c
> @@ -49,10 +49,6 @@ struct posix_acl *gfs2_get_acl(struct inode *inode, int type)
>  	if (!ip->i_eattr)
>  		return NULL;
>  
> -	acl = get_cached_acl(&ip->i_inode, type);
> -	if (acl != ACL_NOT_CACHED)
> -		return acl;
> -
>  	name = gfs2_acl_name(type);
>  	if (name == NULL)
>  		return ERR_PTR(-EINVAL);
> @@ -80,7 +76,7 @@ static int gfs2_set_mode(struct inode *inode, umode_t mode)
>  	return error;
>  }
>  
> -static int gfs2_acl_set(struct inode *inode, int type, struct posix_acl *acl)
> +int gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  {
>  	int error;
>  	int len;
> @@ -88,219 +84,49 @@ static int gfs2_acl_set(struct inode *inode, int type, struct posix_acl *acl)
>  	const char *name = gfs2_acl_name(type);
>  
>  	BUG_ON(name == NULL);
> -	len = posix_acl_to_xattr(&init_user_ns, acl, NULL, 0);
> -	if (len == 0)
> -		return 0;
> -	data = kmalloc(len, GFP_NOFS);
> -	if (data == NULL)
> -		return -ENOMEM;
> -	error = posix_acl_to_xattr(&init_user_ns, acl, data, len);
> -	if (error < 0)
> -		goto out;
> -	error = __gfs2_xattr_set(inode, name, data, len, 0, GFS2_EATYPE_SYS);
> -	if (!error)
> -		set_cached_acl(inode, type, acl);
> -out:
> -	kfree(data);
> -	return error;
> -}
> -
> -int gfs2_acl_create(struct gfs2_inode *dip, struct inode *inode)
> -{
> -	struct gfs2_sbd *sdp = GFS2_SB(&dip->i_inode);
> -	struct posix_acl *acl;
> -	umode_t mode = inode->i_mode;
> -	int error = 0;
> -
> -	if (!sdp->sd_args.ar_posix_acl)
> -		return 0;
> -	if (S_ISLNK(inode->i_mode))
> -		return 0;
> -
> -	acl = gfs2_get_acl(&dip->i_inode, ACL_TYPE_DEFAULT);
> -	if (IS_ERR(acl))
> -		return PTR_ERR(acl);
> -	if (!acl) {
> -		mode &= ~current_umask();
> -		return gfs2_set_mode(inode, mode);
> -	}
> -
> -	if (S_ISDIR(inode->i_mode)) {
> -		error = gfs2_acl_set(inode, ACL_TYPE_DEFAULT, acl);
> -		if (error)
> -			goto out;
> -	}
> -
> -	error = __posix_acl_create(&acl, GFP_NOFS, &mode);
> -	if (error < 0)
> -		return error;
>  
> -	if (error == 0)
> -		goto munge;
> -
> -	error = gfs2_acl_set(inode, ACL_TYPE_ACCESS, acl);
> -	if (error)
> -		goto out;
> -munge:
> -	error = gfs2_set_mode(inode, mode);
> -out:
> -	posix_acl_release(acl);
> -	return error;
> -}
> -
> -int gfs2_acl_chmod(struct gfs2_inode *ip, struct iattr *attr)
> -{
> -	struct inode *inode = &ip->i_inode;
> -	struct posix_acl *acl;
> -	char *data;
> -	unsigned int len;
> -	int error;
> -
> -	acl = gfs2_get_acl(&ip->i_inode, ACL_TYPE_ACCESS);
> -	if (IS_ERR(acl))
> -		return PTR_ERR(acl);
> -	if (!acl)
> -		return gfs2_setattr_simple(inode, attr);
> -
> -	error = __posix_acl_chmod(&acl, GFP_NOFS, attr->ia_mode);
> -	if (error)
> -		return error;
> -
> -	len = posix_acl_to_xattr(&init_user_ns, acl, NULL, 0);
> -	data = kmalloc(len, GFP_NOFS);
> -	error = -ENOMEM;
> -	if (data == NULL)
> -		goto out;
> -	posix_acl_to_xattr(&init_user_ns, acl, data, len);
> -	error = gfs2_xattr_acl_chmod(ip, attr, data);
> -	kfree(data);
> -	set_cached_acl(&ip->i_inode, ACL_TYPE_ACCESS, acl);
> -
> -out:
> -	posix_acl_release(acl);
> -	return error;
> -}
> -
> -static int gfs2_acl_type(const char *name)
> -{
> -	if (strcmp(name, GFS2_POSIX_ACL_ACCESS) == 0)
> -		return ACL_TYPE_ACCESS;
> -	if (strcmp(name, GFS2_POSIX_ACL_DEFAULT) == 0)
> -		return ACL_TYPE_DEFAULT;
> -	return -EINVAL;
> -}
> -
> -static int gfs2_xattr_system_get(struct dentry *dentry, const char *name,
> -				 void *buffer, size_t size, int xtype)
> -{
> -	struct inode *inode = dentry->d_inode;
> -	struct gfs2_sbd *sdp = GFS2_SB(inode);
> -	struct posix_acl *acl;
> -	int type;
> -	int error;
> -
> -	if (!sdp->sd_args.ar_posix_acl)
> -		return -EOPNOTSUPP;
> -
> -	type = gfs2_acl_type(name);
> -	if (type < 0)
> -		return type;
> -
> -	acl = gfs2_get_acl(inode, type);
> -	if (IS_ERR(acl))
> -		return PTR_ERR(acl);
> -	if (acl == NULL)
> -		return -ENODATA;
> -
> -	error = posix_acl_to_xattr(&init_user_ns, acl, buffer, size);
> -	posix_acl_release(acl);
> -
> -	return error;
> -}
> -
> -static int gfs2_xattr_system_set(struct dentry *dentry, const char *name,
> -				 const void *value, size_t size, int flags,
> -				 int xtype)
> -{
> -	struct inode *inode = dentry->d_inode;
> -	struct gfs2_sbd *sdp = GFS2_SB(inode);
> -	struct posix_acl *acl = NULL;
> -	int error = 0, type;
> -
> -	if (!sdp->sd_args.ar_posix_acl)
> -		return -EOPNOTSUPP;
> -
> -	type = gfs2_acl_type(name);
> -	if (type < 0)
> -		return type;
> -	if (flags & XATTR_CREATE)
> -		return -EINVAL;
> -	if (type == ACL_TYPE_DEFAULT && !S_ISDIR(inode->i_mode))
> -		return value ? -EACCES : 0;
> -	if (!uid_eq(current_fsuid(), inode->i_uid) && !capable(CAP_FOWNER))
> -		return -EPERM;
> -	if (S_ISLNK(inode->i_mode))
> -		return -EOPNOTSUPP;
> -
> -	if (!value)
> -		goto set_acl;
> -
> -	acl = posix_acl_from_xattr(&init_user_ns, value, size);
> -	if (!acl) {
> -		/*
> -		 * acl_set_file(3) may request that we set default ACLs with
> -		 * zero length -- defend (gracefully) against that here.
> -		 */
> -		goto out;
> -	}
> -	if (IS_ERR(acl)) {
> -		error = PTR_ERR(acl);
> -		goto out;
> -	}
> -
> -	error = posix_acl_valid(acl);
> -	if (error)
> -		goto out_release;
> -
> -	error = -EINVAL;
>  	if (acl->a_count > GFS2_ACL_MAX_ENTRIES)
> -		goto out_release;
> +		return -EINVAL;
>  
>  	if (type == ACL_TYPE_ACCESS) {
>  		umode_t mode = inode->i_mode;
> +
>  		error = posix_acl_equiv_mode(acl, &mode);
> +		if (error < 0)
> +			return error;
>  
> -		if (error <= 0) {
> -			posix_acl_release(acl);
> +		if (error == 0)
>  			acl = NULL;
>  
> -			if (error < 0)
> -				return error;
> -		}
> -
>  		error = gfs2_set_mode(inode, mode);
>  		if (error)
> -			goto out_release;
> +			return error;
>  	}
>  
> -set_acl:
> -	error = __gfs2_xattr_set(inode, name, value, size, 0, GFS2_EATYPE_SYS);
> -	if (!error) {
> -		if (acl)
> -			set_cached_acl(inode, type, acl);
> -		else
> -			forget_cached_acl(inode, type);
> +	if (acl) {
> +		len = posix_acl_to_xattr(&init_user_ns, acl, NULL, 0);
> +		if (len == 0)
> +			return 0;
> +		data = kmalloc(len, GFP_NOFS);
> +		if (data == NULL)
> +			return -ENOMEM;
> +		error = posix_acl_to_xattr(&init_user_ns, acl, data, len);
> +		if (error < 0)
> +			goto out;
> +	} else {
> +		data = NULL;
> +		len = 0;
>  	}
> -out_release:
> -	posix_acl_release(acl);
> +
> +	error = __gfs2_xattr_set(inode, name, data, len, 0, GFS2_EATYPE_SYS);
> +	if (error)
> +		goto out;
> +
> +	if (acl)
> +		set_cached_acl(inode, type, acl);
> +	else
> +		forget_cached_acl(inode, type);
>  out:
> +	kfree(data);
>  	return error;
>  }
> -
> -const struct xattr_handler gfs2_xattr_system_handler = {
> -	.prefix = XATTR_SYSTEM_PREFIX,
> -	.flags  = GFS2_EATYPE_SYS,
> -	.get    = gfs2_xattr_system_get,
> -	.set    = gfs2_xattr_system_set,
> -};
> -
> diff --git a/fs/gfs2/acl.h b/fs/gfs2/acl.h
> index 0da38dc..301260c 100644
> --- a/fs/gfs2/acl.h
> +++ b/fs/gfs2/acl.h
> @@ -17,8 +17,6 @@
>  #define GFS2_ACL_MAX_ENTRIES		25
>  
>  extern struct posix_acl *gfs2_get_acl(struct inode *inode, int type);
> -extern int gfs2_acl_create(struct gfs2_inode *dip, struct inode *inode);
> -extern int gfs2_acl_chmod(struct gfs2_inode *ip, struct iattr *attr);
> -extern const struct xattr_handler gfs2_xattr_system_handler;
> +extern int gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type);
>  
>  #endif /* __ACL_DOT_H__ */
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 7119504..92b8959 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -552,6 +552,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
>  			     unsigned int size, int excl, int *opened)
>  {
>  	const struct qstr *name = &dentry->d_name;
> +	struct posix_acl *default_acl, *acl;
>  	struct gfs2_holder ghs[2];
>  	struct inode *inode = NULL;
>  	struct gfs2_inode *dip = GFS2_I(dir), *ip;
> @@ -611,10 +612,14 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
>  	if (!inode)
>  		goto fail_gunlock;
>  
> +	error = posix_acl_create(dir, &mode, &default_acl, &acl);
> +	if (error)
> +		goto fail_free_vfs_inode;
> +
>  	ip = GFS2_I(inode);
>  	error = gfs2_rs_alloc(ip);
>  	if (error)
> -		goto fail_free_inode;
> +		goto fail_free_acls;
>  
>  	inode->i_mode = mode;
>  	set_nlink(inode, S_ISDIR(mode) ? 2 : 1);
> @@ -682,7 +687,16 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
>  	gfs2_set_iop(inode);
>  	insert_inode_hash(inode);
>  
> -	error = gfs2_acl_create(dip, inode);
> +	if (default_acl) {
> +		error = gfs2_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
> +		posix_acl_release(default_acl);
> +	}
> +	if (acl) {
> +		if (!error)
> +			error = gfs2_set_acl(inode, acl, ACL_TYPE_ACCESS);
> +		posix_acl_release(acl);
> +	}
> +
>  	if (error)
>  		goto fail_gunlock3;
>  
> @@ -716,6 +730,12 @@ fail_free_inode:
>  	if (ip->i_gl)
>  		gfs2_glock_put(ip->i_gl);
>  	gfs2_rs_delete(ip, NULL);
> +fail_free_acls:
> +	if (default_acl)
> +		posix_acl_release(default_acl);
> +	if (acl)
> +		posix_acl_release(acl);
> +fail_free_vfs_inode:
>  	free_inode_nonrcu(inode);
>  	inode = NULL;
>  fail_gunlock:
> @@ -1678,10 +1698,11 @@ static int gfs2_setattr(struct dentry *dentry, struct iattr *attr)
>  		error = gfs2_setattr_size(inode, attr->ia_size);
>  	else if (attr->ia_valid & (ATTR_UID | ATTR_GID))
>  		error = setattr_chown(inode, attr);
> -	else if ((attr->ia_valid & ATTR_MODE) && IS_POSIXACL(inode))
> -		error = gfs2_acl_chmod(ip, attr);
> -	else
> +	else {
>  		error = gfs2_setattr_simple(inode, attr);
> +		if (!error && attr->ia_valid & ATTR_MODE)
> +			error = posix_acl_chmod(inode, inode->i_mode);
> +	}
>  
>  out:
>  	if (!error)
> @@ -1841,6 +1862,7 @@ const struct inode_operations gfs2_file_iops = {
>  	.removexattr = gfs2_removexattr,
>  	.fiemap = gfs2_fiemap,
>  	.get_acl = gfs2_get_acl,
> +	.set_acl = gfs2_set_acl,
>  };
>  
>  const struct inode_operations gfs2_dir_iops = {
> @@ -1862,6 +1884,7 @@ const struct inode_operations gfs2_dir_iops = {
>  	.removexattr = gfs2_removexattr,
>  	.fiemap = gfs2_fiemap,
>  	.get_acl = gfs2_get_acl,
> +	.set_acl = gfs2_set_acl,
>  	.atomic_open = gfs2_atomic_open,
>  };
>  
> diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
> index 8c6a6f6..0b81f78 100644
> --- a/fs/gfs2/xattr.c
> +++ b/fs/gfs2/xattr.c
> @@ -13,6 +13,7 @@
>  #include <linux/buffer_head.h>
>  #include <linux/xattr.h>
>  #include <linux/gfs2_ondisk.h>
> +#include <linux/posix_acl_xattr.h>
>  #include <asm/uaccess.h>
>  
>  #include "gfs2.h"
> @@ -1500,7 +1501,8 @@ static const struct xattr_handler gfs2_xattr_security_handler = {
>  const struct xattr_handler *gfs2_xattr_handlers[] = {
>  	&gfs2_xattr_user_handler,
>  	&gfs2_xattr_security_handler,
> -	&gfs2_xattr_system_handler,
> +	&posix_acl_access_xattr_handler,
> +	&posix_acl_default_xattr_handler,
>  	NULL,
>  };
>  

[Cluster-devel] [PATCH 16/18] gfs2: use generic posix ACL infrastructure

[Andreas Gruenbacher]

Christoph,

gfs2 has a left-over get_acl callback in gfs2_symlink_iops in
fs/gfs2/inode.c, from a long time ago, which should be removed
as well.

Andreas

[Cluster-devel] [PATCH 16/18] gfs2: use generic posix ACL infrastructure

[Christoph Hellwig]

On Thu, Dec 12, 2013 at 08:08:38PM +0100, Andreas Gruenbacher wrote:
> Christoph,
> 
> gfs2 has a left-over get_acl callback in gfs2_symlink_iops in
> fs/gfs2/inode.c, from a long time ago, which should be removed
> as well.

Ok, will fix.

[Cluster-devel] [PATCH 17/18] nfs: use generic posix ACL infrastructure for v3 Posix ACLs

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0017-nfs-use-generic-posix-ACL-infrastructure-for-v3-Posi.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/ce18263a/attachment.ksh>

[Cluster-devel] [PATCH 18/18] fs: remove generic_acl

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0018-fs-remove-generic_acl.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/ff393c13/attachment.ksh>

[Cluster-devel] [PATCH 00/18] Consolidate Posix ACL implementation

[Christoph Hellwig]

This series consolidates the various cut'n'pasted Posix ACL implementations
into a single common one based on the ->get_acl method Linus added a while
ago and a new ->set_acl counterpart.

This 1600 lines of code and provides a single place to implement various
nasty little gems of the semantics.

Unfortunately the 9p code is still left out - it implements the ACLs
in two very weird ways, one using the common code but on the client only,
and one pasing things straight through to the server.  We could easily
convert it to the new code on the write side if ->set_acl took a dentry,
but there's no cance to do that on the ->get_acl side.  Ideas how to
handle it welcome.

After that we'd be ready to never go into the fs for the ACL attributes
and branch straight to the ACL code below the syscall, repairing the
old API braindamage of overloading ACLs onto the xattrs.

Btw, I'd be almost tempted to do that for all system.* attrs.  Besides
Posix ACLs we only have CIFS and NFSv4 ACL variants, weird advice crap
in f2fs, and the magic mushroom proto name on sockets.

[Cluster-devel] [PATCH 04/18] fs: add generic xattr_acl handlers

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0004-fs-add-generic-xattr_acl-handlers.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131201/ae723c7a/attachment.ksh>

[Cluster-devel] [PATCH 04/18] fs: add generic xattr_acl handlers

[Jan Kara]

On Sun 01-12-13 03:59:07, Christoph Hellwig wrote:
> With the ->set_acl inode operation we can implement the Posix ACL
> xattr handlers in generic code instead of duplicating them all
> over the tree.
  Looks good. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>

							Honza

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xattr_acl.c                  |   95 +++++++++++++++++++++++++++++++++++++++
>  include/linux/posix_acl_xattr.h |    3 ++
>  2 files changed, 98 insertions(+)
> 
> diff --git a/fs/xattr_acl.c b/fs/xattr_acl.c
> index 9fbea87..932ec76 100644
> --- a/fs/xattr_acl.c
> +++ b/fs/xattr_acl.c
> @@ -10,6 +10,7 @@
>  #include <linux/posix_acl_xattr.h>
>  #include <linux/gfp.h>
>  #include <linux/user_namespace.h>
> +#include <linux/xattr.h>
>  
>  /*
>   * Fix up the uids and gids in posix acl extended attributes in place.
> @@ -178,3 +179,97 @@ posix_acl_to_xattr(struct user_namespace *user_ns, const struct posix_acl *acl,
>  	return real_size;
>  }
>  EXPORT_SYMBOL (posix_acl_to_xattr);
> +
> +static int
> +posix_acl_xattr_get(struct dentry *dentry, const char *name,
> +		void *value, size_t size, int type)
> +{
> +	struct posix_acl *acl;
> +	int error;
> +
> +	if (!IS_POSIXACL(dentry->d_inode))
> +		return -EOPNOTSUPP;
> +
> +	acl = get_acl(dentry->d_inode, type);
> +	if (IS_ERR(acl))
> +		return PTR_ERR(acl);
> +	if (acl == NULL)
> +		return -ENODATA;
> +
> +	error = posix_acl_to_xattr(&init_user_ns, acl, value, size);
> +	posix_acl_release(acl);
> +
> +	return error;
> +}
> +
> +static int
> +posix_acl_xattr_set(struct dentry *dentry, const char *name,
> +		const void *value, size_t size, int flags, int type)
> +{
> +	struct inode *inode = dentry->d_inode;
> +	struct posix_acl *acl = NULL;
> +	int ret;
> +
> +	if (type == ACL_TYPE_DEFAULT && !S_ISDIR(inode->i_mode))
> +		return value ? -EACCES : 0;
> +	if (!inode_owner_or_capable(inode))
> +		return -EPERM;
> +	if (!IS_POSIXACL(inode))
> +		return -EOPNOTSUPP;
> +
> +	if (value) {
> +		acl = posix_acl_from_xattr(&init_user_ns, value, size);
> +		if (IS_ERR(acl))
> +			return PTR_ERR(acl);
> +
> +		if (acl) {
> +			ret = posix_acl_valid(acl);
> +			if (ret)
> +				goto out;
> +		}
> +	}
> +
> +	ret = inode->i_op->set_acl(inode, acl, type);
> +out:
> +	posix_acl_release(acl);
> +	return ret;
> +}
> +
> +static size_t
> +posix_acl_xattr_list(struct dentry *dentry, char *list, size_t list_size,
> +		const char *name, size_t name_len, int type)
> +{
> +	const char *xname;
> +	size_t size;
> +
> +	if (!IS_POSIXACL(dentry->d_inode))
> +		return -EOPNOTSUPP;
> +
> +	if (type == ACL_TYPE_ACCESS)
> +		xname = POSIX_ACL_XATTR_ACCESS;
> +	else
> +		xname = POSIX_ACL_XATTR_DEFAULT;
> +
> +	size = strlen(xname) + 1;
> +	if (list && size <= list_size)
> +		memcpy(list, xname, size);
> +	return size;
> +}
> +
> +const struct xattr_handler posix_acl_access_xattr_handler = {
> +	.prefix = POSIX_ACL_XATTR_ACCESS,
> +	.flags = ACL_TYPE_ACCESS,
> +	.list = posix_acl_xattr_list,
> +	.get = posix_acl_xattr_get,
> +	.set = posix_acl_xattr_set,
> +};
> +EXPORT_SYMBOL_GPL(posix_acl_access_xattr_handler);
> +
> +const struct xattr_handler posix_acl_default_xattr_handler = {
> +	.prefix = POSIX_ACL_XATTR_DEFAULT,
> +	.flags = ACL_TYPE_DEFAULT,
> +	.list = posix_acl_xattr_list,
> +	.get = posix_acl_xattr_get,
> +	.set = posix_acl_xattr_set,
> +};
> +EXPORT_SYMBOL_GPL(posix_acl_default_xattr_handler);
> diff --git a/include/linux/posix_acl_xattr.h b/include/linux/posix_acl_xattr.h
> index ad93ad0..6f14ee2 100644
> --- a/include/linux/posix_acl_xattr.h
> +++ b/include/linux/posix_acl_xattr.h
> @@ -69,4 +69,7 @@ struct posix_acl *posix_acl_from_xattr(struct user_namespace *user_ns,
>  int posix_acl_to_xattr(struct user_namespace *user_ns,
>  		       const struct posix_acl *acl, void *buffer, size_t size);
>  
> +extern const struct xattr_handler posix_acl_access_xattr_handler;
> +extern const struct xattr_handler posix_acl_default_xattr_handler;
> +
>  #endif	/* _POSIX_ACL_XATTR_H */
> -- 
> 1.7.10.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR