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

[Andreas GrÃnbacher]

Am Fr., 3. Mai 2019 um 01:24 Uhr schrieb NeilBrown <neilb@xxxxxxxx>:
> On Thu, May 02 2019, Andreas Gruenbacher wrote:
> > On Thu, 2 May 2019 at 05:57, NeilBrown <neilb@xxxxxxxx> wrote:
> >> On Wed, May 01 2019, Amir Goldstein wrote:
> >> > On Wed, May 1, 2019 at 10:03 PM NeilBrown <neilb@xxxxxxxx> 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 <miklos@xxxxxxxxxx> wrote:
> >> >> >> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas GrÃnbacher
> >> >> >> > <andreas.gruenbacher@xxxxxxxxx> wrote:
> >> >> >> >> 2016-12-06 0:19 GMT+01:00 Andreas GrÃnbacher <andreas.gruenbacher@xxxxxxxxx>:
> >> >> >> >
> >> >> >> >>> 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.
>
> Maybe ... but this feels a bit like "sweeping it under the carpet".
> What happens if some file on the lower layer does have a more complex
> ACL?
> Do we just fail any attempt to modify that object?  Doesn't that violate
> the law of least surprise?

It would at least expose that there is a problem only if there is an
actual problem.

> Maybe if the lower-layer has an i_op->permission method, then overlayfs
> should *always* call that for permission checking - unless a
> chmod/chown/etc has happened on the file.  That way, we wouldn't need to
> copy-up the ACL, but would still get correct ACL testing.

No, the permissions need to stick with the object. Otherwise, what
would you do on rename or when the lower layer changes?

Andreas

> Thanks,
> NeilBrown
>
>
> > 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.
> >
> > 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.