[PATCH v18 00/22] Richacls (Core and Ext4)

classic Classic list List threaded Threaded
59 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

Andreas Gruenbacher-6
On Fri, Mar 11, 2016 at 3:27 PM, Christoph Hellwig <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v18 00/22] Richacls (Core and Ext4)

Jeremy Allison
In reply to this post by Andreas Gruenbacher-6
On Mon, Mar 14, 2016 at 12:02:13AM +0100, Andreas Gruenbacher wrote:

> On Sat, Mar 12, 2016 at 12:02 AM, Jeremy Allison <[hidden email]> wrote:
> > On Fri, Mar 11, 2016 at 02:05:16PM -0600, Steve French wrote:
> >> Sounds like I need to quickly rework the SMB3 ACL helper functions
> >> for cifs.ko
> >>
> >> Also do you know where is the current version of the corresponding
> >> vfs_richacl for
> >> Samba which works with the current RichACL format?
> >
> > I have a patch for a new vfs_richacl somewhere. I remember
> > sending it to Andreas for testing...
>
> Ah, the patch was for testing, not resting ... how could I get that mixed up.

:-).

> I've applied your patch to the latest master branch, made it compile
> again, and fixed a few obvious problems. The results I get with
> smbcacls look reasonable now.
>
> The code is here:
>   https://github.com/andreas-gruenbacher/samba richacl
>
> I've used the following smb.conf:
>   [richacl]
>   comment = Richacl directory
>   path = /mnt/ext4
>   vfs objects = richacl
>   writeable = yes
>   browseable = yes

Great ! Once richacls gets into the kernel I'll submit
this into the Samba master branch.

> Is there a particular reason why you didn't make vfs_richacl a
> dynamically loadable module?

Probably sheer lazyness :-).
Reply | Threaded
Open this post in threaded view
|

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

Andreas Gruenbacher-6
In reply to this post by Christoph Hellwig
On Fri, Mar 11, 2016 at 3:27 PM, Christoph Hellwig <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v18 00/22] Richacls (Core and Ext4)

Christoph Hellwig
In reply to this post by J. Bruce Fields
On Fri, Mar 11, 2016 at 09:07:57AM -0500, J. Bruce Fields wrote:
> Could you explain what you mean by "adding allow and deny ACE at the
> same time"?

NFSv4/rich ACLs have both ALLOW and DENY ACE, which is contrary to
the model how we've operated since the dawn of time.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v18 19/22] richacl: Add richacl xattr handler

Christoph Hellwig
In reply to this post by J. Bruce Fields
On Fri, Mar 11, 2016 at 09:19:05AM -0500, J. Bruce Fields wrote:
> On Fri, Mar 11, 2016 at 06:17:35AM -0800, Christoph Hellwig wrote:
> > On Mon, Feb 29, 2016 at 09:17:24AM +0100, Andreas Gruenbacher wrote:
> > > Add richacl xattr handler implementing the xattr operations based on the
> > > get_richacl and set_richacl inode operations.
> >
> > Given all the issues with Posix ACLs and selinux attributes these really
> > should be proper syscalls instead of abusing the xattr interface.
>
> What are those problems exactly?

That people get confused between the attr used by the xattr syscall
interface and the attr used to store things on disk or the protocol.
This has happened every time we have non-native support, e.g. XFS, NFS,
CIFS, ntfs, etc.  And it's only going to become worse.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v18 00/22] Richacls (Core and Ext4)

Christoph Hellwig
In reply to this post by Andreas Gruenbacher-6
On Fri, Mar 11, 2016 at 05:11:51PM +0100, Andreas Gruenbacher wrote:
> > while breaking a lot of assumptions,
>
> The model is designed specifically to be compliant with the POSIX
> permission model. What assumptions are you talking about?

People have long learned that we only have 'alloc' permissions.  Any
model that mixes allow and deny ACE is a mistake.

> > especially by adding allow and deny ACE at the same time.
>
> I remember from past discussions that a permission model like the
> POSIX ACL model that doesn't have DENY ACEs would be more to your
> liking. This argument is dead from the start though: NFSv4 ACLs
> without DENY ACEs cannot represent basic file permissions like 0604
> where the owning group has fewer permissions than others, for example
> (see the richaclex(7) man page). We would end up with a permission
> model that isn't even compatible with the traditional POSIX file
> permission model, one which nobody else implements or cares about.

So let's stick to the model that we already have.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v18 11/22] vfs: Cache base_acl objects in inodes

Christoph Hellwig
In reply to this post by Andreas Gruenbacher-6
On Fri, Mar 11, 2016 at 05:24:45PM +0100, Andreas Gruenbacher wrote:
> POSIX ACLs and RichACLs are different objects, with different members
> and different algorithms operating on them. The only commonality is
> that they are both kmalloc()ed, reference counted objects, and when an
> inode is destroyed, both kinds of ACLs can be put in the same way,
> avoiding an unnecessary if. What kind of common-code container beyond
> that are you still dreaming about?

We still have a main object that is simply a list of ACEs.  But if that
doesn't work out (I suspect it should) I don't think the common base
object is a good idea.  It just leads to a lot of crazy container_of
calls.  If the common object abstraction doesn't work out we'll need
a procedural one instead that has common acl_* calls that decide what
do to based on the file system acl flag.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v18 00/22] Richacls (Core and Ext4)

Christoph Hellwig
In reply to this post by Steve French-2
On Fri, Mar 11, 2016 at 02:05:16PM -0600, Steve French wrote:
> A loosely related question is what can be done for tools around existing
> interfaces for ACLs.  I recently found out NTFS-3g has this xattr:
>
>     static const char nf_ns_xattr_ntfs_acl[] = "system.ntfs_acl";
>
> which allows you to query system.ntfs_acl xattr to get their full ACL

Bah.  Filesystems really have no business exposing random system xattrs,
and we really need to add a filter to fs/xattr.c to not expose
arbitrary attrs ouside the user.* prefix.
Reply | Threaded
Open this post in threaded view
|

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

Christoph Hellwig
In reply to this post by Andreas Gruenbacher-6
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.
Reply | Threaded
Open this post in threaded view
|

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

Christoph Hellwig
In reply to this post by Andreas Gruenbacher-6
On Mon, Mar 14, 2016 at 02:02:33PM +0100, Andreas Gruenbacher wrote:

> On Fri, Mar 11, 2016 at 3:27 PM, Christoph Hellwig <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v18 00/22] Richacls (Core and Ext4)

Jeremy Allison
In reply to this post by Christoph Hellwig
On Tue, Mar 15, 2016 at 12:11:03AM -0700, Christoph Hellwig wrote:
> On Fri, Mar 11, 2016 at 05:11:51PM +0100, Andreas Gruenbacher wrote:
> > > while breaking a lot of assumptions,
> >
> > The model is designed specifically to be compliant with the POSIX
> > permission model. What assumptions are you talking about?
>
> People have long learned that we only have 'alloc' permissions.  Any
> model that mixes allow and deny ACE is a mistake.

People can also learn and change though :-). One of the
biggest complaints people deploying Samba on Linux have is the
incompatible ACL models.

Whilst I have sympathy with your intense dislike of the
Windows ACL model, this comes down to the core of "who
do we serve ?" IMHO we should serve the users (although
I must confess I'd look awful in a TRON suit :-).
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v18 00/22] Richacls (Core and Ext4)

Volker Lendecke
On Tue, Mar 15, 2016 at 08:45:14AM -0700, Jeremy Allison wrote:
> On Tue, Mar 15, 2016 at 12:11:03AM -0700, Christoph Hellwig wrote:
> > People have long learned that we only have 'alloc' permissions.  Any
> > model that mixes allow and deny ACE is a mistake.
>
> People can also learn and change though :-). One of the
> biggest complaints people deploying Samba on Linux have is the
> incompatible ACL models.

Just to confirm: I see this a lot in the field. NFSv4 ACLs, while not a
perfect match for NTFS ACLs are a lot closer much more usable to people
who want to serve Windows clients.

Also in the pure linux world there is a lot that you can not express
with just rwx, sgid, sticky bits and friends. If you want the additional
functionality of the richacl bits, I would call it a big mistake to
omit negative aces, if just for the reason not to create yet another
ACLs flavor.

> Whilst I have sympathy with your intense dislike of the
> Windows ACL model, this comes down to the core of "who
> do we serve ?"

The world has enough confusion around ACL semanics, please do not add
more to it by creating your own model of the day.

Volker
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v18 19/22] richacl: Add richacl xattr handler

J. Bruce Fields
In reply to this post by Christoph Hellwig
On Tue, Mar 15, 2016 at 12:10:14AM -0700, Christoph Hellwig wrote:

> On Fri, Mar 11, 2016 at 09:19:05AM -0500, J. Bruce Fields wrote:
> > On Fri, Mar 11, 2016 at 06:17:35AM -0800, Christoph Hellwig wrote:
> > > On Mon, Feb 29, 2016 at 09:17:24AM +0100, Andreas Gruenbacher wrote:
> > > > Add richacl xattr handler implementing the xattr operations based on the
> > > > get_richacl and set_richacl inode operations.
> > >
> > > Given all the issues with Posix ACLs and selinux attributes these really
> > > should be proper syscalls instead of abusing the xattr interface.
> >
> > What are those problems exactly?
>
> That people get confused between the attr used by the xattr syscall
> interface and the attr used to store things on disk or the protocol.
> This has happened every time we have non-native support, e.g. XFS, NFS,
> CIFS, ntfs, etc.  And it's only going to become worse.

How has that confusion caused problems in practice?

--b.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v18 00/22] Richacls (Core and Ext4)

Steve French-2
In reply to this post by Volker Lendecke
On Tue, Mar 15, 2016 at 3:17 PM, Volker Lendecke
<[hidden email]> wrote:

> On Tue, Mar 15, 2016 at 08:45:14AM -0700, Jeremy Allison wrote:
>> On Tue, Mar 15, 2016 at 12:11:03AM -0700, Christoph Hellwig wrote:
>> > People have long learned that we only have 'alloc' permissions.  Any
>> > model that mixes allow and deny ACE is a mistake.
>>
>> People can also learn and change though :-). One of the
>> biggest complaints people deploying Samba on Linux have is the
>> incompatible ACL models.
>
> Just to confirm: I see this a lot in the field. NFSv4 ACLs, while not a
> perfect match for NTFS ACLs are a lot closer much more usable to people
> who want to serve Windows clients.

Yes (and presumably Macs as well)



--
Thanks,

Steve
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v18 00/22] Richacls (Core and Ext4)

Steve French-2
In reply to this post by Christoph Hellwig
On Tue, Mar 15, 2016 at 2:14 AM, Christoph Hellwig <[hidden email]> wrote:

> On Fri, Mar 11, 2016 at 02:05:16PM -0600, Steve French wrote:
>> A loosely related question is what can be done for tools around existing
>> interfaces for ACLs.  I recently found out NTFS-3g has this xattr:
>>
>>     static const char nf_ns_xattr_ntfs_acl[] = "system.ntfs_acl";
>>
>> which allows you to query system.ntfs_acl xattr to get their full ACL
>
> Bah.  Filesystems really have no business exposing random system xattrs,
> and we really need to add a filter to fs/xattr.c to not expose
> arbitrary attrs ouside the user.* prefix.

Hopefully we don't consider them random system xattrs, it is
plausible that ntfs uses these for user space tools that I don't
have.

At least for cifs.ko a similar subset (querying ACLs, streams and
reparse info e.g.)
to the ntfs set would be very helpful.  For example,
Being able to query the actual ACL over the wire, is important for backup
and for debug, the only question is whether to do it via an xattr (possibly
being able to have some synergy with existing ntfs-3g tools) or an ioctl
(since adding an NTFS specific syscall for a couple fs doesn't make sense).


For the specific example of the odd ntfs.streams.list xattr, I can see why
they have it.  I would have mixed feelings about having no way to tell
streams and EAs from each other
since NTFS-3g displaying streams as xattrs and also Extended
Attributes (EAs) as xattrs
(and if they didn't have an additional xattr to list streams)
without a way to tell the difference (at least a system xattr to list
the alternate
data streams is useful).   There is useful information in alternate data streams
that backup (and debugging) programs need for some workloads,
for example the origin (where internet explorer downloaded a file from)
and file classification information.

--
Thanks,

Steve
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v18 11/22] vfs: Cache base_acl objects in inodes

Andreas Gruenbacher-6
In reply to this post by Christoph Hellwig
On Tue, Mar 15, 2016 at 8:12 AM, Christoph Hellwig <[hidden email]> wrote:

> On Fri, Mar 11, 2016 at 05:24:45PM +0100, Andreas Gruenbacher wrote:
>> POSIX ACLs and RichACLs are different objects, with different members
>> and different algorithms operating on them. The only commonality is
>> that they are both kmalloc()ed, reference counted objects, and when an
>> inode is destroyed, both kinds of ACLs can be put in the same way,
>> avoiding an unnecessary if. What kind of common-code container beyond
>> that are you still dreaming about?
>
> We still have a main object that is simply a list of ACEs.  But if that
> doesn't work out (I suspect it should) I don't think the common base
> object is a good idea.  It just leads to a lot of crazy container_of
> calls.

There are two such container_of calls for POSIX ACLs in fs/jffs2/acl.c
[which could be replaced by get_acl()], two in fs/posix_acl.c for
POSIX ACLs, and two in fs/richacl.c for RichACLs. That's it.

> If the common object abstraction doesn't work out we'll need
> a procedural one instead that has common acl_* calls that decide what
> do to based on the file system acl flag.

I've already made such abstractions where it made sense; if you can
find more, I don't see why we shouldn't add them.

Thanks,
Andreas
Reply | Threaded
Open this post in threaded view
|

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

Andreas Gruenbacher-6
In reply to this post by Christoph Hellwig
On Tue, Mar 15, 2016 at 8:17 AM, Christoph Hellwig <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v18 19/22] richacl: Add richacl xattr handler

Christoph Hellwig
In reply to this post by J. Bruce Fields
On Tue, Mar 15, 2016 at 05:05:26PM -0400, J. Bruce Fields wrote:
> > That people get confused between the attr used by the xattr syscall
> > interface and the attr used to store things on disk or the protocol.
> > This has happened every time we have non-native support, e.g. XFS, NFS,
> > CIFS, ntfs, etc.  And it's only going to become worse.
>
> How has that confusion caused problems in practice?

We had all kinds of bugs in this area that were only slowly uncovered.
We also had all kind of privilegue escalations with (non-ACLs) xattrs
as people never grasped the way different free-form namespaces have
different permission checking.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v18 00/22] Richacls (Core and Ext4)

Michael Adam
In reply to this post by Volker Lendecke
On 2016-03-15 at 21:17 +0100, Volker Lendecke wrote:

> On Tue, Mar 15, 2016 at 08:45:14AM -0700, Jeremy Allison wrote:
> > On Tue, Mar 15, 2016 at 12:11:03AM -0700, Christoph Hellwig wrote:
> > > People have long learned that we only have 'alloc' permissions.  Any
> > > model that mixes allow and deny ACE is a mistake.
> >
> > People can also learn and change though :-). One of the
> > biggest complaints people deploying Samba on Linux have is the
> > incompatible ACL models.
>
> Just to confirm: I see this a lot in the field. NFSv4 ACLs, while not a
> perfect match for NTFS ACLs are a lot closer much more usable to people
> who want to serve Windows clients.
>
> Also in the pure linux world there is a lot that you can not express
> with just rwx, sgid, sticky bits and friends. If you want the additional
> functionality of the richacl bits, I would call it a big mistake to
> omit negative aces, if just for the reason not to create yet another
> ACLs flavor.
>
> > Whilst I have sympathy with your intense dislike of the
> > Windows ACL model, this comes down to the core of "who
> > do we serve ?"
>
> The world has enough confusion around ACL semanics, please do not add
> more to it by creating your own model of the day.
Exacty: Like it or not, Windows ACLs are a fact. And the
approximation by the NFSv4 ACLs is getting closer and closer
with each iteration... ;-) So it is not only that Windows world
looking into this.

As Volker and Jeremy have pointed out, the lack of ACL semantics
is one of things the users of Samba complain about most bitterly.
While Samba can work around it when it is acting exclusively on
the files, this is not an option when NFS or other protocols are
to access the data concurrently. In that case we need more
precision down in the file system. So because they make use of
*existing* formats and semantics, I think Andreas' richacls are
just the way to go, as alien as they may seem from the pure linux
filesystem point of view at first.

Cheers - Michael

signature.asc (206 bytes) Download Attachment
123