Re: [PATCH v18 21/22] ext4: Add richacl support

2016-03-19 Thread Andreas Gruenbacher 
On Tue, Mar 15, 2016 at 8:17 AM, Christoph Hellwig  wrote:
> On Mon, Mar 14, 2016 at 12:08:31AM +0100, Andreas Gruenbacher wrote:
>> The xattr representation is the same on disk and at the xattr syscall
>> layer, and so richacl_from_xattr is used for converting into the
>> in-memory representation in both cases. The error codes are not the
>> same when a user supplies an invalid value via setxattr or NFS and
>> when an invalid xattr is read from disk though. I'll add a parameter
>> to richacl_from_xattr to make this more explicit.
>
> Better add a wrapper instead of a parameter.
>
>>
>> >> +static int
>> >> +__ext4_set_richacl(handle_t *handle, struct inode *inode, struct richacl 
>> >> *acl)
>> >> +{
>> >> + const int name_index = EXT4_XATTR_INDEX_RICHACL;
>> >> + umode_t mode = inode->i_mode;
>> >> + int retval, size;
>> >> + void *value;
>> >> +
>> >> + if (richacl_equiv_mode(acl, ) == 0) {
>> >> + inode->i_ctime = ext4_current_time(inode);
>> >> + inode->i_mode = mode;
>> >> + ext4_mark_inode_dirty(handle, inode);
>> >> + return __ext4_remove_richacl(handle, inode);
>> >> + }
>> >
>> > Should this check for a NULL acl instead of special casing that
>> > in ext4_set_richacl?
>>
>> I'm not sure I understand what you mean. When the
>
> ext4_set_richacl checks for a NULL acl pointer and then calls into
> __ext4_remove_richacl.  I'd rather have that special casing in one
> place.

Those are two different cases: the first is where ext4_set_richacl is
called with a NULL acl to remove an existing ACL; the second is where
ext4_set_richacl is called with a mode-equivalent ACL to set the mode
and remove any existing ACL.

The check for mode-equivalent ACLs is in __ext4_set_richacl and not in
ext4_set_richacl because an inherited ACL (ext4_init_acl) can also be
mode-equivalent.

Andreas

Re: [PATCH v18 21/22] ext4: Add richacl support

2016-03-19 Thread Andreas Gruenbacher 
On Tue, Mar 15, 2016 at 8:17 AM, Christoph Hellwig  wrote:
> On Mon, Mar 14, 2016 at 12:08:31AM +0100, Andreas Gruenbacher wrote:
>> The xattr representation is the same on disk and at the xattr syscall
>> layer, and so richacl_from_xattr is used for converting into the
>> in-memory representation in both cases. The error codes are not the
>> same when a user supplies an invalid value via setxattr or NFS and
>> when an invalid xattr is read from disk though. I'll add a parameter
>> to richacl_from_xattr to make this more explicit.
>
> Better add a wrapper instead of a parameter.
>
>>
>> >> +static int
>> >> +__ext4_set_richacl(handle_t *handle, struct inode *inode, struct richacl 
>> >> *acl)
>> >> +{
>> >> + const int name_index = EXT4_XATTR_INDEX_RICHACL;
>> >> + umode_t mode = inode->i_mode;
>> >> + int retval, size;
>> >> + void *value;
>> >> +
>> >> + if (richacl_equiv_mode(acl, ) == 0) {
>> >> + inode->i_ctime = ext4_current_time(inode);
>> >> + inode->i_mode = mode;
>> >> + ext4_mark_inode_dirty(handle, inode);
>> >> + return __ext4_remove_richacl(handle, inode);
>> >> + }
>> >
>> > Should this check for a NULL acl instead of special casing that
>> > in ext4_set_richacl?
>>
>> I'm not sure I understand what you mean. When the
>
> ext4_set_richacl checks for a NULL acl pointer and then calls into
> __ext4_remove_richacl.  I'd rather have that special casing in one
> place.

Those are two different cases: the first is where ext4_set_richacl is
called with a NULL acl to remove an existing ACL; the second is where
ext4_set_richacl is called with a mode-equivalent ACL to set the mode
and remove any existing ACL.

The check for mode-equivalent ACLs is in __ext4_set_richacl and not in
ext4_set_richacl because an inherited ACL (ext4_init_acl) can also be
mode-equivalent.

Andreas

Re: [PATCH v18 21/22] ext4: Add richacl support

2016-03-15 Thread Christoph Hellwig 
On Mon, Mar 14, 2016 at 02:02:33PM +0100, Andreas Gruenbacher wrote:
> On Fri, Mar 11, 2016 at 3:27 PM, Christoph Hellwig  wrote:
> >> +static inline int
> >> +ext4_acl_chmod(struct inode *inode, umode_t mode)
> >> +{
> >> + if (IS_RICHACL(inode))
> >> + return richacl_chmod(inode, inode->i_mode);
> >> + return posix_acl_chmod(inode, inode->i_mode);
> >> +}
> >
> > Thi isn't ext4-specific and potentially duplicated in every caller.
> > Please provide this as a common helper.
> 
> This can go in neither fs.h nor posix_acl.h nor richacl.h unless we
> turn it into a macro, and I don't think we want to add a new header
> file for such extreme trivia.

I'd expect us to grow a few more of thos helper if we get the sharing
right (either a real common base object, or wrappers for anything
dealing with the acl pointers in the inode), so a new linux/acl.h
should be fine.

Re: [PATCH v18 21/22] ext4: Add richacl support

2016-03-15 Thread Christoph Hellwig 
On Mon, Mar 14, 2016 at 02:02:33PM +0100, Andreas Gruenbacher wrote:
> On Fri, Mar 11, 2016 at 3:27 PM, Christoph Hellwig  wrote:
> >> +static inline int
> >> +ext4_acl_chmod(struct inode *inode, umode_t mode)
> >> +{
> >> + if (IS_RICHACL(inode))
> >> + return richacl_chmod(inode, inode->i_mode);
> >> + return posix_acl_chmod(inode, inode->i_mode);
> >> +}
> >
> > Thi isn't ext4-specific and potentially duplicated in every caller.
> > Please provide this as a common helper.
> 
> This can go in neither fs.h nor posix_acl.h nor richacl.h unless we
> turn it into a macro, and I don't think we want to add a new header
> file for such extreme trivia.

I'd expect us to grow a few more of thos helper if we get the sharing
right (either a real common base object, or wrappers for anything
dealing with the acl pointers in the inode), so a new linux/acl.h
should be fine.

Re: [PATCH v18 21/22] ext4: Add richacl support

2016-03-15 Thread Christoph Hellwig 
On Mon, Mar 14, 2016 at 12:08:31AM +0100, Andreas Gruenbacher wrote:
> The xattr representation is the same on disk and at the xattr syscall
> layer, and so richacl_from_xattr is used for converting into the
> in-memory representation in both cases. The error codes are not the
> same when a user supplies an invalid value via setxattr or NFS and
> when an invalid xattr is read from disk though. I'll add a parameter
> to richacl_from_xattr to make this more explicit.

Better add a wrapper instead of a parameter.

> 
> >> +static int
> >> +__ext4_set_richacl(handle_t *handle, struct inode *inode, struct richacl 
> >> *acl)
> >> +{
> >> + const int name_index = EXT4_XATTR_INDEX_RICHACL;
> >> + umode_t mode = inode->i_mode;
> >> + int retval, size;
> >> + void *value;
> >> +
> >> + if (richacl_equiv_mode(acl, ) == 0) {
> >> + inode->i_ctime = ext4_current_time(inode);
> >> + inode->i_mode = mode;
> >> + ext4_mark_inode_dirty(handle, inode);
> >> + return __ext4_remove_richacl(handle, inode);
> >> + }
> >
> > Should this check for a NULL acl instead of special casing that
> > in ext4_set_richacl?
> 
> I'm not sure I understand what you mean. When the

ext4_set_richacl checks for a NULL acl pointer and then calls into
__ext4_remove_richacl.  I'd rather have that special casing in one
place.

Re: [PATCH v18 21/22] ext4: Add richacl support

2016-03-15 Thread Christoph Hellwig 
On Mon, Mar 14, 2016 at 12:08:31AM +0100, Andreas Gruenbacher wrote:
> The xattr representation is the same on disk and at the xattr syscall
> layer, and so richacl_from_xattr is used for converting into the
> in-memory representation in both cases. The error codes are not the
> same when a user supplies an invalid value via setxattr or NFS and
> when an invalid xattr is read from disk though. I'll add a parameter
> to richacl_from_xattr to make this more explicit.

Better add a wrapper instead of a parameter.

> 
> >> +static int
> >> +__ext4_set_richacl(handle_t *handle, struct inode *inode, struct richacl 
> >> *acl)
> >> +{
> >> + const int name_index = EXT4_XATTR_INDEX_RICHACL;
> >> + umode_t mode = inode->i_mode;
> >> + int retval, size;
> >> + void *value;
> >> +
> >> + if (richacl_equiv_mode(acl, ) == 0) {
> >> + inode->i_ctime = ext4_current_time(inode);
> >> + inode->i_mode = mode;
> >> + ext4_mark_inode_dirty(handle, inode);
> >> + return __ext4_remove_richacl(handle, inode);
> >> + }
> >
> > Should this check for a NULL acl instead of special casing that
> > in ext4_set_richacl?
> 
> I'm not sure I understand what you mean. When the

ext4_set_richacl checks for a NULL acl pointer and then calls into
__ext4_remove_richacl.  I'd rather have that special casing in one
place.

Re: [PATCH v18 21/22] ext4: Add richacl support

2016-03-14 Thread Andreas Gruenbacher 
On Fri, Mar 11, 2016 at 3:27 PM, Christoph Hellwig  wrote:
>> +static inline int
>> +ext4_acl_chmod(struct inode *inode, umode_t mode)
>> +{
>> + if (IS_RICHACL(inode))
>> + return richacl_chmod(inode, inode->i_mode);
>> + return posix_acl_chmod(inode, inode->i_mode);
>> +}
>
> Thi isn't ext4-specific and potentially duplicated in every caller.
> Please provide this as a common helper.

This can go in neither fs.h nor posix_acl.h nor richacl.h unless we
turn it into a macro, and I don't think we want to add a new header
file for such extreme trivia.

> Also while we're at it, the mode argument is ignore and the function
> always uses inode->i_mode instead.

Right, thanks.

Andreas

Re: [PATCH v18 21/22] ext4: Add richacl support

2016-03-14 Thread Andreas Gruenbacher 
On Fri, Mar 11, 2016 at 3:27 PM, Christoph Hellwig  wrote:
>> +static inline int
>> +ext4_acl_chmod(struct inode *inode, umode_t mode)
>> +{
>> + if (IS_RICHACL(inode))
>> + return richacl_chmod(inode, inode->i_mode);
>> + return posix_acl_chmod(inode, inode->i_mode);
>> +}
>
> Thi isn't ext4-specific and potentially duplicated in every caller.
> Please provide this as a common helper.

This can go in neither fs.h nor posix_acl.h nor richacl.h unless we
turn it into a macro, and I don't think we want to add a new header
file for such extreme trivia.

> Also while we're at it, the mode argument is ignore and the function
> always uses inode->i_mode instead.

Right, thanks.

Andreas

Re: [PATCH v18 21/22] ext4: Add richacl support

2016-03-13 Thread Andreas Gruenbacher 
On Fri, Mar 11, 2016 at 3:27 PM, Christoph Hellwig  wrote:
>> +static int
>> +__ext4_set_richacl(handle_t *handle, struct inode *inode, struct richacl 
>> *acl)
>> +{
>> + const int name_index = EXT4_XATTR_INDEX_RICHACL;
>> + umode_t mode = inode->i_mode;
>> + int retval, size;
>> + void *value;
>> +
>> + if (richacl_equiv_mode(acl, ) == 0) {
>> + inode->i_ctime = ext4_current_time(inode);
>> + inode->i_mode = mode;
>> + ext4_mark_inode_dirty(handle, inode);
>> + return __ext4_remove_richacl(handle, inode);
>> + }
>
> Should this check for a NULL acl instead of special casing that
> in ext4_set_richacl?

I'm not sure I understand what you mean. When iop->set_richacl is
called with a richacl that is mode-equivalent, the file permission
bits need to be updated and any existing acl needs to be removed.
Doing this at the vfs level would result in two calls, iop->setattr
and iop->set_richacl, which can cause problems. To remove an existing
acl without setting the mode, set_richacl is called with a NULL
richacl.

__ext4_set_richacl() was split into __ext4_set_richacl() and
__ext4_remove_richacl() to align with the xfs code due to the
following comment from Dave Chinner:
  http://oss.sgi.com/archives/xfs/2015-10/msg00354.html

Diff here:
  
https://git.kernel.org/cgit/linux/kernel/git/agruen/linux-richacl.git/diff/fs/ext4/richacl.c?id=richacl-2015-10-16=richacl-2015-10-12

>> +int
>> +ext4_init_richacl(handle_t *handle, struct inode *inode, struct inode *dir)
>> +{
>> + struct richacl *acl = richacl_create(>i_mode, dir);
>> + int error;
>> +
>> + error = PTR_ERR(acl);
>> + if (IS_ERR(acl))
>> + return error;
>
> if (IS_ERR(acl))
> return PTR_ERR(acl);
>
>> + if (acl) {
>> + error = __ext4_set_richacl(handle, inode, acl);
>> + richacl_put(acl);
>> + }
>
> Shouldn't richacl_create return NULL if the ACL is equivalent to the
> mode bits instead of letting every filesystem figure that out on it's
> own?

Hm, that's what it does?

Thanks,
Andreas

Re: [PATCH v18 21/22] ext4: Add richacl support

2016-03-13 Thread Andreas Gruenbacher 
On Fri, Mar 11, 2016 at 3:27 PM, Christoph Hellwig  wrote:
>> +static int
>> +__ext4_set_richacl(handle_t *handle, struct inode *inode, struct richacl 
>> *acl)
>> +{
>> + const int name_index = EXT4_XATTR_INDEX_RICHACL;
>> + umode_t mode = inode->i_mode;
>> + int retval, size;
>> + void *value;
>> +
>> + if (richacl_equiv_mode(acl, ) == 0) {
>> + inode->i_ctime = ext4_current_time(inode);
>> + inode->i_mode = mode;
>> + ext4_mark_inode_dirty(handle, inode);
>> + return __ext4_remove_richacl(handle, inode);
>> + }
>
> Should this check for a NULL acl instead of special casing that
> in ext4_set_richacl?

I'm not sure I understand what you mean. When iop->set_richacl is
called with a richacl that is mode-equivalent, the file permission
bits need to be updated and any existing acl needs to be removed.
Doing this at the vfs level would result in two calls, iop->setattr
and iop->set_richacl, which can cause problems. To remove an existing
acl without setting the mode, set_richacl is called with a NULL
richacl.

__ext4_set_richacl() was split into __ext4_set_richacl() and
__ext4_remove_richacl() to align with the xfs code due to the
following comment from Dave Chinner:
  http://oss.sgi.com/archives/xfs/2015-10/msg00354.html

Diff here:
  
https://git.kernel.org/cgit/linux/kernel/git/agruen/linux-richacl.git/diff/fs/ext4/richacl.c?id=richacl-2015-10-16=richacl-2015-10-12

>> +int
>> +ext4_init_richacl(handle_t *handle, struct inode *inode, struct inode *dir)
>> +{
>> + struct richacl *acl = richacl_create(>i_mode, dir);
>> + int error;
>> +
>> + error = PTR_ERR(acl);
>> + if (IS_ERR(acl))
>> + return error;
>
> if (IS_ERR(acl))
> return PTR_ERR(acl);
>
>> + if (acl) {
>> + error = __ext4_set_richacl(handle, inode, acl);
>> + richacl_put(acl);
>> + }
>
> Shouldn't richacl_create return NULL if the ACL is equivalent to the
> mode bits instead of letting every filesystem figure that out on it's
> own?

Hm, that's what it does?

Thanks,
Andreas

Re: [PATCH v18 21/22] ext4: Add richacl support

2016-03-13 Thread Andreas Gruenbacher 
On Fri, Mar 11, 2016 at 3:27 PM, Christoph Hellwig  wrote:
>> +static inline int
>> +ext4_acl_chmod(struct inode *inode, umode_t mode)
>> +{
>> + if (IS_RICHACL(inode))
>> + return richacl_chmod(inode, inode->i_mode);
>> + return posix_acl_chmod(inode, inode->i_mode);
>> +}
>
> Thi isn't ext4-specific and potentially duplicated in every caller.
> Please provide this as a common helper.
>
> Also while we're at it, the mode argument is ignore and the function
> always uses inode->i_mode instead.
>
>> +ext4_get_richacl(struct inode *inode)
>> +{
>> + const int name_index = EXT4_XATTR_INDEX_RICHACL;
>> + void *value = NULL;
>> + struct richacl *acl = NULL;
>> + int retval;
>> +
>> + retval = ext4_xattr_get(inode, name_index, "", NULL, 0);
>> + if (retval > 0) {
>> + value = kmalloc(retval, GFP_NOFS);
>> + if (!value)
>> + return ERR_PTR(-ENOMEM);
>> + retval = ext4_xattr_get(inode, name_index, "", value, retval);
>> + }
>> + if (retval > 0) {
>> + acl = richacl_from_xattr(_user_ns, value, retval);
>> + if (acl == ERR_PTR(-EINVAL))
>> + acl = ERR_PTR(-EIO);
>
> Shouldn't richacl_from_xattr return the error pointer that ->get_richacl
> callers expect?

The xattr representation is the same on disk and at the xattr syscall
layer, and so richacl_from_xattr is used for converting into the
in-memory representation in both cases. The error codes are not the
same when a user supplies an invalid value via setxattr or NFS and
when an invalid xattr is read from disk though. I'll add a parameter
to richacl_from_xattr to make this more explicit.

>> +static int
>> +__ext4_set_richacl(handle_t *handle, struct inode *inode, struct richacl 
>> *acl)
>> +{
>> + const int name_index = EXT4_XATTR_INDEX_RICHACL;
>> + umode_t mode = inode->i_mode;
>> + int retval, size;
>> + void *value;
>> +
>> + if (richacl_equiv_mode(acl, ) == 0) {
>> + inode->i_ctime = ext4_current_time(inode);
>> + inode->i_mode = mode;
>> + ext4_mark_inode_dirty(handle, inode);
>> + return __ext4_remove_richacl(handle, inode);
>> + }
>
> Should this check for a NULL acl instead of special casing that
> in ext4_set_richacl?

I'm not sure I understand what you mean. When the

>> +int
>> +ext4_init_richacl(handle_t *handle, struct inode *inode, struct inode *dir)
>> +{
>> + struct richacl *acl = richacl_create(>i_mode, dir);
>> + int error;
>> +
>> + error = PTR_ERR(acl);
>> + if (IS_ERR(acl))
>> + return error;
>
> if (IS_ERR(acl))
> return PTR_ERR(acl);
>
>> + if (acl) {
>> + error = __ext4_set_richacl(handle, inode, acl);
>> + richacl_put(acl);
>> + }
>
> Shouldn't richacl_create return NULL if the ACL is equivalent to the
> mode bits instead of letting every filesystem figure that out on it's
> own?
>

Re: [PATCH v18 21/22] ext4: Add richacl support

2016-03-13 Thread Andreas Gruenbacher 
On Fri, Mar 11, 2016 at 3:27 PM, Christoph Hellwig  wrote:
>> +static inline int
>> +ext4_acl_chmod(struct inode *inode, umode_t mode)
>> +{
>> + if (IS_RICHACL(inode))
>> + return richacl_chmod(inode, inode->i_mode);
>> + return posix_acl_chmod(inode, inode->i_mode);
>> +}
>
> Thi isn't ext4-specific and potentially duplicated in every caller.
> Please provide this as a common helper.
>
> Also while we're at it, the mode argument is ignore and the function
> always uses inode->i_mode instead.
>
>> +ext4_get_richacl(struct inode *inode)
>> +{
>> + const int name_index = EXT4_XATTR_INDEX_RICHACL;
>> + void *value = NULL;
>> + struct richacl *acl = NULL;
>> + int retval;
>> +
>> + retval = ext4_xattr_get(inode, name_index, "", NULL, 0);
>> + if (retval > 0) {
>> + value = kmalloc(retval, GFP_NOFS);
>> + if (!value)
>> + return ERR_PTR(-ENOMEM);
>> + retval = ext4_xattr_get(inode, name_index, "", value, retval);
>> + }
>> + if (retval > 0) {
>> + acl = richacl_from_xattr(_user_ns, value, retval);
>> + if (acl == ERR_PTR(-EINVAL))
>> + acl = ERR_PTR(-EIO);
>
> Shouldn't richacl_from_xattr return the error pointer that ->get_richacl
> callers expect?

The xattr representation is the same on disk and at the xattr syscall
layer, and so richacl_from_xattr is used for converting into the
in-memory representation in both cases. The error codes are not the
same when a user supplies an invalid value via setxattr or NFS and
when an invalid xattr is read from disk though. I'll add a parameter
to richacl_from_xattr to make this more explicit.

>> +static int
>> +__ext4_set_richacl(handle_t *handle, struct inode *inode, struct richacl 
>> *acl)
>> +{
>> + const int name_index = EXT4_XATTR_INDEX_RICHACL;
>> + umode_t mode = inode->i_mode;
>> + int retval, size;
>> + void *value;
>> +
>> + if (richacl_equiv_mode(acl, ) == 0) {
>> + inode->i_ctime = ext4_current_time(inode);
>> + inode->i_mode = mode;
>> + ext4_mark_inode_dirty(handle, inode);
>> + return __ext4_remove_richacl(handle, inode);
>> + }
>
> Should this check for a NULL acl instead of special casing that
> in ext4_set_richacl?

I'm not sure I understand what you mean. When the

>> +int
>> +ext4_init_richacl(handle_t *handle, struct inode *inode, struct inode *dir)
>> +{
>> + struct richacl *acl = richacl_create(>i_mode, dir);
>> + int error;
>> +
>> + error = PTR_ERR(acl);
>> + if (IS_ERR(acl))
>> + return error;
>
> if (IS_ERR(acl))
> return PTR_ERR(acl);
>
>> + if (acl) {
>> + error = __ext4_set_richacl(handle, inode, acl);
>> + richacl_put(acl);
>> + }
>
> Shouldn't richacl_create return NULL if the ACL is equivalent to the
> mode bits instead of letting every filesystem figure that out on it's
> own?
>

Re: [PATCH v18 21/22] ext4: Add richacl support

2016-03-11 Thread Christoph Hellwig 
> +static inline int
> +ext4_acl_chmod(struct inode *inode, umode_t mode)
> +{
> + if (IS_RICHACL(inode))
> + return richacl_chmod(inode, inode->i_mode);
> + return posix_acl_chmod(inode, inode->i_mode);
> +}

Thi isn't ext4-specific and potentially duplicated in every caller.
Please provide this as a common helper.

Also while we're at it, the mode argument is ignore and the function
always uses inode->i_mode instead.

> +ext4_get_richacl(struct inode *inode)
> +{
> + const int name_index = EXT4_XATTR_INDEX_RICHACL;
> + void *value = NULL;
> + struct richacl *acl = NULL;
> + int retval;
> +
> + retval = ext4_xattr_get(inode, name_index, "", NULL, 0);
> + if (retval > 0) {
> + value = kmalloc(retval, GFP_NOFS);
> + if (!value)
> + return ERR_PTR(-ENOMEM);
> + retval = ext4_xattr_get(inode, name_index, "", value, retval);
> + }
> + if (retval > 0) {
> + acl = richacl_from_xattr(_user_ns, value, retval);
> + if (acl == ERR_PTR(-EINVAL))
> + acl = ERR_PTR(-EIO);

Shouldn't richacl_from_xattr return the error pointer that ->get_richacl
callers expect?

> +static int
> +__ext4_set_richacl(handle_t *handle, struct inode *inode, struct richacl 
> *acl)
> +{
> + const int name_index = EXT4_XATTR_INDEX_RICHACL;
> + umode_t mode = inode->i_mode;
> + int retval, size;
> + void *value;
> +
> + if (richacl_equiv_mode(acl, ) == 0) {
> + inode->i_ctime = ext4_current_time(inode);
> + inode->i_mode = mode;
> + ext4_mark_inode_dirty(handle, inode);
> + return __ext4_remove_richacl(handle, inode);
> + }

Should this check for a NULL acl instead of special casing that
in ext4_set_richacl?

> +int
> +ext4_init_richacl(handle_t *handle, struct inode *inode, struct inode *dir)
> +{
> + struct richacl *acl = richacl_create(>i_mode, dir);
> + int error;
> +
> + error = PTR_ERR(acl);
> + if (IS_ERR(acl))
> + return error;

if (IS_ERR(acl))
return PTR_ERR(acl);

> + if (acl) {
> + error = __ext4_set_richacl(handle, inode, acl);
> + richacl_put(acl);
> + }

Shouldn't richacl_create return NULL if the ACL is equivalent to the
mode bits instead of letting every filesystem figure that out on it's
own?

Re: [PATCH v18 21/22] ext4: Add richacl support

2016-03-11 Thread Christoph Hellwig 
> +static inline int
> +ext4_acl_chmod(struct inode *inode, umode_t mode)
> +{
> + if (IS_RICHACL(inode))
> + return richacl_chmod(inode, inode->i_mode);
> + return posix_acl_chmod(inode, inode->i_mode);
> +}

Thi isn't ext4-specific and potentially duplicated in every caller.
Please provide this as a common helper.

Also while we're at it, the mode argument is ignore and the function
always uses inode->i_mode instead.

> +ext4_get_richacl(struct inode *inode)
> +{
> + const int name_index = EXT4_XATTR_INDEX_RICHACL;
> + void *value = NULL;
> + struct richacl *acl = NULL;
> + int retval;
> +
> + retval = ext4_xattr_get(inode, name_index, "", NULL, 0);
> + if (retval > 0) {
> + value = kmalloc(retval, GFP_NOFS);
> + if (!value)
> + return ERR_PTR(-ENOMEM);
> + retval = ext4_xattr_get(inode, name_index, "", value, retval);
> + }
> + if (retval > 0) {
> + acl = richacl_from_xattr(_user_ns, value, retval);
> + if (acl == ERR_PTR(-EINVAL))
> + acl = ERR_PTR(-EIO);

Shouldn't richacl_from_xattr return the error pointer that ->get_richacl
callers expect?

> +static int
> +__ext4_set_richacl(handle_t *handle, struct inode *inode, struct richacl 
> *acl)
> +{
> + const int name_index = EXT4_XATTR_INDEX_RICHACL;
> + umode_t mode = inode->i_mode;
> + int retval, size;
> + void *value;
> +
> + if (richacl_equiv_mode(acl, ) == 0) {
> + inode->i_ctime = ext4_current_time(inode);
> + inode->i_mode = mode;
> + ext4_mark_inode_dirty(handle, inode);
> + return __ext4_remove_richacl(handle, inode);
> + }

Should this check for a NULL acl instead of special casing that
in ext4_set_richacl?

> +int
> +ext4_init_richacl(handle_t *handle, struct inode *inode, struct inode *dir)
> +{
> + struct richacl *acl = richacl_create(>i_mode, dir);
> + int error;
> +
> + error = PTR_ERR(acl);
> + if (IS_ERR(acl))
> + return error;

if (IS_ERR(acl))
return PTR_ERR(acl);

> + if (acl) {
> + error = __ext4_set_richacl(handle, inode, acl);
> + richacl_put(acl);
> + }

Shouldn't richacl_create return NULL if the ACL is equivalent to the
mode bits instead of letting every filesystem figure that out on it's
own?

[PATCH v18 21/22] ext4: Add richacl support

2016-02-29 Thread Andreas Gruenbacher 
From: "Aneesh Kumar K.V" 

Support the richacl permission model in ext4.  The richacls are stored
in "system.richacl" xattrs.  Richacls need to be enabled by tune2fs or
at file system create time.

Signed-off-by: Aneesh Kumar K.V 
Signed-off-by: Andreas Gruenbacher 
Reviewed-by: Andreas Dilger 
---
 fs/ext4/Kconfig   |  11 +
 fs/ext4/Makefile  |   1 +
 fs/ext4/file.c|   3 ++
 fs/ext4/ialloc.c  |  11 -
 fs/ext4/inode.c   |  12 -
 fs/ext4/namei.c   |   5 ++
 fs/ext4/richacl.c | 142 ++
 fs/ext4/richacl.h |  40 +++
 fs/ext4/xattr.c   |   7 +++
 9 files changed, 229 insertions(+), 3 deletions(-)
 create mode 100644 fs/ext4/richacl.c
 create mode 100644 fs/ext4/richacl.h

diff --git a/fs/ext4/Kconfig b/fs/ext4/Kconfig
index b46e9fc..4e21c18 100644
--- a/fs/ext4/Kconfig
+++ b/fs/ext4/Kconfig
@@ -22,6 +22,17 @@ config EXT3_FS_POSIX_ACL
  This config option is here only for backward compatibility. ext3
  filesystem is now handled by the ext4 driver.
 
+config EXT4_FS_RICHACL
+   bool "Ext4 Rich Access Control Lists"
+   depends on EXT4_FS
+   select FS_RICHACL
+   help
+ Richacls are an implementation of NFSv4 ACLs, extended by file masks
+ to cleanly integrate into the POSIX file permission model.  To learn
+ more about them, see http://www.bestbits.at/richacl/.
+
+ If you don't know what Richacls are, say N.
+
 config EXT3_FS_SECURITY
bool "Ext3 Security Labels"
depends on EXT3_FS
diff --git a/fs/ext4/Makefile b/fs/ext4/Makefile
index f52cf54..1fb7f11 100644
--- a/fs/ext4/Makefile
+++ b/fs/ext4/Makefile
@@ -14,3 +14,4 @@ ext4-$(CONFIG_EXT4_FS_POSIX_ACL)  += acl.o
 ext4-$(CONFIG_EXT4_FS_SECURITY)+= xattr_security.o
 ext4-$(CONFIG_EXT4_FS_ENCRYPTION)  += crypto_policy.o crypto.o \
crypto_key.o crypto_fname.o
+ext4-$(CONFIG_EXT4_FS_RICHACL) += richacl.o
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 474f1a4..30bfc50 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -30,6 +30,7 @@
 #include "ext4_jbd2.h"
 #include "xattr.h"
 #include "acl.h"
+#include "richacl.h"
 
 /*
  * Called when an inode is released. Note that this is different
@@ -764,6 +765,8 @@ const struct inode_operations ext4_file_inode_operations = {
.removexattr= generic_removexattr,
.get_acl= ext4_get_acl,
.set_acl= ext4_set_acl,
+   .get_richacl= ext4_get_richacl,
+   .set_richacl= ext4_set_richacl,
.fiemap = ext4_fiemap,
 };
 
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index acc0ad5..f2d31c2 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -27,6 +27,7 @@
 #include "ext4_jbd2.h"
 #include "xattr.h"
 #include "acl.h"
+#include "richacl.h"
 
 #include 
 
@@ -729,6 +730,14 @@ out:
return ret;
 }
 
+static inline int
+ext4_new_acl(handle_t *handle, struct inode *inode, struct inode *dir)
+{
+   if (IS_RICHACL(dir))
+   return ext4_init_richacl(handle, inode, dir);
+   return ext4_init_acl(handle, inode, dir);
+}
+
 /*
  * There are two policies for allocating an inode.  If the new inode is
  * a directory, then a forward search is made for a block group with both
@@ -1093,7 +1102,7 @@ got:
if (err)
goto fail_drop;
 
-   err = ext4_init_acl(handle, inode, dir);
+   err = ext4_new_acl(handle, inode, dir);
if (err)
goto fail_free_drop;
 
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 9cc57c3..ef6fb86 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -42,6 +42,7 @@
 #include "xattr.h"
 #include "acl.h"
 #include "truncate.h"
+#include "richacl.h"
 
 #include 
 
@@ -4851,6 +4852,14 @@ static void ext4_wait_for_tail_page_commit(struct inode 
*inode)
}
 }
 
+static inline int
+ext4_acl_chmod(struct inode *inode, umode_t mode)
+{
+   if (IS_RICHACL(inode))
+   return richacl_chmod(inode, inode->i_mode);
+   return posix_acl_chmod(inode, inode->i_mode);
+}
+
 /*
  * ext4_setattr()
  *
@@ -5021,8 +5030,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr 
*attr)
ext4_orphan_del(NULL, inode);
 
if (!rc && (ia_valid & ATTR_MODE))
-   rc = posix_acl_chmod(inode, inode->i_mode);
-
+   rc = ext4_acl_chmod(inode, inode->i_mode);
 err_out:
ext4_std_error(inode->i_sb, error);
if (!error)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 48e4b89..d86c5f2 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -38,6 +38,7 @@
 
 #include "xattr.h"
 #include "acl.h"
+#include "richacl.h"
 
 #include 
 /*
@@ -3888,6 +3889,8 @@ const struct inode_operations ext4_dir_inode_operations = 
{
.removexattr= generic_removexattr,
.get_acl= ext4_get_acl,

[PATCH v18 21/22] ext4: Add richacl support

2016-02-29 Thread Andreas Gruenbacher 
From: "Aneesh Kumar K.V" 

Support the richacl permission model in ext4.  The richacls are stored
in "system.richacl" xattrs.  Richacls need to be enabled by tune2fs or
at file system create time.

Signed-off-by: Aneesh Kumar K.V 
Signed-off-by: Andreas Gruenbacher 
Reviewed-by: Andreas Dilger 
---
 fs/ext4/Kconfig   |  11 +
 fs/ext4/Makefile  |   1 +
 fs/ext4/file.c|   3 ++
 fs/ext4/ialloc.c  |  11 -
 fs/ext4/inode.c   |  12 -
 fs/ext4/namei.c   |   5 ++
 fs/ext4/richacl.c | 142 ++
 fs/ext4/richacl.h |  40 +++
 fs/ext4/xattr.c   |   7 +++
 9 files changed, 229 insertions(+), 3 deletions(-)
 create mode 100644 fs/ext4/richacl.c
 create mode 100644 fs/ext4/richacl.h

diff --git a/fs/ext4/Kconfig b/fs/ext4/Kconfig
index b46e9fc..4e21c18 100644
--- a/fs/ext4/Kconfig
+++ b/fs/ext4/Kconfig
@@ -22,6 +22,17 @@ config EXT3_FS_POSIX_ACL
  This config option is here only for backward compatibility. ext3
  filesystem is now handled by the ext4 driver.
 
+config EXT4_FS_RICHACL
+   bool "Ext4 Rich Access Control Lists"
+   depends on EXT4_FS
+   select FS_RICHACL
+   help
+ Richacls are an implementation of NFSv4 ACLs, extended by file masks
+ to cleanly integrate into the POSIX file permission model.  To learn
+ more about them, see http://www.bestbits.at/richacl/.
+
+ If you don't know what Richacls are, say N.
+
 config EXT3_FS_SECURITY
bool "Ext3 Security Labels"
depends on EXT3_FS
diff --git a/fs/ext4/Makefile b/fs/ext4/Makefile
index f52cf54..1fb7f11 100644
--- a/fs/ext4/Makefile
+++ b/fs/ext4/Makefile
@@ -14,3 +14,4 @@ ext4-$(CONFIG_EXT4_FS_POSIX_ACL)  += acl.o
 ext4-$(CONFIG_EXT4_FS_SECURITY)+= xattr_security.o
 ext4-$(CONFIG_EXT4_FS_ENCRYPTION)  += crypto_policy.o crypto.o \
crypto_key.o crypto_fname.o
+ext4-$(CONFIG_EXT4_FS_RICHACL) += richacl.o
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 474f1a4..30bfc50 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -30,6 +30,7 @@
 #include "ext4_jbd2.h"
 #include "xattr.h"
 #include "acl.h"
+#include "richacl.h"
 
 /*
  * Called when an inode is released. Note that this is different
@@ -764,6 +765,8 @@ const struct inode_operations ext4_file_inode_operations = {
.removexattr= generic_removexattr,
.get_acl= ext4_get_acl,
.set_acl= ext4_set_acl,
+   .get_richacl= ext4_get_richacl,
+   .set_richacl= ext4_set_richacl,
.fiemap = ext4_fiemap,
 };
 
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index acc0ad5..f2d31c2 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -27,6 +27,7 @@
 #include "ext4_jbd2.h"
 #include "xattr.h"
 #include "acl.h"
+#include "richacl.h"
 
 #include 
 
@@ -729,6 +730,14 @@ out:
return ret;
 }
 
+static inline int
+ext4_new_acl(handle_t *handle, struct inode *inode, struct inode *dir)
+{
+   if (IS_RICHACL(dir))
+   return ext4_init_richacl(handle, inode, dir);
+   return ext4_init_acl(handle, inode, dir);
+}
+
 /*
  * There are two policies for allocating an inode.  If the new inode is
  * a directory, then a forward search is made for a block group with both
@@ -1093,7 +1102,7 @@ got:
if (err)
goto fail_drop;
 
-   err = ext4_init_acl(handle, inode, dir);
+   err = ext4_new_acl(handle, inode, dir);
if (err)
goto fail_free_drop;
 
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 9cc57c3..ef6fb86 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -42,6 +42,7 @@
 #include "xattr.h"
 #include "acl.h"
 #include "truncate.h"
+#include "richacl.h"
 
 #include 
 
@@ -4851,6 +4852,14 @@ static void ext4_wait_for_tail_page_commit(struct inode 
*inode)
}
 }
 
+static inline int
+ext4_acl_chmod(struct inode *inode, umode_t mode)
+{
+   if (IS_RICHACL(inode))
+   return richacl_chmod(inode, inode->i_mode);
+   return posix_acl_chmod(inode, inode->i_mode);
+}
+
 /*
  * ext4_setattr()
  *
@@ -5021,8 +5030,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr 
*attr)
ext4_orphan_del(NULL, inode);
 
if (!rc && (ia_valid & ATTR_MODE))
-   rc = posix_acl_chmod(inode, inode->i_mode);
-
+   rc = ext4_acl_chmod(inode, inode->i_mode);
 err_out:
ext4_std_error(inode->i_sb, error);
if (!error)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 48e4b89..d86c5f2 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -38,6 +38,7 @@
 
 #include "xattr.h"
 #include "acl.h"
+#include "richacl.h"
 
 #include 
 /*
@@ -3888,6 +3889,8 @@ const struct inode_operations ext4_dir_inode_operations = 
{
.removexattr= generic_removexattr,
.get_acl= ext4_get_acl,
.set_acl= ext4_set_acl,
+   .get_richacl= ext4_get_richacl,
+   .set_richacl=