Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1355945 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v18 21/22] ext4: Add richacl support Christoph Hellwig <hch@infradead.org> - 2016-03-11 15:30 +0100
    Re: [PATCH v18 21/22] ext4: Add richacl support Andreas Gruenbacher <agruenba@redhat.com> - 2016-03-14 00:10 +0100
      Re: [PATCH v18 21/22] ext4: Add richacl support Christoph Hellwig <hch@infradead.org> - 2016-03-15 08:20 +0100
        Re: [PATCH v18 21/22] ext4: Add richacl support Andreas Gruenbacher <agruenba@redhat.com> - 2016-03-16 23:40 +0100
    Re: [PATCH v18 21/22] ext4: Add richacl support Andreas Gruenbacher <agruenba@redhat.com> - 2016-03-14 01:00 +0100
    Re: [PATCH v18 21/22] ext4: Add richacl support Andreas Gruenbacher <agruenba@redhat.com> - 2016-03-14 14:10 +0100
      Re: [PATCH v18 21/22] ext4: Add richacl support Christoph Hellwig <hch@infradead.org> - 2016-03-15 08:30 +0100

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

> +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(&init_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, &mode) == 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(&inode->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?

[toc] | [next] | [standalone]

#1356769

On Fri, Mar 11, 2016 at 3:27 PM, Christoph Hellwig <hch@infradead.org> 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(&init_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, &mode) == 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(&inode->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?
>

[toc] | [prev] | [next] | [standalone]

#1357840

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, &mode) == 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.

[toc] | [prev] | [next] | [standalone]

#1359426

On Tue, Mar 15, 2016 at 8:17 AM, Christoph Hellwig <hch@infradead.org> 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, &mode) == 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

[toc] | [prev] | [next] | [standalone]

#1356777

On Fri, Mar 11, 2016 at 3:27 PM, Christoph Hellwig <hch@infradead.org> 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, &mode) == 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&id2=richacl-2015-10-12

>> +int
>> +ext4_init_richacl(handle_t *handle, struct inode *inode, struct inode *dir)
>> +{
>> +     struct richacl *acl = richacl_create(&inode->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

[toc] | [prev] | [next] | [standalone]

#1357228

On Fri, Mar 11, 2016 at 3:27 PM, Christoph Hellwig <hch@infradead.org> 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

[toc] | [prev] | [next] | [standalone]

#1357849

On Mon, Mar 14, 2016 at 02:02:33PM +0100, Andreas Gruenbacher wrote:
> On Fri, Mar 11, 2016 at 3:27 PM, Christoph Hellwig <hch@infradead.org> 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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web