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

[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/20131201/19e00dc0/attachment.ksh>

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

[Steven Whitehouse]

Hi,

On Sun, 2013-12-01 at 03:59 -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>
> ---
>  fs/gfs2/acl.c   |  229 +++++++------------------------------------------------
>  fs/gfs2/acl.h   |    4 +-
>  fs/gfs2/inode.c |   33 ++++++--
>  fs/gfs2/xattr.c |    4 +-
>  4 files changed, 61 insertions(+), 209 deletions(-)
> 
Looks very good. I'd really like to be able to do something similar with
the security xattrs, in terms of the refactoring that at inode creation
to give the xattrs ahead of the inode allocation itself. That way it
should be possible to allocate the xattr blocks at the same time as the
inode, rather than as an after thought.

Some more comments below....

> diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
> index e82e4ac..e6c7a2c 100644
> --- a/fs/gfs2/acl.c
> +++ b/fs/gfs2/acl.c
[snip]
> -
> -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)
>  
Andy Price has pointed out a missing "return error;" here

> -		if (error <= 0) {
> -			posix_acl_release(acl);
> +		if (error == 0)
>  			acl = NULL;
>  
> -			if (error < 0)
> -				return error;
> -		}
> -

Also, there seems to be a white space error in the xfs patch around line
170 in fs/xfs/xfs_iops.c where there is an added "if (default_acl)" with
a space before the tab,

Steve.

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

[Christoph Hellwig]

On Wed, Dec 04, 2013 at 12:12:37PM +0000, Steven Whitehouse wrote:
> >  		error = posix_acl_equiv_mode(acl, &mode);
> > +		if (error < 0)
> >  
> Andy Price has pointed out a missing "return error;" here
> 
> > -		if (error <= 0) {
> > -			posix_acl_release(acl);
> > +		if (error == 0)
> >  			acl = NULL;
> >  
> > -			if (error < 0)
> > -				return error;
> > -		}
> > -
> 
> Also, there seems to be a white space error in the xfs patch around line
> 170 in fs/xfs/xfs_iops.c where there is an added "if (default_acl)" with
> a space before the tab,

I'll take care of these for the next version.

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