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

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

[Miklos Szeredi]

[Added a few more CCs]

On Mon, Dec 5, 2016 at 1:51 AM, Patrick Plagwitz
<Patrick_Plagwitz@web.de> wrote:
> Mounting an overlayfs with an NFSv4 lowerdir and an ext4 upperdir causes copy_up operations, specifically the function copy_up.c:ovl_copy_xattr, to fail with EOPNOTSUPP.
> For example, having the following folders:
>
> |- nfs <- NFSv4 is mounted here
> |--|- folder
> |- root <- ext4 is mounted here
> |- work <- also ext4
> |- merged <- overlay is mounted here with
>              lowerdir=nfs,upperdir=root,workdir=work
>
> And calling
> # touch merged/folder/file
> will print
> touch: cannot touch 'merged/folder/file': Operation not supported
>
> This is because NFS returns the xattr system.nfs4_acl with an empty value even if no NFS ACLs are in use in the lower filesystem. Trying to set this xattr in the upperdir
> fails because ext4 does not support it.
>
> Fix this by explicitly checking for the name of the xattr and an empty value and ignoring EOPNOTSUPP if both things match.
>
> Signed-off-by: Patrick Plagwitz <patrick_plagwitz@web.de>
> ---
> Maybe NFS could be changed to not return empty system.nfs4_acl values, I don't know. In any case, to support upperdir ext4 + lowerdir NFSv4, returning the error code from
> vfs_setxattr with this xattr name must be avoided as long as the value is empty.
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 36795ee..505b86e 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -123,6 +123,9 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
>                         continue; /* Discard */
>                 }
>                 error = vfs_setxattr(new, name, value, size, 0);
> +               if (error == -EOPNOTSUPP && *value == '\0' &&
> +                               strcmp(name, "system.nfs4_acl") == 0)
> +                       error = 0;
>                 if (error)
>                         break;
>         }

I agree that this should be fixed, but adding such exceptions for
certain filesystems or xattrs is not the proper way, IMO.

Can NFS people comment on this?  Where does the nfs4_acl come from?

What can overlayfs do if it's a non-empty ACL?

Does knfsd translate posix ACL into NFS acl?  If so, we can translate
back.  Should we do a generic POSIX<->NFS acl translator?

Thanks,
Miklos

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

[J. Bruce Fields]

On Mon, Dec 05, 2016 at 10:28:18AM +0100, Miklos Szeredi wrote:
> [Added a few more CCs]
> 
> On Mon, Dec 5, 2016 at 1:51 AM, Patrick Plagwitz
> <Patrick_Plagwitz@web.de> wrote:
> > Mounting an overlayfs with an NFSv4 lowerdir and an ext4 upperdir causes copy_up operations, specifically the function copy_up.c:ovl_copy_xattr, to fail with EOPNOTSUPP.
> > For example, having the following folders:
> >
> > |- nfs <- NFSv4 is mounted here
> > |--|- folder
> > |- root <- ext4 is mounted here
> > |- work <- also ext4
> > |- merged <- overlay is mounted here with
> >              lowerdir=nfs,upperdir=root,workdir=work
> >
> > And calling
> > # touch merged/folder/file
> > will print
> > touch: cannot touch 'merged/folder/file': Operation not supported
> >
> > This is because NFS returns the xattr system.nfs4_acl with an empty value even if no NFS ACLs are in use in the lower filesystem. Trying to set this xattr in the upperdir
> > fails because ext4 does not support it.
> >
> > Fix this by explicitly checking for the name of the xattr and an empty value and ignoring EOPNOTSUPP if both things match.
> >
> > Signed-off-by: Patrick Plagwitz <patrick_plagwitz@web.de>
> > ---
> > Maybe NFS could be changed to not return empty system.nfs4_acl values, I don't know. In any case, to support upperdir ext4 + lowerdir NFSv4, returning the error code from
> > vfs_setxattr with this xattr name must be avoided as long as the value is empty.
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 36795ee..505b86e 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -123,6 +123,9 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
> >                         continue; /* Discard */
> >                 }
> >                 error = vfs_setxattr(new, name, value, size, 0);
> > +               if (error == -EOPNOTSUPP && *value == '\0' &&
> > +                               strcmp(name, "system.nfs4_acl") == 0)
> > +                       error = 0;
> >                 if (error)
> >                         break;
> >         }
> 
> I agree that this should be fixed, but adding such exceptions for
> certain filesystems or xattrs is not the proper way, IMO.
> 
> Can NFS people comment on this?  Where does the nfs4_acl come from?

This is the interface the NFS client provides for applications to modify
NFSv4 ACLs on servers that support them.

> What can overlayfs do if it's a non-empty ACL?

As little as possible.  You can't copy it up, can you?  So any attempt
to support it is going to be incomplete.

> Does knfsd translate posix ACL into NFS acl?  If so, we can translate
> back.  Should we do a generic POSIX<->NFS acl translator?

knsd does translate between POSIX and NFSv4 ACLs.  It's a complicated
algorithm, and lossy (in the NFSv4->POSIX direction).  The client
developers have been understandably reluctant to have anything to do
with it.

So, I think listxattr should omit system.nfs4_acl, and attempts to
set/get the attribute should error out.  The same should apply to any
"system." attribute not supported by both filesystems, I think?

I don't understand overlayfs very well, though.

--b.

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

[Miklos Szeredi]

On Mon, Dec 5, 2016 at 4:19 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> Can NFS people comment on this?  Where does the nfs4_acl come from?
>
> This is the interface the NFS client provides for applications to modify
> NFSv4 ACLs on servers that support them.

Fine, but why are we seeing this xattr on exports where no xattrs are
set on the exported fs?

>> What can overlayfs do if it's a non-empty ACL?
>
> As little as possible.  You can't copy it up, can you?  So any attempt
> to support it is going to be incomplete.

Right.

>
>> Does knfsd translate posix ACL into NFS acl?  If so, we can translate
>> back.  Should we do a generic POSIX<->NFS acl translator?
>
> knsd does translate between POSIX and NFSv4 ACLs.  It's a complicated

This does explain the nfs4_acl xattr on the client.  Question: if it's
empty, why have it at all?

> algorithm, and lossy (in the NFSv4->POSIX direction).  The client
> developers have been understandably reluctant to have anything to do
> with it.
>
> So, I think listxattr should omit system.nfs4_acl, and attempts to
> set/get the attribute should error out.  The same should apply to any
> "system." attribute not supported by both filesystems, I think?

Basically that's what happens now.  The problem is that nfsv4 mounts
seem always have these xattrs, even when the exported fs doesn't have
anything.

We could do the copy up even if the NFS4->POSIX translation was
possible (which is the case with POSIX ACL translated by knfsd).  We'd
just get back the original ACL, so that's OK.  NFS is only supported
as lower (read-only) layer, so we don't care about changing the ACL on
the server.

Thanks,
Miklos

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

[J. Bruce Fields]

On Mon, Dec 05, 2016 at 04:36:03PM +0100, Miklos Szeredi wrote:
> On Mon, Dec 5, 2016 at 4:19 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >> Can NFS people comment on this?  Where does the nfs4_acl come from?
> >
> > This is the interface the NFS client provides for applications to modify
> > NFSv4 ACLs on servers that support them.
> 
> Fine, but why are we seeing this xattr on exports where no xattrs are
> set on the exported fs?

I don't know.  I took another look at the original patch and don't see
any details on the server setup: which server is it (knfsd, ganesha,
netapp, ...)?  How is it configured?

> >> What can overlayfs do if it's a non-empty ACL?
> >
> > As little as possible.  You can't copy it up, can you?  So any attempt
> > to support it is going to be incomplete.
> 
> Right.
> 
> >
> >> Does knfsd translate posix ACL into NFS acl?  If so, we can translate
> >> back.  Should we do a generic POSIX<->NFS acl translator?
> >
> > knsd does translate between POSIX and NFSv4 ACLs.  It's a complicated
> 
> This does explain the nfs4_acl xattr on the client.  Question: if it's
> empty, why have it at all?

I'm honestly not sure what's going on there.  I'd be curious to see a
network trace if possible.

> > algorithm, and lossy (in the NFSv4->POSIX direction).  The client
> > developers have been understandably reluctant to have anything to do
> > with it.
> >
> > So, I think listxattr should omit system.nfs4_acl, and attempts to
> > set/get the attribute should error out.  The same should apply to any
> > "system." attribute not supported by both filesystems, I think?
> 
> Basically that's what happens now.  The problem is that nfsv4 mounts
> seem always have these xattrs, even when the exported fs doesn't have
> anything.

I said "both", that's a logical "and".  Whether or not nfs claims
support would then be irrelevant in this case, since ext4 doesn't
support system.nfs4_acl.

> We could do the copy up even if the NFS4->POSIX translation was
> possible (which is the case with POSIX ACL translated by knfsd).  We'd
> just get back the original ACL, so that's OK.

Note that knfsd is an exception, most NFSv4-acl-supporting servers
aren't translating from POSIX ACLs.

> NFS is only supported as lower (read-only) layer, so we don't care
> about changing the ACL on the server.

Out of curiosity, how do you check permissions after copy up?

The client doesn't do much permissions-checking normally, because it's
hard to get right--even in the absence of ACLs, it may not understand
the server's owners and groups completely.

I guess that's fine, you may be happy to let people write to the file
without permissions to the lower file, since the writes aren't going
there anyway.

So, I don't know what want here.

You're not going to want to use the ACL for actual permission-checking,
and you can't modify it, so it doesn't seem very useful.

--b.

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

[Patrick Plagwitz]

On 12/05/2016 05:25 PM, J. Bruce Fields wrote:
> On Mon, Dec 05, 2016 at 04:36:03PM +0100, Miklos Szeredi wrote:
>> On Mon, Dec 5, 2016 at 4:19 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>>>> Can NFS people comment on this?  Where does the nfs4_acl come from?
>>>
>>> This is the interface the NFS client provides for applications to modify
>>> NFSv4 ACLs on servers that support them.
>>
>> Fine, but why are we seeing this xattr on exports where no xattrs are
>> set on the exported fs?
> 
> I don't know.  I took another look at the original patch and don't see
> any details on the server setup: which server is it (knfsd, ganesha,
> netapp, ...)?  How is it configured?
> 
The server is the one in the kernel (knfsd, I assume), with completely default
configuration. /etc/exports is

/srv/nfsv4 localhost(fsid=root)

Adding rw, or various other options, does not change the observations in the
original mail. As far as I know, there are no options for controlling ACLs.

>>>> What can overlayfs do if it's a non-empty ACL?
>>>
>>> As little as possible.  You can't copy it up, can you?  So any attempt
>>> to support it is going to be incomplete.
>>
>> Right.
>>
>>>
>>>> Does knfsd translate posix ACL into NFS acl?  If so, we can translate
>>>> back.  Should we do a generic POSIX<->NFS acl translator?
>>>
>>> knsd does translate between POSIX and NFSv4 ACLs.  It's a complicated
>>
>> This does explain the nfs4_acl xattr on the client.  Question: if it's
>> empty, why have it at all?
> 
> I'm honestly not sure what's going on there.  I'd be curious to see a
> network trace if possible.
> 
Just make folders like in the original mail, start the NFS server with the
exports from above and touch merged/folder/file to reproduce the problem.
I don't know anything about the NFS protocol but here you have a tshark
packet summary as well as the details from the folder/ lookup reply (51).
As far as I can tell, no ACL information is in there.

 22 NFS 286 V4 Call CREATE_SESSION
 23 NFS 214 V4 Reply (Call In 22) CREATE_SESSION
 24 NFS 214 V4 Call RECLAIM_COMPLETE
 26 NFS 178 V4 Reply (Call In 24) RECLAIM_COMPLETE
 27 NFS 218 V4 Call SECINFO_NO_NAME
 29 NFS 194 V4 Reply (Call In 27) SECINFO_NO_NAME
 30 NFS 230 V4 Call PUTROOTFH | GETATTR
 31 NFS 350 V4 Reply (Call In 30) PUTROOTFH | GETATTR
 32 NFS 242 V4 Call GETATTR FH: 0x62d40c52
 33 NFS 254 V4 Reply (Call In 32) GETATTR
 34 NFS 242 V4 Call GETATTR FH: 0x62d40c52
 35 NFS 254 V4 Reply (Call In 34) GETATTR
 36 NFS 242 V4 Call GETATTR FH: 0x62d40c52
 37 NFS 254 V4 Reply (Call In 36) GETATTR
 38 NFS 242 V4 Call GETATTR FH: 0x62d40c52
 39 NFS 254 V4 Reply (Call In 38) GETATTR
 40 NFS 234 V4 Call GETATTR FH: 0x62d40c52
 41 NFS 206 V4 Reply (Call In 40) GETATTR
 42 NFS 242 V4 Call GETATTR FH: 0x62d40c52
 43 NFS 254 V4 Reply (Call In 42) GETATTR
 44 NFS 238 V4 Call GETATTR FH: 0x62d40c52
 45 NFS 330 V4 Reply (Call In 44) GETATTR
 46 NFS 246 V4 Call ACCESS FH: 0x62d40c52, [Check: RD LU MD XT DL]
 47 NFS 258 V4 Reply (Call In 46) ACCESS, [Access Denied: MD XT DL], [Allowed: RD LU]
 48 NFS 238 V4 Call GETATTR FH: 0x62d40c52
 49 NFS 250 V4 Reply (Call In 48) GETATTR
 50 NFS 258 V4 Call LOOKUP DH: 0x62d40c52/folder
 51 NFS 366 V4 Reply (Call In 50) LOOKUP
 52 NFS 254 V4 Call ACCESS FH: 0x96d0c04a, [Check: RD LU MD XT DL]
 53 NFS 258 V4 Reply (Call In 52) ACCESS, [Access Denied: MD XT DL], [Allowed: RD LU]
 54 NFS 262 V4 Call LOOKUP DH: 0x96d0c04a/file
 55 NFS 186 V4 Reply (Call In 54) LOOKUP Status: NFS4ERR_NOENT
 56 NFS 246 V4 Call GETATTR FH: 0x96d0c04a
 57 NFS 278 V4 Reply (Call In 56) GETATTR

Network File System, Ops(5): SEQUENCE PUTFH LOOKUP GETFH GETATTR
    [Program Version: 4]
    [V4 Procedure: COMPOUND (1)]
    Status: NFS4_OK (0)
    Tag: <EMPTY>
        length: 0
        contents: <EMPTY>
    Operations (count: 5)
        Opcode: SEQUENCE (53)
            Status: NFS4_OK (0)
            sessionid: 50a245584a819c9a0a00000000000000
            seqid: 0x0000000d
            slot id: 0
            high slot id: 30
            target high slot id: 30
            status flags: 0x00000000
                .... .... .... .... .... .... .... ...0 = SEQ4_STATUS_CB_PATH_DOWN: Not set
                .... .... .... .... .... .... .... ..0. = SEQ4_STATUS_CB_GSS_CONTEXTS_EXPIRING: Not set
                .... .... .... .... .... .... .... .0.. = SEQ4_STATUS_CB_GSS_CONTEXTS_EXPIRED: Not set
                .... .... .... .... .... .... .... 0... = SEQ4_STATUS_EXPIRED_ALL_STATE_REVOKED: Not set
                .... .... .... .... .... .... ...0 .... = SEQ4_STATUS_EXPIRED_SOME_STATE_REVOKED: Not set
                .... .... .... .... .... .... ..0. .... = SEQ4_STATUS_ADMIN_STATE_REVOKED: Not set
                .... .... .... .... .... .... .0.. .... = SEQ4_STATUS_RECALLABLE_STATE_REVOKED: Not set
                .... .... .... .... .... .... 0... .... = SEQ4_STATUS_LEASE_MOVED: Not set
                .... .... .... .... .... ...0 .... .... = SEQ4_STATUS_RESTART_RECLAIM_NEEDED: Not set
                .... .... .... .... .... ..0. .... .... = SEQ4_STATUS_CB_PATH_DOWN_SESSION: Not set
                .... .... .... .... .... .0.. .... .... = SEQ4_STATUS_BACKCHANNEL_FAULT: Not set
                .... .... .... .... .... 0... .... .... = SEQ4_STATUS_DEVID_CHANGED: Not set
                .... .... .... .... ...0 .... .... .... = SEQ4_STATUS_DEVID_DELETED: Not set
        Opcode: PUTFH (22)
            Status: NFS4_OK (0)
        Opcode: LOOKUP (15)
            Status: NFS4_OK (0)
        Opcode: GETFH (10)
            Status: NFS4_OK (0)
            Filehandle
                length: 16
                [hash (CRC-32): 0x96d0c04a]
                filehandle: 0100010100000000790f04000762a435
        Opcode: GETATTR (9)
            Status: NFS4_OK (0)
            Attr mask[0]: 0x0010011a (Type, Change, Size, FSID, FileId)
                reqd_attr: Type (1)
                    ftype4: NF4DIR (2)
                reqd_attr: Change (3)
                    changeid: 6360241138682687914
                reqd_attr: Size (4)
                    size: 4096
                reqd_attr: FSID (8)
                    fattr4_fsid
                        fsid4.major: 0
                        fsid4.minor: 0
                reco_attr: FileId (20)
                    fileid: 266105
            Attr mask[1]: 0x00b0a23a (Mode, NumLinks, Owner, Owner_Group, RawDev, Space_Used, Time_Access, Time_Metadata, Time_Modify, Mounted_on_FileId)
                reco_attr: Mode (33)
                    mode: 0755, Name: Unknown, Read permission for owner, Write permission for owner, Execute permission for owner, Read permission for group, Execute
permission for group, Read permission for others, Execute permission for others
                        .... .... .... .... 000. .... .... .... = Name: Unknown (0)
                        .... .... .... .... .... 0... .... .... = Set user id on exec: No
                        .... .... .... .... .... .0.. .... .... = Set group id on exec: No
                        .... .... .... .... .... ..0. .... .... = Save swapped text even after use: No
                        .... .... .... .... .... ...1 .... .... = Read permission for owner: Yes
                        .... .... .... .... .... .... 1... .... = Write permission for owner: Yes
                        .... .... .... .... .... .... .1.. .... = Execute permission for owner: Yes
                        .... .... .... .... .... .... ..1. .... = Read permission for group: Yes
                        .... .... .... .... .... .... ...0 .... = Write permission for group: No
                        .... .... .... .... .... .... .... 1... = Execute permission for group: Yes
                        .... .... .... .... .... .... .... .1.. = Read permission for others: Yes
                        .... .... .... .... .... .... .... ..0. = Write permission for others: No
                        .... .... .... .... .... .... .... ...1 = Execute permission for others: Yes
                reco_attr: NumLinks (35)
                    numlinks: 2
                reco_attr: Owner (36)
                    fattr4_owner: 0
                        length: 1
                        contents: 0
                        fill bytes: opaque data
                reco_attr: Owner_Group (37)
                    fattr4_owner_group: 0
                        length: 1
                        contents: 0
                        fill bytes: opaque data
                reco_attr: RawDev (41)
                    specdata1: 0
                    specdata2: 0
                reco_attr: Space_Used (45)
                    space_used: 4096
                reco_attr: Time_Access (47)
                    seconds: 1480888617
                    nseconds: 513333330
                reco_attr: Time_Metadata (52)
                    seconds: 1480859038
                    nseconds: 486666666
                reco_attr: Time_Modify (53)
                    seconds: 1480859038
                    nseconds: 486666666
                reco_attr: Mounted_on_FileId (55)
                    fileid: 0x0000000000040f79
    [Main Opcode: LOOKUP (15)]

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

[Andreas Grünbacher]

2016-12-05 17:25 GMT+01:00 J. Bruce Fields <bfields@fieldses.org>:
> On Mon, Dec 05, 2016 at 04:36:03PM +0100, Miklos Szeredi wrote:
>> On Mon, Dec 5, 2016 at 4:19 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> >> Can NFS people comment on this?  Where does the nfs4_acl come from?
>> >
>> > This is the interface the NFS client provides for applications to modify
>> > NFSv4 ACLs on servers that support them.
>>
>> Fine, but why are we seeing this xattr on exports where no xattrs are
>> set on the exported fs?
>
> I don't know.  I took another look at the original patch and don't see
> any details on the server setup: which server is it (knfsd, ganesha,
> netapp, ...)?  How is it configured?
>
>> >> What can overlayfs do if it's a non-empty ACL?
>> >
>> > As little as possible.  You can't copy it up, can you?  So any attempt
>> > to support it is going to be incomplete.
>>
>> Right.
>>
>> >
>> >> Does knfsd translate posix ACL into NFS acl?  If so, we can translate
>> >> back.  Should we do a generic POSIX<->NFS acl translator?
>> >
>> > knsd does translate between POSIX and NFSv4 ACLs.  It's a complicated
>>
>> This does explain the nfs4_acl xattr on the client.  Question: if it's
>> empty, why have it at all?
>
> I'm honestly not sure what's going on there.  I'd be curious to see a
> network trace if possible.

I do see "system.nfs4_acl" attributes on knfsd exported filesystems
that support POSIX ACLs (for ext4: "mount -o acl"). For exported
filesystem that don't support POSIX ACLs (ext4: mount -o noacl), that
attribute is missing. The attribute shouldn't be empty though; when
the file has no real ACL, "system.nfs4_acl" represents the file mode
permissions. The "system.nfs4_acl" attribute exposes the information
on the wire; there is no resonable way to translate that into an ACL
on another filesystem, really.

Patrick, what does 'getfattr -m- -d /nfs/file' give you?

>> > algorithm, and lossy (in the NFSv4->POSIX direction).  The client
>> > developers have been understandably reluctant to have anything to do
>> > with it.
>> >
>> > So, I think listxattr should omit system.nfs4_acl, and attempts to
>> > set/get the attribute should error out.  The same should apply to any
>> > "system." attribute not supported by both filesystems, I think?
>>
>> Basically that's what happens now.  The problem is that nfsv4 mounts
>> seem always have these xattrs, even when the exported fs doesn't have
>> anything.
>
> I said "both", that's a logical "and".  Whether or not nfs claims
> support would then be irrelevant in this case, since ext4 doesn't
> support system.nfs4_acl.
>
>> We could do the copy up even if the NFS4->POSIX translation was
>> possible (which is the case with POSIX ACL translated by knfsd).  We'd
>> just get back the original ACL, so that's OK.
>
> Note that knfsd is an exception, most NFSv4-acl-supporting servers
> aren't translating from POSIX ACLs.
>
>> NFS is only supported as lower (read-only) layer, so we don't care
>> about changing the ACL on the server.
>
> Out of curiosity, how do you check permissions after copy up?
>
> The client doesn't do much permissions-checking normally, because it's
> hard to get right--even in the absence of ACLs, it may not understand
> the server's owners and groups completely.
>
> I guess that's fine, you may be happy to let people write to the file
> without permissions to the lower file, since the writes aren't going
> there anyway.
>
> So, I don't know what want here.
>
> You're not going to want to use the ACL for actual permission-checking,
> and you can't modify it, so it doesn't seem very useful.

IMO the only thing overlayfs can do is hide those attributes from the
user and ignore them when copying up. Still, the attributes shouldn't
be empty.

Andreas

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

[Patrick Plagwitz]

On 12/05/2016 08:37 PM, Andreas Grünbacher wrote:
> 2016-12-05 17:25 GMT+01:00 J. Bruce Fields <bfields@fieldses.org>:
>> On Mon, Dec 05, 2016 at 04:36:03PM +0100, Miklos Szeredi wrote:
>>> On Mon, Dec 5, 2016 at 4:19 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>>>>> Can NFS people comment on this?  Where does the nfs4_acl come from?
>>>>
>>>> This is the interface the NFS client provides for applications to modify
>>>> NFSv4 ACLs on servers that support them.
>>>
>>> Fine, but why are we seeing this xattr on exports where no xattrs are
>>> set on the exported fs?
>>
>> I don't know.  I took another look at the original patch and don't see
>> any details on the server setup: which server is it (knfsd, ganesha,
>> netapp, ...)?  How is it configured?
>>
>>>>> What can overlayfs do if it's a non-empty ACL?
>>>>
>>>> As little as possible.  You can't copy it up, can you?  So any attempt
>>>> to support it is going to be incomplete.
>>>
>>> Right.
>>>
>>>>
>>>>> Does knfsd translate posix ACL into NFS acl?  If so, we can translate
>>>>> back.  Should we do a generic POSIX<->NFS acl translator?
>>>>
>>>> knsd does translate between POSIX and NFSv4 ACLs.  It's a complicated
>>>
>>> This does explain the nfs4_acl xattr on the client.  Question: if it's
>>> empty, why have it at all?
>>
>> I'm honestly not sure what's going on there.  I'd be curious to see a
>> network trace if possible.
> 
> I do see "system.nfs4_acl" attributes on knfsd exported filesystems
> that support POSIX ACLs (for ext4: "mount -o acl"). For exported
> filesystem that don't support POSIX ACLs (ext4: mount -o noacl), that
> attribute is missing. The attribute shouldn't be empty though; when
> the file has no real ACL, "system.nfs4_acl" represents the file mode
> permissions. The "system.nfs4_acl" attribute exposes the information
> on the wire; there is no resonable way to translate that into an ACL
> on another filesystem, really.
> 
> Patrick, what does 'getfattr -m- -d /nfs/file' give you?
> 
getfattr -m - -d nfs/folder -e text gives

# file: nfs/folder/
system.nfs4_acl="\000\000\000^C\000\000\000\000\000\000\000\000\000^V^A<E7>\000\000\000^FOWNER@\000\000\000\000\000\000\000\000\000\000\000^R\000<A1>\000\000\000^FGROUP@\000\000\000\000\000\000\000\000\000\000\000^R\000<A1>\000\000\000
    EVERYONE@\000\000"

Those are 80 bytes. I checked again and vfs_getxattr indeed returns size=80.
It just looked empty because the first byte is 0... Ok, so nfs4_acl is not
empty after all and checking *value == 0 does not tell if there are actually
ACLs present or not, sorry for the confusion.

You are right, when I mount the exported fs with noacl the problem goes away.
You already helped me there, thanks.

Still, I think there should be a way to copy up files that actually have no
ACLs since acl is often the default for ext4 mounts and giving an "Operation
not supported" for random open(2)s is not a very good way to convey what's
going on.

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

[Andreas Grünbacher]

2016-12-05 23:58 GMT+01:00 Patrick Plagwitz <Patrick_Plagwitz@web.de>:
> On 12/05/2016 08:37 PM, Andreas Gr=C3=BCnbacher wrote:
>> 2016-12-05 17:25 GMT+01:00 J. Bruce Fields <bfields@fieldses.org>:
>>> On Mon, Dec 05, 2016 at 04:36:03PM +0100, Miklos Szeredi wrote:
>>>> On Mon, Dec 5, 2016 at 4:19 PM, J. Bruce Fields <bfields@fieldses.org>=
 wrote:
>>>>>> Can NFS people comment on this?  Where does the nfs4_acl come from?
>>>>>
>>>>> This is the interface the NFS client provides for applications to mod=
ify
>>>>> NFSv4 ACLs on servers that support them.
>>>>
>>>> Fine, but why are we seeing this xattr on exports where no xattrs are
>>>> set on the exported fs?
>>>
>>> I don't know.  I took another look at the original patch and don't see
>>> any details on the server setup: which server is it (knfsd, ganesha,
>>> netapp, ...)?  How is it configured?
>>>
>>>>>> What can overlayfs do if it's a non-empty ACL?
>>>>>
>>>>> As little as possible.  You can't copy it up, can you?  So any attemp=
t
>>>>> to support it is going to be incomplete.
>>>>
>>>> Right.
>>>>
>>>>>
>>>>>> Does knfsd translate posix ACL into NFS acl?  If so, we can translat=
e
>>>>>> back.  Should we do a generic POSIX<->NFS acl translator?
>>>>>
>>>>> knsd does translate between POSIX and NFSv4 ACLs.  It's a complicated
>>>>
>>>> This does explain the nfs4_acl xattr on the client.  Question: if it's
>>>> empty, why have it at all?
>>>
>>> I'm honestly not sure what's going on there.  I'd be curious to see a
>>> network trace if possible.
>>
>> I do see "system.nfs4_acl" attributes on knfsd exported filesystems
>> that support POSIX ACLs (for ext4: "mount -o acl"). For exported
>> filesystem that don't support POSIX ACLs (ext4: mount -o noacl), that
>> attribute is missing. The attribute shouldn't be empty though; when
>> the file has no real ACL, "system.nfs4_acl" represents the file mode
>> permissions. The "system.nfs4_acl" attribute exposes the information
>> on the wire; there is no resonable way to translate that into an ACL
>> on another filesystem, really.
>>
>> Patrick, what does 'getfattr -m- -d /nfs/file' give you?
>>
> getfattr -m - -d nfs/folder -e text gives
>
> # file: nfs/folder/
> system.nfs4_acl=3D"\000\000\000^C\000\000\000\000\000\000\000\000\000^V^A=
<E7>\000\000\000^FOWNER@\000\000\000\000\000\000\000\000\000\000\000^R\000<=
A1>\000\000\000^FGROUP@\000\000\000\000\000\000\000\000\000\000\000^R\000<A=
1>\000\000\000
>     EVERYONE@\000\000"
>
> Those are 80 bytes. I checked again and vfs_getxattr indeed returns size=
=3D80.
> It just looked empty because the first byte is 0... Ok, so nfs4_acl is no=
t
> empty after all and checking *value =3D=3D 0 does not tell if there are a=
ctually
> ACLs present or not, sorry for the confusion.
>
> You are right, when I mount the exported fs with noacl the problem goes a=
way.
> You already helped me there, thanks.
>
> Still, I think there should be a way to copy up files that actually have =
no
> ACLs since acl is often the default for ext4 mounts and giving an "Operat=
ion
> not supported" for random open(2)s is not a very good way to convey what'=
s
> going on.

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 ...

Andreas

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

[Andreas Grünbacher]

2016-12-06 0:19 GMT+01:00 Andreas Gr=C3=BCnbacher <andreas.gruenbacher@gmai=
l.com>:
> 2016-12-05 23:58 GMT+01:00 Patrick Plagwitz <Patrick_Plagwitz@web.de>:
>> On 12/05/2016 08:37 PM, Andreas Gr=C3=BCnbacher wrote:
>>> 2016-12-05 17:25 GMT+01:00 J. Bruce Fields <bfields@fieldses.org>:
>>>> On Mon, Dec 05, 2016 at 04:36:03PM +0100, Miklos Szeredi wrote:
>>>>> On Mon, Dec 5, 2016 at 4:19 PM, J. Bruce Fields <bfields@fieldses.org=
> wrote:
>>>>>>> Can NFS people comment on this?  Where does the nfs4_acl come from?
>>>>>>
>>>>>> This is the interface the NFS client provides for applications to mo=
dify
>>>>>> NFSv4 ACLs on servers that support them.
>>>>>
>>>>> Fine, but why are we seeing this xattr on exports where no xattrs are
>>>>> set on the exported fs?
>>>>
>>>> I don't know.  I took another look at the original patch and don't see
>>>> any details on the server setup: which server is it (knfsd, ganesha,
>>>> netapp, ...)?  How is it configured?
>>>>
>>>>>>> What can overlayfs do if it's a non-empty ACL?
>>>>>>
>>>>>> As little as possible.  You can't copy it up, can you?  So any attem=
pt
>>>>>> to support it is going to be incomplete.
>>>>>
>>>>> Right.
>>>>>
>>>>>>
>>>>>>> Does knfsd translate posix ACL into NFS acl?  If so, we can transla=
te
>>>>>>> back.  Should we do a generic POSIX<->NFS acl translator?
>>>>>>
>>>>>> knsd does translate between POSIX and NFSv4 ACLs.  It's a complicate=
d
>>>>>
>>>>> This does explain the nfs4_acl xattr on the client.  Question: if it'=
s
>>>>> empty, why have it at all?
>>>>
>>>> I'm honestly not sure what's going on there.  I'd be curious to see a
>>>> network trace if possible.
>>>
>>> I do see "system.nfs4_acl" attributes on knfsd exported filesystems
>>> that support POSIX ACLs (for ext4: "mount -o acl"). For exported
>>> filesystem that don't support POSIX ACLs (ext4: mount -o noacl), that
>>> attribute is missing. The attribute shouldn't be empty though; when
>>> the file has no real ACL, "system.nfs4_acl" represents the file mode
>>> permissions. The "system.nfs4_acl" attribute exposes the information
>>> on the wire; there is no resonable way to translate that into an ACL
>>> on another filesystem, really.
>>>
>>> Patrick, what does 'getfattr -m- -d /nfs/file' give you?
>>>
>> getfattr -m - -d nfs/folder -e text gives
>>
>> # file: nfs/folder/
>> system.nfs4_acl=3D"\000\000\000^C\000\000\000\000\000\000\000\000\000^V^=
A<E7>\000\000\000^FOWNER@\000\000\000\000\000\000\000\000\000\000\000^R\000=
<A1>\000\000\000^FGROUP@\000\000\000\000\000\000\000\000\000\000\000^R\000<=
A1>\000\000\000
>>     EVERYONE@\000\000"
>>
>> Those are 80 bytes. I checked again and vfs_getxattr indeed returns size=
=3D80.
>> It just looked empty because the first byte is 0... Ok, so nfs4_acl is n=
ot
>> empty after all and checking *value =3D=3D 0 does not tell if there are =
actually
>> ACLs present or not, sorry for the confusion.
>>
>> You are right, when I mount the exported fs with noacl the problem goes =
away.
>> You already helped me there, thanks.
>>
>> Still, I think there should be a way to copy up files that actually have=
 no
>> ACLs since acl is often the default for ext4 mounts and giving an "Opera=
tion
>> not supported" for random open(2)s is not a very good way to convey what=
's
>> going on.
>
> 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.

Andreas

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

[Miklos Szeredi]

On Tue, Dec 6, 2016 at 12:24 AM, Andreas Gr=C3=BCnbacher
<andreas.gruenbacher@gmail.com> wrote:
> 2016-12-06 0:19 GMT+01:00 Andreas Gr=C3=BCnbacher <andreas.gruenbacher@gm=
ail.com>:

>> 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.

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 =3D get_acl(inode, ACL_TYPE_ACCESS);
    if (!pacl)
        pacl =3D posix_acl_from_mode(inode->i_mode, GFP_KERNEL);

What's the point?

Thanks,
Miklos

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

[Andreas Gruenbacher]

On Tue, Dec 6, 2016 at 11:08 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
> <andreas.gruenbacher@gmail.com> wrote:
>> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <andreas.gruenbacher@gmail.com>:
>
>>> 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.
>
> 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. (I'm not saying that that's very helpful.)

Andreas

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

[J. Bruce Fields]

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@szeredi.hu> wrote:
> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
> > <andreas.gruenbacher@gmail.com> wrote:
> >> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <andreas.gruenbacher@gmail.com>:
> >
> >>> 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.
> >
> > 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

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3258 bytes --]

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@szeredi.hu> wrote:
>> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
>> > <andreas.gruenbacher@gmail.com> wrote:
>> >> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <andreas.gruenbacher@gmail.com>:
>> >
>> >>> 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.
>> >
>> > 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.  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.


 Miklos: would you find that acceptable?

Thanks,
NeilBrown

   

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Amir Goldstein]

On Wed, May 1, 2019 at 10:03 PM NeilBrown <neilb@suse.com> 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@szeredi.hu> wrote:
> >> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
> >> > <andreas.gruenbacher@gmail.com> wrote:
> >> >> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <andreas.gruenbacher@gmail.com>:
> >> >
> >> >>> 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.
> >> >
> >> > 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.

> 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?

Thanks,
Amir.

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4469 bytes --]

On Wed, May 01 2019, Amir Goldstein wrote:

> On Wed, May 1, 2019 at 10:03 PM NeilBrown <neilb@suse.com> 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@szeredi.hu> wrote:
>> >> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
>> >> > <andreas.gruenbacher@gmail.com> wrote:
>> >> >> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <andreas.gruenbacher@gmail.com>:
>> >> >
>> >> >>> 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.
>> >> >
>> >> > 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Andreas Gruenbacher]

On Thu, 2 May 2019 at 05:57, NeilBrown <neilb@suse.com> wrote:
> On Wed, May 01 2019, Amir Goldstein wrote:
> > On Wed, May 1, 2019 at 10:03 PM NeilBrown <neilb@suse.com> 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@szeredi.hu> wrote:
> >> >> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
> >> >> > <andreas.gruenbacher@gmail.com> wrote:
> >> >> >> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <andreas.gruenbacher@gmail.com>:
> >> >> >
> >> >> >>> 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.

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.

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

[Miklos Szeredi]

On Thu, May 2, 2019 at 10:05 AM Andreas Gruenbacher <agruenba@redhat.com> wrote:
>
> On Thu, 2 May 2019 at 05:57, NeilBrown <neilb@suse.com> wrote:
> > On Wed, May 01 2019, Amir Goldstein wrote:
> > > On Wed, May 1, 2019 at 10:03 PM NeilBrown <neilb@suse.com> 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@szeredi.hu> wrote:
> > >> >> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
> > >> >> > <andreas.gruenbacher@gmail.com> wrote:
> > >> >> >> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <andreas.gruenbacher@gmail.com>:
> > >> >> >
> > >> >> >>> 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?

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.

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

[Andreas Grünbacher]

Am Do., 2. Mai 2019 um 16:28 Uhr schrieb Miklos Szeredi <miklos@szeredi.hu>:
> On Thu, May 2, 2019 at 10:05 AM Andreas Gruenbacher <agruenba@redhat.com> wrote:
> > On Thu, 2 May 2019 at 05:57, NeilBrown <neilb@suse.com> wrote:
> > > On Wed, May 01 2019, Amir Goldstein wrote:
> > > > On Wed, May 1, 2019 at 10:03 PM NeilBrown <neilb@suse.com> 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@szeredi.hu> wrote:
> > > >> >> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
> > > >> >> > <andreas.gruenbacher@gmail.com> wrote:
> > > >> >> >> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <andreas.gruenbacher@gmail.com>:
> > > >> >> >
> > > >> >> >>> 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.

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

[J. Bruce Fields]

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.

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

[Andreas Gruenbacher]

On Thu, 2 May 2019 at 19:16, J. Bruce Fields <bfields@fieldses.org> 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.

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2379 bytes --]

On Thu, May 02 2019, Miklos Szeredi wrote:

> On Thu, May 2, 2019 at 10:05 AM Andreas Gruenbacher <agruenba@redhat.com> wrote:
>>
>> On Thu, 2 May 2019 at 05:57, NeilBrown <neilb@suse.com> wrote:
>> > On Wed, May 01 2019, Amir Goldstein wrote:
>> > > On Wed, May 1, 2019 at 10:03 PM NeilBrown <neilb@suse.com> 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@szeredi.hu> wrote:
>> > >> >> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
>> > >> >> > <andreas.gruenbacher@gmail.com> wrote:
>> > >> >> >> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <andreas.gruenbacher@gmail.com>:
>> > >> >> >
>> > >> >> >>> 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?

No.
"noacl" only affect NFSv3 (and maybe v2) and it disables use of the
NFSACL side-protocol.
"noacl" has no effect on an NFSv4 mount.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 6196 bytes --]

On Thu, May 02 2019, Andreas Gruenbacher wrote:

> On Thu, 2 May 2019 at 05:57, NeilBrown <neilb@suse.com> wrote:
>> On Wed, May 01 2019, Amir Goldstein wrote:
>> > On Wed, May 1, 2019 at 10:03 PM NeilBrown <neilb@suse.com> 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@szeredi.hu> wrote:
>> >> >> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
>> >> >> > <andreas.gruenbacher@gmail.com> wrote:
>> >> >> >> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <andreas.gruenbacher@gmail.com>:
>> >> >> >
>> >> >> >>> 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?

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.

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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@suse.com>:
> On Thu, May 02 2019, Andreas Gruenbacher wrote:
> > On Thu, 2 May 2019 at 05:57, NeilBrown <neilb@suse.com> wrote:
> >> On Wed, May 01 2019, Amir Goldstein wrote:
> >> > On Wed, May 1, 2019 at 10:03 PM NeilBrown <neilb@suse.com> 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@szeredi.hu> wrote:
> >> >> >> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
> >> >> >> > <andreas.gruenbacher@gmail.com> wrote:
> >> >> >> >> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <andreas.gruenbacher@gmail.com>:
> >> >> >> >
> >> >> >> >>> 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.

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

[Goetz, Patrick G]

On 5/1/19 10:57 PM, NeilBrown wrote:
> 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.


Is there some reason why there hasn't been a greater effort to add NFSv4 
ACL support to the mainstream linux filesystems?  I have to support a 
hybrid linux/windows environment and not having these ACLs on ext4 is a 
daily headache for me.

Also, it doesn't take much need for security granularity to realize that 
POSIX ACLs (not ever even formally standardized!) are fairly inadequate, 
but more importantly, don't play nicely with their Windows friends.

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

[Andreas Gruenbacher]

On Thu, 2 May 2019 at 19:27, Goetz, Patrick G <pgoetz@math.utexas.edu> wrote:
> On 5/1/19 10:57 PM, NeilBrown wrote:
> > 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.
>
> Is there some reason why there hasn't been a greater effort to add NFSv4
> ACL support to the mainstream linux filesystems?  I have to support a
> hybrid linux/windows environment and not having these ACLs on ext4 is a
> daily headache for me.

The patches for implementing that have been rejected over and over
again, and nobody is working on them anymore.

Andreas

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

[Goetz, Patrick G]

On 5/2/19 12:44 PM, Andreas Gruenbacher wrote:
> On Thu, 2 May 2019 at 19:27, Goetz, Patrick G <pgoetz@math.utexas.edu> wrote:
>> On 5/1/19 10:57 PM, NeilBrown wrote:
>>> 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.
>>
>> Is there some reason why there hasn't been a greater effort to add NFSv4
>> ACL support to the mainstream linux filesystems?  I have to support a
>> hybrid linux/windows environment and not having these ACLs on ext4 is a
>> daily headache for me.
> 
> The patches for implementing that have been rejected over and over
> again, and nobody is working on them anymore.
> 
> Andreas
> 


That's the part I don't understand -- why are the RichACL patches being 
rejected?

Everyone loves the simplicity of mode bits (including me) until you run 
into things like the need to automatically create home directories on an 
NFS-mounted filesystem or security situations where, for example, you 
want users to be able to edit but not delete files, and then you're kind 
of stuck listening to your Windows colleagues propose a Storage Spaces 
solution.

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

[J. Bruce Fields]

On Thu, May 02, 2019 at 05:51:12PM +0000, Goetz, Patrick G wrote:
> 
> 
> On 5/2/19 12:44 PM, Andreas Gruenbacher wrote:
> > On Thu, 2 May 2019 at 19:27, Goetz, Patrick G <pgoetz@math.utexas.edu> wrote:
> >> On 5/1/19 10:57 PM, NeilBrown wrote:
> >>> 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.
> >>
> >> Is there some reason why there hasn't been a greater effort to add NFSv4
> >> ACL support to the mainstream linux filesystems?  I have to support a
> >> hybrid linux/windows environment and not having these ACLs on ext4 is a
> >> daily headache for me.
> > 
> > The patches for implementing that have been rejected over and over
> > again, and nobody is working on them anymore.
> > 
> > Andreas
> 
> That's the part I don't understand -- why are the RichACL patches being 
> rejected?

Looking back through old mail....:

	http://lkml.kernel.org/r/20160311140134.GA14808@infradead.org

	For one I still see no reason to merge this broken ACL model at
	all.  It provides our actualy Linux users no benefit at all,
	while breaking a lot of assumptions, especially by adding allow
	and deny ACE at the same sime.

	It also doesn't help with the issue that the main thing it's
	trying to be compatible with (Windows) actually uses a
	fundamentally different identifier to apply the ACLs to - as
	long as you're still limited to users and groups and not guids
	we'll still have that mapping problem anyway.

Christoph also had some objections to the implementation which I think
were addressed, but I could be wrong.

--b.

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

[Goetz, Patrick G]

On 5/3/19 10:27 AM, J. Bruce Fields wrote:
> Christoph also had some objections to the implementation which I think
> were addressed, but I could be wrong.


I'm certainly no expert, but yes, the objections to the RichACL patches 
were addressed and not really counter challenged.  It seems like a case 
of Windows ACLs are yucky and complicated and not needed in all linux 
environments (which, by the way, I've learned isn't entirely true).


-----------------------------------------
Christoph Hellwig:
 > For one I still see no reason to merge this broken ACL model at all.
 > It provides our actualy Linux users no benefit at all, while breaking
 > a lot of assumptions, especially by adding allow and deny ACE at the
 > same sime.

From: Andreas Gruenbacher:
This permission model is useful in mixed environments that involve
UNIX and Windows machines. Think of NAS boxes with Linux and Windows
clients, for example. It also fits the NFSv4 ACL model very well. If
you're not a user dealing with such environments, then the model
likely won't provide any benefits to *you*, and you're better off with
a less complicated permission model. That doesn't say anything about
other users, though.
-----------------------------------------

and here:

-----------------------------------------
Christoph Hellwig:
 > It also doesn't help with the issue that the main thing it's trying
 > to be compatible with (Windows) actually uses a fundamentally
 > different identifier to apply the ACLs to - as long as you're
 > still limited to users and groups and not guids we'll still
 > have that mapping problem anyway.

From: Andreas Gruenbacher:
Samba has been dealing with mapping between SIDs and UIDs/GIDs for a
long time, and it's working acceptably well.

We could store SIDs in ACEs, but that wouldn't make the actual
problems go away: Files on Linux have an owner and an owning group
which are identitifed by UID/GID, whereas a file is owned by a SID
which can be either a user or a group in a SID world. Also, processes
on Linux have an owner and a list of groups which are identified by
UID/GID, so any SIDs stored in filesystems would never match a
process, anyway.

(NFSv4 refers to users and groups as opposed to SIDs, and so it
doesn't have this problem.)
-----------------------------------------

Further, the counterarguments seem weak, at best:

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

As someone pointed out elsewhere in the thread, linux people are 
hopefully used to learning new tricks.

Who these days has the luxury of not working in a mixed environment?  I 
agree that Windows ACLs are kind of gross and overly complicated (and as 
a result, many people don't use them at all and end up with low security 
permission environments), but the professional Windows admins I know do 
use them, and we've had a number of use cases already where permissions 
problems are simply solved using Windows ACLs, impossible to get just 
right with mode bits and POSIX ACLs.  Not having RichACLs just makes it 
really easy for the Windows storage people to win every argument.

The SID <--> UID thing is manageable; we're doing it now.  Dealing with 
groups is a bit harder.  What I've been doing is continuing to use 
/etc/group, but populating the entries with the usernames associated 
with SIDs (which in our case are mapped directly to UIDs).  For files 
owned by a group SID, the simple solution is just to treat this SID as a 
dummy UID.  I haven't had to use this yet, but it seems like it should 
work. sssd has made much of this more manageable.  The missing killer 
feature is a somewhat unified authorization model.

The fact that Windows assigns SIDs to machines is actually a major 
technological advantage over the UNIX/linux model (this sure would 
simplify NFS, for example), but then they had the advantages of 
hindsight.  And that's just an aside anyway.

[PATCH] OVL: add honoracl=off mount option.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 7121 bytes --]


If the upper and lower layers use incompatible ACL formats, it is not
possible to copy the ACL xttr from one to the other, so overlayfs
cannot work with them.
This happens particularly with NFSv4 which uses system.nfs4_acl, and
ext4 which uses system.posix_acl_access.

If all ACLs actually make to Unix permissions, then there is no need
to copy up the ACLs, but overlayfs cannot determine this.

So allow the sysadmin it assert that ACLs are not needed with a mount
option
  honoracl=off
This causes the ACLs to not be copied, so filesystems with different
ACL formats can be overlaid together.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 Documentation/filesystems/overlayfs.txt | 24 ++++++++++++++++++++++++
 fs/overlayfs/copy_up.c                  |  9 +++++++--
 fs/overlayfs/dir.c                      |  2 +-
 fs/overlayfs/overlayfs.h                |  2 +-
 fs/overlayfs/ovl_entry.h                |  1 +
 fs/overlayfs/super.c                    | 15 +++++++++++++++
 6 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
index eef7d9d259e8..7ad675940c93 100644
--- a/Documentation/filesystems/overlayfs.txt
+++ b/Documentation/filesystems/overlayfs.txt
@@ -245,6 +245,30 @@ filesystem - future operations on the file are barely noticed by the
 overlay filesystem (though an operation on the name of the file such as
 rename or unlink will of course be noticed and handled).
 
+ACL copy-up
+-----------
+
+When a file that only exists on the lower layer is modified it needs
+to be copied up to the upper layer.  This means copying the metadata
+and (usually) the data (though see "Metadata only copy up" below).
+One part of the metadata can be problematic: the ACLs.
+
+Now all filesystems support ACLs, and when they do they don't all use
+the same format.  A significant conflict appears between POSIX acls
+used on many local filesystems, and NFSv4 ACLs used with NFSv4.  There
+two formats are, in general, not inter-convertible.
+
+If a site only uses regular Unix permissions (Read, Write, eXecute by
+User, Group and Other), then as these permissions are compatible with
+all ACLs, there is no need to copy ACLs.  overlayfs cannot determine
+if this is the case itself.
+
+For this reason, overlayfs supports a mount option "honoracl=off"
+which causes ACLs, any "system." extended attribute, on the lower
+layer to be ignored and, particularly, not copied to the upper later.
+This allows NFSv4 to be overlaid with a local filesystem, but should
+only be used if the only access controls used on the filesystem are
+Unix permission bits.
 
 Multiple lower layers
 ---------------------
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 68b3303e4b46..032aa88f21c1 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -39,7 +39,7 @@ static int ovl_ccup_get(char *buf, const struct kernel_param *param)
 module_param_call(check_copy_up, ovl_ccup_set, ovl_ccup_get, NULL, 0644);
 MODULE_PARM_DESC(ovl_check_copy_up, "Obsolete; does nothing");
 
-int ovl_copy_xattr(struct dentry *old, struct dentry *new)
+int ovl_copy_xattr(struct dentry *old, struct dentry *new, struct ovl_fs *ofs)
 {
 	ssize_t list_size, size, value_size = 0;
 	char *buf, *name, *value = NULL;
@@ -77,6 +77,10 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
 		}
 		list_size -= slen;
 
+		if (strncmp(name, XATTR_SYSTEM_PREFIX, XATTR_SYSTEM_PREFIX_LEN) == 0 &&
+		    !ofs->config.honoracl)
+			continue;
+
 		if (ovl_is_private_xattr(name))
 			continue;
 retry:
@@ -461,7 +465,8 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 			return err;
 	}
 
-	err = ovl_copy_xattr(c->lowerpath.dentry, temp);
+	err = ovl_copy_xattr(c->lowerpath.dentry, temp,
+			     c->dentry->d_sb->s_fs_info);
 	if (err)
 		return err;
 
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 82c129bfe58d..cc8fb9eeb7df 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -368,7 +368,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 	if (IS_ERR(opaquedir))
 		goto out_unlock;
 
-	err = ovl_copy_xattr(upper, opaquedir);
+	err = ovl_copy_xattr(upper, opaquedir, upper->d_sb->s_fs_info);
 	if (err)
 		goto out_cleanup;
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 9c6018287d57..4a104a4732af 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -422,7 +422,7 @@ int ovl_copy_up(struct dentry *dentry);
 int ovl_copy_up_with_data(struct dentry *dentry);
 int ovl_copy_up_flags(struct dentry *dentry, int flags);
 int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags);
-int ovl_copy_xattr(struct dentry *old, struct dentry *new);
+int ovl_copy_xattr(struct dentry *old, struct dentry *new, struct ovl_fs *ofs);
 int ovl_set_attr(struct dentry *upper, struct kstat *stat);
 struct ovl_fh *ovl_encode_real_fh(struct dentry *real, bool is_upper);
 int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index ec237035333a..c541e3fed5b9 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -20,6 +20,7 @@ struct ovl_config {
 	bool nfs_export;
 	int xino;
 	bool metacopy;
+	bool honoracl;
 };
 
 struct ovl_sb {
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 0116735cc321..ceb8fdb7ce14 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -362,6 +362,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 	if (ofs->config.metacopy != ovl_metacopy_def)
 		seq_printf(m, ",metacopy=%s",
 			   ofs->config.metacopy ? "on" : "off");
+	if (!ofs->config.honoracl)
+		seq_puts(m, ",honoracl=off");
 	return 0;
 }
 
@@ -401,6 +403,8 @@ enum {
 	OPT_XINO_AUTO,
 	OPT_METACOPY_ON,
 	OPT_METACOPY_OFF,
+	OPT_HONORACL_ON,
+	OPT_HONORACL_OFF,
 	OPT_ERR,
 };
 
@@ -419,6 +423,8 @@ static const match_table_t ovl_tokens = {
 	{OPT_XINO_AUTO,			"xino=auto"},
 	{OPT_METACOPY_ON,		"metacopy=on"},
 	{OPT_METACOPY_OFF,		"metacopy=off"},
+	{OPT_HONORACL_ON,		"honoracl=on"},
+	{OPT_HONORACL_OFF,		"honoracl=off"},
 	{OPT_ERR,			NULL}
 };
 
@@ -557,6 +563,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			config->metacopy = false;
 			break;
 
+		case OPT_HONORACL_ON:
+			config->honoracl = true;
+			break;
+
+		case OPT_HONORACL_OFF:
+			config->honoracl = false;
+			break;
+
 		default:
 			pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
 			return -EINVAL;
@@ -1440,6 +1454,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	ofs->config.nfs_export = ovl_nfs_export_def;
 	ofs->config.xino = ovl_xino_def();
 	ofs->config.metacopy = ovl_metacopy_def;
+	ofs->config.honoracl = true;
 	err = ovl_parse_opt((char *) data, &ofs->config);
 	if (err)
 		goto out_err;
-- 
2.14.0.rc0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] OVL: add honoracl=off mount option.

[Randy Dunlap]

Hi Neil,

On 5/1/19 9:35 PM, NeilBrown wrote:
> 
> If the upper and lower layers use incompatible ACL formats, it is not
> possible to copy the ACL xttr from one to the other, so overlayfs

                           attr (?)

> cannot work with them.
> This happens particularly with NFSv4 which uses system.nfs4_acl, and
> ext4 which uses system.posix_acl_access.
> 
> If all ACLs actually make to Unix permissions, then there is no need

                       map (?)

> to copy up the ACLs, but overlayfs cannot determine this.
> 
> So allow the sysadmin it assert that ACLs are not needed with a mount
> option
>   honoracl=off
> This causes the ACLs to not be copied, so filesystems with different
> ACL formats can be overlaid together.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  Documentation/filesystems/overlayfs.txt | 24 ++++++++++++++++++++++++
>  fs/overlayfs/copy_up.c                  |  9 +++++++--
>  fs/overlayfs/dir.c                      |  2 +-
>  fs/overlayfs/overlayfs.h                |  2 +-
>  fs/overlayfs/ovl_entry.h                |  1 +
>  fs/overlayfs/super.c                    | 15 +++++++++++++++
>  6 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
> index eef7d9d259e8..7ad675940c93 100644
> --- a/Documentation/filesystems/overlayfs.txt
> +++ b/Documentation/filesystems/overlayfs.txt
> @@ -245,6 +245,30 @@ filesystem - future operations on the file are barely noticed by the
>  overlay filesystem (though an operation on the name of the file such as
>  rename or unlink will of course be noticed and handled).
>  
> +ACL copy-up
> +-----------
> +
> +When a file that only exists on the lower layer is modified it needs
> +to be copied up to the upper layer.  This means copying the metadata
> +and (usually) the data (though see "Metadata only copy up" below).
> +One part of the metadata can be problematic: the ACLs.
> +
> +Now all filesystems support ACLs, and when they do they don't all use

   Not

> +the same format.  A significant conflict appears between POSIX acls

                                                                  ACLs

> +used on many local filesystems, and NFSv4 ACLs used with NFSv4.  There

                                                                    These (or the)

> +two formats are, in general, not inter-convertible.
> +
> +If a site only uses regular Unix permissions (Read, Write, eXecute by
> +User, Group and Other), then as these permissions are compatible with
> +all ACLs, there is no need to copy ACLs.  overlayfs cannot determine
> +if this is the case itself.
> +
> +For this reason, overlayfs supports a mount option "honoracl=off"
> +which causes ACLs, any "system." extended attribute, on the lower
> +layer to be ignored and, particularly, not copied to the upper later.
> +This allows NFSv4 to be overlaid with a local filesystem, but should
> +only be used if the only access controls used on the filesystem are
> +Unix permission bits.
>  
>  Multiple lower layers
>  ---------------------



-- 
~Randy

Re: [PATCH] OVL: add honoracl=off mount option.

[Amir Goldstein]

On Thu, May 2, 2019 at 12:35 AM NeilBrown <neilb@suse.com> wrote:
>
>
> If the upper and lower layers use incompatible ACL formats, it is not
> possible to copy the ACL xttr from one to the other, so overlayfs
> cannot work with them.
> This happens particularly with NFSv4 which uses system.nfs4_acl, and
> ext4 which uses system.posix_acl_access.
>
> If all ACLs actually make to Unix permissions, then there is no need
> to copy up the ACLs, but overlayfs cannot determine this.
>
> So allow the sysadmin it assert that ACLs are not needed with a mount
> option
>   honoracl=off
> This causes the ACLs to not be copied, so filesystems with different
> ACL formats can be overlaid together.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  Documentation/filesystems/overlayfs.txt | 24 ++++++++++++++++++++++++
>  fs/overlayfs/copy_up.c                  |  9 +++++++--
>  fs/overlayfs/dir.c                      |  2 +-
>  fs/overlayfs/overlayfs.h                |  2 +-
>  fs/overlayfs/ovl_entry.h                |  1 +
>  fs/overlayfs/super.c                    | 15 +++++++++++++++
>  6 files changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
> index eef7d9d259e8..7ad675940c93 100644
> --- a/Documentation/filesystems/overlayfs.txt
> +++ b/Documentation/filesystems/overlayfs.txt
> @@ -245,6 +245,30 @@ filesystem - future operations on the file are barely noticed by the
>  overlay filesystem (though an operation on the name of the file such as
>  rename or unlink will of course be noticed and handled).
>
> +ACL copy-up
> +-----------
> +
> +When a file that only exists on the lower layer is modified it needs
> +to be copied up to the upper layer.  This means copying the metadata
> +and (usually) the data (though see "Metadata only copy up" below).
> +One part of the metadata can be problematic: the ACLs.
> +
> +Now all filesystems support ACLs, and when they do they don't all use
> +the same format.  A significant conflict appears between POSIX acls
> +used on many local filesystems, and NFSv4 ACLs used with NFSv4.  There
> +two formats are, in general, not inter-convertible.
> +
> +If a site only uses regular Unix permissions (Read, Write, eXecute by
> +User, Group and Other), then as these permissions are compatible with
> +all ACLs, there is no need to copy ACLs.  overlayfs cannot determine
> +if this is the case itself.
> +
> +For this reason, overlayfs supports a mount option "honoracl=off"
> +which causes ACLs, any "system." extended attribute, on the lower
> +layer to be ignored and, particularly, not copied to the upper later.
> +This allows NFSv4 to be overlaid with a local filesystem, but should
> +only be used if the only access controls used on the filesystem are
> +Unix permission bits.
>

I don't know. On the one hand "system." is not only ACLs.
On the other hand, "honoracl=off" is not the same as -o noacl,
but it sure sounds the same.

I'd be a lot more comfortable with "ignore_xattrs=system.nfs4_acl"
argument takes a comma separated list of xattr prefixes to ignore.

ovl_is_private_xattr() can be generalized to ovl_is_ignored_xattr(),
going over a blacklist of N>=1 which will also be called from
ovl_can_list(), because there is no point in listing the ACLs that
are ignored. right?

Thanks,
Amir.

Re: [PATCH] OVL: add honoracl=off mount option.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4925 bytes --]

On Thu, May 02 2019, Amir Goldstein wrote:

> On Thu, May 2, 2019 at 12:35 AM NeilBrown <neilb@suse.com> wrote:
>>
>>
>> If the upper and lower layers use incompatible ACL formats, it is not
>> possible to copy the ACL xttr from one to the other, so overlayfs
>> cannot work with them.
>> This happens particularly with NFSv4 which uses system.nfs4_acl, and
>> ext4 which uses system.posix_acl_access.
>>
>> If all ACLs actually make to Unix permissions, then there is no need
>> to copy up the ACLs, but overlayfs cannot determine this.
>>
>> So allow the sysadmin it assert that ACLs are not needed with a mount
>> option
>>   honoracl=off
>> This causes the ACLs to not be copied, so filesystems with different
>> ACL formats can be overlaid together.
>>
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>  Documentation/filesystems/overlayfs.txt | 24 ++++++++++++++++++++++++
>>  fs/overlayfs/copy_up.c                  |  9 +++++++--
>>  fs/overlayfs/dir.c                      |  2 +-
>>  fs/overlayfs/overlayfs.h                |  2 +-
>>  fs/overlayfs/ovl_entry.h                |  1 +
>>  fs/overlayfs/super.c                    | 15 +++++++++++++++
>>  6 files changed, 49 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
>> index eef7d9d259e8..7ad675940c93 100644
>> --- a/Documentation/filesystems/overlayfs.txt
>> +++ b/Documentation/filesystems/overlayfs.txt
>> @@ -245,6 +245,30 @@ filesystem - future operations on the file are barely noticed by the
>>  overlay filesystem (though an operation on the name of the file such as
>>  rename or unlink will of course be noticed and handled).
>>
>> +ACL copy-up
>> +-----------
>> +
>> +When a file that only exists on the lower layer is modified it needs
>> +to be copied up to the upper layer.  This means copying the metadata
>> +and (usually) the data (though see "Metadata only copy up" below).
>> +One part of the metadata can be problematic: the ACLs.
>> +
>> +Now all filesystems support ACLs, and when they do they don't all use
>> +the same format.  A significant conflict appears between POSIX acls
>> +used on many local filesystems, and NFSv4 ACLs used with NFSv4.  There
>> +two formats are, in general, not inter-convertible.
>> +
>> +If a site only uses regular Unix permissions (Read, Write, eXecute by
>> +User, Group and Other), then as these permissions are compatible with
>> +all ACLs, there is no need to copy ACLs.  overlayfs cannot determine
>> +if this is the case itself.
>> +
>> +For this reason, overlayfs supports a mount option "honoracl=off"
>> +which causes ACLs, any "system." extended attribute, on the lower
>> +layer to be ignored and, particularly, not copied to the upper later.
>> +This allows NFSv4 to be overlaid with a local filesystem, but should
>> +only be used if the only access controls used on the filesystem are
>> +Unix permission bits.
>>
>
> I don't know. On the one hand "system." is not only ACLs.

Isn't it?  What else goes in "system." "??

"man xattr" says:

   Extended system attributes
       Extended system attributes are used by the kernel to store system
       objects  such  as  Access Control Lists.  Read and write access
       permissions to system attributes depend on the policy implemented
       for each system attribute implemented by filesystems in the
       kernel.

so it *allows* things other than ACLs, but doesn't confirm that there
are any.

In the kernel source, "XATTR_SYSTEM_PREFIX" is only used with POSIX acls
and "system.sockprotoname" - which is socket specific and no likely to
be found on a filesystem.

"system.
also appears in
   CIFS_XATTR_CIFS_ACL
   SMB3_XATTR_CIFS_ACL
   F2FS_SYSTEM_ADVISE_NAME
   XATTR_NAME_NFSV4_ACL
   SYSTEM_ORANGEFS_KEY

which should all use XATTR_SYSTEM_PREFIX ...

So yes,  I guess they aren't (quite) all ACLs.  Bother.


> On the other hand, "honoracl=off" is not the same as -o noacl,
> but it sure sounds the same.
>
> I'd be a lot more comfortable with "ignore_xattrs=system.nfs4_acl"
> argument takes a comma separated list of xattr prefixes to ignore.

That requires the sysadmin to know a lot more about the internals of the
relevant filesystems.... Maybe that is a good idea, but it feels rather
clunky.

In each of these cases, except maybe POSIX_ACLs, it doesn't make sense
to copy-up the "system." xattr unless it is the exact same filesystem
type.

So if given a "noacl" flag (or similar), ignoring copy-up failure for
all "system." attributes is probably the right thing to do, as ACLs are
the only system. attribute for which it can make any sense at all to
copy them.

Thanks,
NeilBrown


>
> ovl_is_private_xattr() can be generalized to ovl_is_ignored_xattr(),
> going over a blacklist of N>=1 which will also be called from
> ovl_can_list(), because there is no point in listing the ACLs that
> are ignored. right?
>
> Thanks,
> Amir.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] OVL: add honoracl=off mount option.

[J. R. Okajima]

NeilBrown:
> If the upper and lower layers use incompatible ACL formats, it is not
> possible to copy the ACL xttr from one to the other, so overlayfs
> cannot work with them.
> This happens particularly with NFSv4 which uses system.nfs4_acl, and
> ext4 which uses system.posix_acl_access.

FYI,
Aufs had met the same problem many years ago, and I introduced some
options called ICEX (Ignore Copyup Error on Xattr).

(from the design doc in aufs)
----------------------------------------
For example, the src branch supports ACL but the dst branch doesn't
because the dst branch may natively un-support it or temporary
un-support it due to "noacl" mount option. Of course, the dst branch fs
may NOT return an error even if the XATTR is not supported. It is
totally up to the branch fs.

Anyway when the aufs internal copy-up gets an error from the dst branch
fs, then aufs tries removing the just copied entry and returns the error
to the userspace. The worst case of this situation will be all copy-up
will fail.

For the copy-up operation, there two basic approaches.
- copy the specified XATTR only (by category above), and return the
  error unconditionally if it happens.
- copy all XATTR, and ignore the error on the specified category only.

In order to support XATTR and to implement the correct behaviour, aufs
chooses the latter approach and introduces some new branch attributes,
"icexsec", "icexsys", "icextr", "icexusr", and "icexoth".
They correspond to the XATTR namespaces (see above). Additionally, to be
convenient, "icex" is also provided which means all "icex*" attributes
are set (here the word "icex" stands for "ignore copy-error on XATTR").

The meaning of these attributes is to ignore the error from setting
XATTR on that branch.
Note that aufs tries copying all XATTR unconditionally, and ignores the
error from the dst branch according to the specified attributes.
----------------------------------------

But recent nfs4 got changed its behaviour around ACL, and it didn't pass
my local tests.  I had posted about that, but got no reply.


J. R. Okajima

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

[J. Bruce Fields]

On Thu, May 02, 2019 at 12:02:33PM +1000, NeilBrown 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@szeredi.hu> wrote:
> >> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
> >> > <andreas.gruenbacher@gmail.com> wrote:
> >> >> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <andreas.gruenbacher@gmail.com>:
> >> >
> >> >>> 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.
> >> >
> >> > 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.  So if the
>  sysadmin wants this (and some clearly do), they need a way to
>  explicitly say "I accept the risk".

So, I feel like silently copying ACLs up *also* carries a risk, if that
means switching from server-enforcement to client-enforcement of those
permissions.

Sorry, I know we had another thread recently about permissions in this
situation, and I've forgotten the conclusion.

Out of curiosity, what's done with selinux labels?

--b.

> 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.
> 
> 
>  Miklos: would you find that acceptable?
> 
> Thanks,
> NeilBrown
> 
>    

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

[Amir Goldstein]

On Fri, May 3, 2019 at 12:03 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Thu, May 02, 2019 at 12:02:33PM +1000, NeilBrown 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@szeredi.hu> wrote:
> > >> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
> > >> > <andreas.gruenbacher@gmail.com> wrote:
> > >> >> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <andreas.gruenbacher@gmail.com>:
> > >> >
> > >> >>> 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.
> > >> >
> > >> > 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.  So if the
> >  sysadmin wants this (and some clearly do), they need a way to
> >  explicitly say "I accept the risk".
>
> So, I feel like silently copying ACLs up *also* carries a risk, if that
> means switching from server-enforcement to client-enforcement of those
> permissions.
>
> Sorry, I know we had another thread recently about permissions in this
> situation, and I've forgotten the conclusion.
>
> Out of curiosity, what's done with selinux labels?
>

overlayfs calls security_inode_copy_up_xattr(name) which
can fail (<0) allow (0) or skip(1).

selinux_inode_copy_up_xattr() as well as smack_inode_copy_up_xattr()
skip their own xattr on copy up and fail any other xattr copy up.

Thanks,
Amir.

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

[J. Bruce Fields]

On Fri, May 03, 2019 at 01:26:01PM -0400, Amir Goldstein wrote:
> On Fri, May 3, 2019 at 12:03 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Thu, May 02, 2019 at 12:02:33PM +1000, NeilBrown 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@szeredi.hu> wrote:
> > > >> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
> > > >> > <andreas.gruenbacher@gmail.com> wrote:
> > > >> >> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <andreas.gruenbacher@gmail.com>:
> > > >> >
> > > >> >>> 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.
> > > >> >
> > > >> > 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.  So if the
> > >  sysadmin wants this (and some clearly do), they need a way to
> > >  explicitly say "I accept the risk".
> >
> > So, I feel like silently copying ACLs up *also* carries a risk, if that
> > means switching from server-enforcement to client-enforcement of those
> > permissions.
> >
> > Sorry, I know we had another thread recently about permissions in this
> > situation, and I've forgotten the conclusion.
> >
> > Out of curiosity, what's done with selinux labels?
> >
> 
> overlayfs calls security_inode_copy_up_xattr(name) which
> can fail (<0) allow (0) or skip(1).
> 
> selinux_inode_copy_up_xattr() as well as smack_inode_copy_up_xattr()
> skip their own xattr on copy up and fail any other xattr copy up.

If it's OK to silently skip copying up security labels, maybe it's OK to
silently skip NFSv4 ACLs too?

--b.

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

[Amir Goldstein]

On Fri, May 3, 2019 at 1:31 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, May 03, 2019 at 01:26:01PM -0400, Amir Goldstein wrote:
> > On Fri, May 3, 2019 at 12:03 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> > >
> > > On Thu, May 02, 2019 at 12:02:33PM +1000, NeilBrown 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@szeredi.hu> wrote:
> > > > >> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
> > > > >> > <andreas.gruenbacher@gmail.com> wrote:
> > > > >> >> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <andreas.gruenbacher@gmail.com>:
> > > > >> >
> > > > >> >>> 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.
> > > > >> >
> > > > >> > 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.  So if the
> > > >  sysadmin wants this (and some clearly do), they need a way to
> > > >  explicitly say "I accept the risk".
> > >
> > > So, I feel like silently copying ACLs up *also* carries a risk, if that
> > > means switching from server-enforcement to client-enforcement of those
> > > permissions.
> > >
> > > Sorry, I know we had another thread recently about permissions in this
> > > situation, and I've forgotten the conclusion.
> > >
> > > Out of curiosity, what's done with selinux labels?
> > >
> >
> > overlayfs calls security_inode_copy_up_xattr(name) which
> > can fail (<0) allow (0) or skip(1).
> >
> > selinux_inode_copy_up_xattr() as well as smack_inode_copy_up_xattr()
> > skip their own xattr on copy up and fail any other xattr copy up.
>
> If it's OK to silently skip copying up security labels, maybe it's OK to
> silently skip NFSv4 ACLs too?
>

I think overlayfs inode security context is taken from overlayfs
mount parameters (i.e. per container context) and therefore
the lower security. xattr are ignored (CC Vivek).

Thanks,
Amir.

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

[Vivek Goyal]

On Fri, May 03, 2019 at 01:41:25PM -0400, Amir Goldstein wrote:
> On Fri, May 3, 2019 at 1:31 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Fri, May 03, 2019 at 01:26:01PM -0400, Amir Goldstein wrote:
> > > On Fri, May 3, 2019 at 12:03 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> > > >
> > > > On Thu, May 02, 2019 at 12:02:33PM +1000, NeilBrown 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@szeredi.hu> wrote:
> > > > > >> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
> > > > > >> > <andreas.gruenbacher@gmail.com> wrote:
> > > > > >> >> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <andreas.gruenbacher@gmail.com>:
> > > > > >> >
> > > > > >> >>> 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.
> > > > > >> >
> > > > > >> > 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.  So if the
> > > > >  sysadmin wants this (and some clearly do), they need a way to
> > > > >  explicitly say "I accept the risk".
> > > >
> > > > So, I feel like silently copying ACLs up *also* carries a risk, if that
> > > > means switching from server-enforcement to client-enforcement of those
> > > > permissions.
> > > >
> > > > Sorry, I know we had another thread recently about permissions in this
> > > > situation, and I've forgotten the conclusion.
> > > >
> > > > Out of curiosity, what's done with selinux labels?
> > > >
> > >
> > > overlayfs calls security_inode_copy_up_xattr(name) which
> > > can fail (<0) allow (0) or skip(1).
> > >
> > > selinux_inode_copy_up_xattr() as well as smack_inode_copy_up_xattr()
> > > skip their own xattr on copy up and fail any other xattr copy up.
> >
> > If it's OK to silently skip copying up security labels, maybe it's OK to
> > silently skip NFSv4 ACLs too?
> >
> 
> I think overlayfs inode security context is taken from overlayfs
> mount parameters (i.e. per container context) and therefore
> the lower security. xattr are ignored (CC Vivek).

If mount was done with "context=" option, then it is used otherwise
selinux security context comes from real inode xattr (lower/upper).

Thanks
Vivek

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3884 bytes --]

On Fri, May 03 2019, J. Bruce Fields wrote:

> On Thu, May 02, 2019 at 12:02:33PM +1000, NeilBrown 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@szeredi.hu> wrote:
>> >> > On Tue, Dec 6, 2016 at 12:24 AM, Andreas Grünbacher
>> >> > <andreas.gruenbacher@gmail.com> wrote:
>> >> >> 2016-12-06 0:19 GMT+01:00 Andreas Grünbacher <andreas.gruenbacher@gmail.com>:
>> >> >
>> >> >>> 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.
>> >> >
>> >> > 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.  So if the
>>  sysadmin wants this (and some clearly do), they need a way to
>>  explicitly say "I accept the risk".
>
> So, I feel like silently copying ACLs up *also* carries a risk, if that
> means switching from server-enforcement to client-enforcement of those
> permissions.

Interesting perspective .... though doesn't NFSv4 explicitly allow
client-side ACL enforcement in the case of delegations?
Not sure how relevant that is....

It seems to me we have two options:
 1/ declare the NFSv4 doesn't work as a lower layer for overlayfs and
    recommend people use NFSv3, or
 2/ Modify overlayfs to work with NFSv4 by ignoring nfsv4 ACLs either
 2a/ always - and ignore all other acls and probably all system. xattrs,
 or
 2b/ based on a mount option that might be
      2bi/ general "noacl" or might be
      2bii/ explicit "noxattr=system.nfs4acl"
 
I think that continuing to discuss the miniature of the options isn't
going to help.  No solution is perfect - we just need to clearly
document the implications of whatever we come up with.

I lean towards 2a, but I be happy with with any '2' and '1' won't kill
me.

Do we have a vote?  Or does someone make an executive decision??

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[J. Bruce Fields]

On Tue, May 07, 2019 at 10:24:58AM +1000, NeilBrown wrote:
> Interesting perspective .... though doesn't NFSv4 explicitly allow
> client-side ACL enforcement in the case of delegations?

Not really.  What you're probably thinking of is the single ACE that the
server can return on granting a delegation, that tells the client it can
skip the ACCESS check for users matching that ACE.  It's unclear how
useful that is.  It's currently unused by the Linux client and server.

> Not sure how relevant that is....
> 
> It seems to me we have two options:
>  1/ declare the NFSv4 doesn't work as a lower layer for overlayfs and
>     recommend people use NFSv3, or
>  2/ Modify overlayfs to work with NFSv4 by ignoring nfsv4 ACLs either
>  2a/ always - and ignore all other acls and probably all system. xattrs,
>  or
>  2b/ based on a mount option that might be
>       2bi/ general "noacl" or might be
>       2bii/ explicit "noxattr=system.nfs4acl"
>  
> I think that continuing to discuss the miniature of the options isn't
> going to help.  No solution is perfect - we just need to clearly
> document the implications of whatever we come up with.
> 
> I lean towards 2a, but I be happy with with any '2' and '1' won't kill
> me.

I guess I'd also lean towards 2a.

I don't think it applies to posix acls, as overlayfs is capable of
copying those up and evaluating them on its own.

--b.

> 
> Do we have a vote?  Or does someone make an executive decision??
> 
> NeilBrown

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

[Miklos Szeredi]

On Fri, May 10, 2019 at 04:09:41PM -0400, J. Bruce Fields wrote:
> On Tue, May 07, 2019 at 10:24:58AM +1000, NeilBrown wrote:
> > Interesting perspective .... though doesn't NFSv4 explicitly allow
> > client-side ACL enforcement in the case of delegations?
> 
> Not really.  What you're probably thinking of is the single ACE that the
> server can return on granting a delegation, that tells the client it can
> skip the ACCESS check for users matching that ACE.  It's unclear how
> useful that is.  It's currently unused by the Linux client and server.
> 
> > Not sure how relevant that is....
> > 
> > It seems to me we have two options:
> >  1/ declare the NFSv4 doesn't work as a lower layer for overlayfs and
> >     recommend people use NFSv3, or
> >  2/ Modify overlayfs to work with NFSv4 by ignoring nfsv4 ACLs either
> >  2a/ always - and ignore all other acls and probably all system. xattrs,
> >  or
> >  2b/ based on a mount option that might be
> >       2bi/ general "noacl" or might be
> >       2bii/ explicit "noxattr=system.nfs4acl"
> >  
> > I think that continuing to discuss the miniature of the options isn't
> > going to help.  No solution is perfect - we just need to clearly
> > document the implications of whatever we come up with.
> > 
> > I lean towards 2a, but I be happy with with any '2' and '1' won't kill
> > me.
> 
> I guess I'd also lean towards 2a.
> 
> I don't think it applies to posix acls, as overlayfs is capable of
> copying those up and evaluating them on its own.

POSIX acls are evaluated and copied up.

I guess same goes for "security.*" attributes, that are evaluated on MAC checks.

I think it would be safe to ignore failure to copy up anything else.  That seems
a bit saner than just blacklisting nfs4_acl...

Something like the following untested patch.

Thanks,
Miklos

---
 fs/overlayfs/copy_up.c |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -36,6 +36,13 @@ static int ovl_ccup_get(char *buf, const
 module_param_call(check_copy_up, ovl_ccup_set, ovl_ccup_get, NULL, 0644);
 MODULE_PARM_DESC(check_copy_up, "Obsolete; does nothing");
 
+static bool ovl_must_copy_xattr(const char *name)
+{
+	return !strcmp(name, XATTR_POSIX_ACL_ACCESS) ||
+	       !strcmp(name, XATTR_POSIX_ACL_DEFAULT) ||
+	       !strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN);
+}
+
 int ovl_copy_xattr(struct dentry *old, struct dentry *new)
 {
 	ssize_t list_size, size, value_size = 0;
@@ -107,8 +114,13 @@ int ovl_copy_xattr(struct dentry *old, s
 			continue; /* Discard */
 		}
 		error = vfs_setxattr(new, name, value, size, 0);
-		if (error)
-			break;
+		if (error) {
+			if (ovl_must_copy_xattr(name))
+				break;
+
+			/* Ignore failure to copy unknown xattrs */
+			error = 0;
+		}
 	}
 	kfree(value);
 out:

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

[J. Bruce Fields]

On Wed, Sep 18, 2019 at 11:07:31AM +0200, Miklos Szeredi wrote:
> On Fri, May 10, 2019 at 04:09:41PM -0400, J. Bruce Fields wrote:
> > On Tue, May 07, 2019 at 10:24:58AM +1000, NeilBrown wrote:
> > > Interesting perspective .... though doesn't NFSv4 explicitly allow
> > > client-side ACL enforcement in the case of delegations?
> > 
> > Not really.  What you're probably thinking of is the single ACE that the
> > server can return on granting a delegation, that tells the client it can
> > skip the ACCESS check for users matching that ACE.  It's unclear how
> > useful that is.  It's currently unused by the Linux client and server.
> > 
> > > Not sure how relevant that is....
> > > 
> > > It seems to me we have two options:
> > >  1/ declare the NFSv4 doesn't work as a lower layer for overlayfs and
> > >     recommend people use NFSv3, or
> > >  2/ Modify overlayfs to work with NFSv4 by ignoring nfsv4 ACLs either
> > >  2a/ always - and ignore all other acls and probably all system. xattrs,
> > >  or
> > >  2b/ based on a mount option that might be
> > >       2bi/ general "noacl" or might be
> > >       2bii/ explicit "noxattr=system.nfs4acl"
> > >  
> > > I think that continuing to discuss the miniature of the options isn't
> > > going to help.  No solution is perfect - we just need to clearly
> > > document the implications of whatever we come up with.
> > > 
> > > I lean towards 2a, but I be happy with with any '2' and '1' won't kill
> > > me.
> > 
> > I guess I'd also lean towards 2a.
> > 
> > I don't think it applies to posix acls, as overlayfs is capable of
> > copying those up and evaluating them on its own.
> 
> POSIX acls are evaluated and copied up.
> 
> I guess same goes for "security.*" attributes, that are evaluated on MAC checks.
> 
> I think it would be safe to ignore failure to copy up anything else.  That seems
> a bit saner than just blacklisting nfs4_acl...
> 
> Something like the following untested patch.

It seems at least simple to implement and explain.

>  fs/overlayfs/copy_up.c |   16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -36,6 +36,13 @@ static int ovl_ccup_get(char *buf, const
>  module_param_call(check_copy_up, ovl_ccup_set, ovl_ccup_get, NULL, 0644);
>  MODULE_PARM_DESC(check_copy_up, "Obsolete; does nothing");
>  
> +static bool ovl_must_copy_xattr(const char *name)
> +{
> +	return !strcmp(name, XATTR_POSIX_ACL_ACCESS) ||
> +	       !strcmp(name, XATTR_POSIX_ACL_DEFAULT) ||
> +	       !strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN);
> +}
> +
>  int ovl_copy_xattr(struct dentry *old, struct dentry *new)
>  {
>  	ssize_t list_size, size, value_size = 0;
> @@ -107,8 +114,13 @@ int ovl_copy_xattr(struct dentry *old, s
>  			continue; /* Discard */
>  		}
>  		error = vfs_setxattr(new, name, value, size, 0);
> -		if (error)
> -			break;
> +		if (error) {

Can we check for EOPNOTSUPP instead of any error?

Maybe we're copying up a user xattr to a filesystem that's perfectly
capable of supporting those.  And maybe there's a disk error or we run
out of disk space or something.  Then I'd rather get EIO or ENOSPC than
silently fail to copy some xattrs.

--b.

> +			if (ovl_must_copy_xattr(name))
> +				break;
> +
> +			/* Ignore failure to copy unknown xattrs */
> +			error = 0;
> +		}
>  	}
>  	kfree(value);
>  out:

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

[Miklos Szeredi]

On Fri, May 3, 2019 at 11:35 AM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Thu, May 02, 2019 at 12:02:33PM +1000, NeilBrown wrote:

> >  Silently not copying the ACLs is probably not a good idea as it might
> >  result in inappropriate permissions being given away.  So if the
> >  sysadmin wants this (and some clearly do), they need a way to
> >  explicitly say "I accept the risk".
>
> So, I feel like silently copying ACLs up *also* carries a risk, if that
> means switching from server-enforcement to client-enforcement of those
> permissions.

That's not correct: permissions are checked on the overlay layer,
regardless of where the actual file resides.  For filesystems using a
server enforced permission model that means possibly different
permissions for accesses through overlayfs than for accesses without
overlayfs.  Apparently this is missing from the documentation and
definitely needs to be added.

So I think it's perfectly fine to allow copying up ACLs, as long as
the ACL is representable on the upper fs.  If that cannot be ensured,
then the only sane thing to do is to disable ACL checking across the
overlay ("noacl" option).

Thanks,
Miklos

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

[J. Bruce Fields]

On Tue, May 07, 2019 at 04:07:21AM -0400, Miklos Szeredi wrote:
> On Fri, May 3, 2019 at 11:35 AM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Thu, May 02, 2019 at 12:02:33PM +1000, NeilBrown wrote:
> 
> > >  Silently not copying the ACLs is probably not a good idea as it might
> > >  result in inappropriate permissions being given away.  So if the
> > >  sysadmin wants this (and some clearly do), they need a way to
> > >  explicitly say "I accept the risk".
> >
> > So, I feel like silently copying ACLs up *also* carries a risk, if that
> > means switching from server-enforcement to client-enforcement of those
> > permissions.
> 
> That's not correct: permissions are checked on the overlay layer,
> regardless of where the actual file resides.  For filesystems using a
> server enforced permission model that means possibly different
> permissions for accesses through overlayfs than for accesses without
> overlayfs.  Apparently this is missing from the documentation and
> definitely needs to be added.

Well, we did have a thread on this pretty recently, I think, and I'm
just not remembering the conclusion.  Yes, it'd be nice to have this
documented.

In the case of NFSv4 ACLs, we not only lack storage for them, we don't
even have code to evaluate them.

--b.

> So I think it's perfectly fine to allow copying up ACLs, as long as
> the ACL is representable on the upper fs.  If that cannot be ensured,
> then the only sane thing to do is to disable ACL checking across the
> overlay ("noacl" option).