2019-05-02 15:08:57

by Andreas Grünbacher

[permalink] [raw]
Subject: Re: [PATCH] overlayfs: ignore empty NFSv4 ACLs in ext4 upperdir

Am Do., 2. Mai 2019 um 16:28 Uhr schrieb Miklos Szeredi <[email protected]>:
> On Thu, May 2, 2019 at 10:05 AM Andreas Gruenbacher <[email protected]> wrote:
> > On Thu, 2 May 2019 at 05:57, NeilBrown <[email protected]> wrote:
> > > On Wed, May 01 2019, Amir Goldstein wrote:
> > > > On Wed, May 1, 2019 at 10:03 PM NeilBrown <[email protected]> wrote:
> > > >> On Tue, Dec 06 2016, J. Bruce Fields wrote:
> > > >> > On Tue, Dec 06, 2016 at 02:18:31PM +0100, Andreas Gruenbacher wrote:
> > > >> >> On Tue, Dec 6, 2016 at 11:08 AM, Miklos Szeredi <[email protected]> wrote:
> > > >> >> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
> > > >> >> > <[email protected]> wrote:
> > > >> >> >> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <[email protected]>:
> > > >> >> >
> > > >> >> >>> It's not hard to come up with a heuristic that determines if a
> > > >> >> >>> system.nfs4_acl value is equivalent to a file mode, and to ignore the
> > > >> >> >>> attribute in that case. (The file mode is transmitted in its own
> > > >> >> >>> attribute already, so actually converting .) That way, overlayfs could
> > > >> >> >>> still fail copying up files that have an actual ACL. It's still an
> > > >> >> >>> ugly hack ...
> > > >> >> >>
> > > >> >> >> Actually, that kind of heuristic would make sense in the NFS client
> > > >> >> >> which could then hide the "system.nfs4_acl" attribute.
> >
> > I still think the nfs client could make this problem mostly go away by
> > not exposing "system.nfs4_acl" xattrs when the acl is equivalent to
> > the file mode. The richacl patches contain a workable abgorithm for
> > that. The problem would remain for files that have an actual NFS4 ACL,
> > which just cannot be mapped to a file mode or to POSIX ACLs in the
> > general case, as well as for files that have a POSIX ACL. Mapping NFS4
> > ACL that used to be a POSIX ACL back to POSIX ACLs could be achieved
> > in many cases as well, but the code would be quite messy. A better way
> > seems to be to using a filesystem that doesn't support POSIX ACLs in
> > the first place. Unfortunately, xfs doesn't allow turning off POSIX
> > ACLs, for example.
>
> How about mounting NFSv4 with noacl? That should fix this issue, right?

You'll still see permissions that differ from what the filesystem
enforces, and copy-up would change that behavior.

Andreas

> Thanks,
> Miklos
>
>
>
> >
> > Andreas
> >
> > > >> >> > Even simpler would be if knfsd didn't send the attribute if not
> > > >> >> > necessary. Looks like there's code actively creating the nfs4_acl on
> > > >> >> > the wire even if the filesystem had none:
> > > >> >> >
> > > >> >> > pacl = get_acl(inode, ACL_TYPE_ACCESS);
> > > >> >> > if (!pacl)
> > > >> >> > pacl = posix_acl_from_mode(inode->i_mode, GFP_KERNEL);
> > > >> >> >
> > > >> >> > What's the point?
> > > >> >>
> > > >> >> That's how the protocol is specified.
> > > >> >
> > > >> > Yep, even if we could make that change to nfsd it wouldn't help the
> > > >> > client with the large number of other servers that are out there
> > > >> > (including older knfsd's).
> > > >> >
> > > >> > --b.
> > > >> >
> > > >> >> (I'm not saying that that's very helpful.)
> > > >> >>
> > > >> >> Andreas
> > > >>
> > > >> Hi everyone.....
> > > >> I have a customer facing this problem, and so stumbled onto the email
> > > >> thread.
> > > >> Unfortunately it didn't resolve anything. Maybe I can help kick things
> > > >> along???
> > > >>
> > > >> The core problem here is that NFSv4 and ext4 use different and largely
> > > >> incompatible ACL implementations. There is no way to accurately
> > > >> translate from one to the other in general (common specific examples
> > > >> can be converted).
> > > >>
> > > >> This means that either:
> > > >> 1/ overlayfs cannot use ext4 for upper and NFS for lower (or vice
> > > >> versa) or
> > > >> 2/ overlayfs need to accept that sometimes it cannot copy ACLs, and
> > > >> that is OK.
> > > >>
> > > >> Silently not copying the ACLs is probably not a good idea as it might
> > > >> result in inappropriate permissions being given away.
> > > >
> > > > For example? permissions given away to do what?
> > > > Note that ovl_permission() only check permissions of *mounter*
> > > > to read the lower NFS file and ovl_open()/ovl_read_iter() access
> > > > the lower file with *mounter* credentials.
> > > >
> > > > I might be wrong, but seems to me that once admin mounted
> > > > overlayfs with lower NFS, NFS ACLs are not being enforced at all
> > > > even before copy up.
> > >
> > > I guess it is just as well that copy-up fails then - if the lower-level
> > > permission check is being ignored.
> > >
> > > >
> > > >> So if the
> > > >> sysadmin wants this (and some clearly do), they need a way to
> > > >> explicitly say "I accept the risk". If only standard Unix permissions
> > > >> are used, there is no risk, so this seems reasonable.
> > > >>
> > > >> So I would like to propose a new option for overlayfs
> > > >> nocopyupacl: when overlayfs is copying a file (or directory etc)
> > > >> from the lower filesystem to the upper filesystem, it does not
> > > >> copy extended attributes with the "system." prefix. These are
> > > >> used for storing ACL information and this is sometimes not
> > > >> compatible between different filesystem types (e.g. ext4 and
> > > >> NFSv4). Standard Unix ownership permission flags (rwx) *are*
> > > >> copied so this option does not risk giving away inappropriate
> > > >> permissions unless the lowerfs uses unusual ACLs.
> > > >>
> > > >>
> > > >
> > > > I am wondering if it would make more sense for nfs to register a
> > > > security_inode_copy_up_xattr() hook.
> > > > That is the mechanism that prevents copying up other security.*
> > > > xattrs?
> > >
> > > No, I don't think that would make sense.
> > > Support some day support for nfs4 acls were added to ext4 (not a totally
> > > ridiculous suggestion). We would then want NFS to allow it's ACLs to be
> > > copied up.
> > >
> > > Thanks,
> > > NeilBrown
> > >
> > >
> > > >
> > > > Thanks,
> > > > Amir.


2019-05-02 17:16:32

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] overlayfs: ignore empty NFSv4 ACLs in ext4 upperdir

On Thu, May 02, 2019 at 05:08:14PM +0200, Andreas Grünbacher wrote:
> You'll still see permissions that differ from what the filesystem
> enforces, and copy-up would change that behavior.

That's always true, and this issue isn't really specific to NFSv4 ACLs
(or ACLs at all), it already exists with just mode bits. The client
doesn't know how principals may be mapped on the server, doesn't know
group membership, etc.

That's the usual model, anyway. Permissions are almost entirely the
server's responsibility, and we just provide a few attributes to set/get
those server-side permissions.

The overlayfs/NFS case is different, I think: the nfs filesystem may be
just a static read-only template for a filesystem that's only ever used
by clients, and for all I know maybe permissions should only be
interpreted on the client side in that case.

--b.

2019-05-02 17:53:44

by Andreas Gruenbacher

[permalink] [raw]
Subject: Re: [PATCH] overlayfs: ignore empty NFSv4 ACLs in ext4 upperdir

On Thu, 2 May 2019 at 19:16, J. Bruce Fields <[email protected]> wrote:
> On Thu, May 02, 2019 at 05:08:14PM +0200, Andreas Grünbacher wrote:
> > You'll still see permissions that differ from what the filesystem
> > enforces, and copy-up would change that behavior.
>
> That's always true, and this issue isn't really specific to NFSv4 ACLs
> (or ACLs at all), it already exists with just mode bits. The client
> doesn't know how principals may be mapped on the server, doesn't know
> group membership, etc.
>
> That's the usual model, anyway. Permissions are almost entirely the
> server's responsibility, and we just provide a few attributes to set/get
> those server-side permissions.

Sure, if the client and server don't share the same user and group
databases, ACLs can get a very different meaning.

Andreas

> The overlayfs/NFS case is different, I think: the nfs filesystem may be
> just a static read-only template for a filesystem that's only ever used
> by clients, and for all I know maybe permissions should only be
> interpreted on the client side in that case.
>
> --b.