Bug 2160675 - [RFE] cp -p should copy NFSv4 acls
Summary: [RFE] cp -p should copy NFSv4 acls
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: coreutils
Version: 37
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Lukáš Zaoral
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2023-01-13 09:36 UTC by Ondrej
Modified: 2023-09-21 00:15 UTC (History)
10 users (show)

Fixed In Version: coreutils-9.3-4.fc39
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2023-09-21 00:15:45 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)

Description Ondrej 2023-01-13 09:36:16 UTC
Description of problem:

Kamile, kindly please merge latest changes to Gnulib that implement the changes requested, see the recent commit:

commit eb6a8a4dfbe89f9f74666df082255cfefa10a407
Author: Ondrej Valousek <ondrej.valousek.xm>
Date:   Wed Jan 4 15:34:26 2023 +0100

    qcopy-acl: Optimize copying of ACLs by directly copying the attributes.
    
    * lib/qcopy-acl.c (is_attr_permissions): New functions.
    (qcopy_acl): If USE_XATTR, copy the ACL related attributes directly.
    * m4/xattr.m4: New file.
    * modules/qcopy-acl (Files): Add it.
    (configure.ac): Invoke gl_FUNC_XATTR.

which implements the requested functionality.
Note, you'd also need to revert the changes you made to /etc/xattr.conf to make this work.

Many thanks

Comment 2 Kamil Dudka 2023-01-13 09:53:05 UTC
Thank you for working on it upstream!  Unfortunately, I do not think that the proposed changes are appropriate for an already released major version of RHEL.  There might be deployments out there that rely on the current behavior.  One can copy NFSv4 ACLs with `cp --preserve=xattr ...` on RHEL-9.  The solution you propose should rather be introduced in RHEL-10.

Comment 3 Ondrej 2023-01-13 09:57:02 UTC
I see, bit unfortunate, but understandable. What about Fedora 37/38 at least?

Comment 5 Ondrej 2023-01-13 10:24:49 UTC
also, I see no changes to xattr.conf would be needed for RHEL-8, so perhaps chance to backport there?

Comment 6 Kamil Dudka 2023-01-13 10:39:05 UTC
Not really.  We should not introduce new features into RHEL-8 if they are not available in RHEL-9, because customers would see regressions after a RHEL-8 -> RHEL-9 upgrade.  Moreover, the changed behavior of `cp -p` in a minor update could be a problem on its own.

Putting the changes to Fedora now is a good idea.  This should give us some feedback before it goes to RHEL-10.

Comment 7 Kamil Dudka 2023-01-19 14:45:22 UTC
(In reply to Ondrej from comment #0)
> Note, you'd also need to revert the changes you made to /etc/xattr.conf to make this work.

Why do we actually need to make any changes to xattr.conf?

`cp -p` does not seem to read the file anyway.  I think one needs to use `cp --preserve=xattr`, `cp -a`, or such to copy extended attributes.

Comment 8 Ondrej 2023-01-19 16:53:25 UTC
With the patch I made upstream, we do read xattr.conf in file qcopy-acl.c in Gnulib.
Hence, if you need the "cp -p" to copy Posix ACLs as well as NFSv4 ACLs, the xattr.conf needs to be restored to default (i.e. must contain "system.nfs4_acl").
I think it's just more appropriate - people do expect cp -p would copy ACLs, no matter which, right?

Comment 9 Ondrej 2023-01-19 17:04:28 UTC
Actually that reminded me of another bug - "man cp" says nothing about the fact that cp --preserve=mode also preserves Posix ACLs (and with this patch applied, we also preserve NFSv4 acls).
I think it's fair to have a note about this behavior in the man page.

Comment 10 Kamil Dudka 2023-01-20 09:10:59 UTC
Thanks for explanation!  The cp(1) man page is generated from the output of `cp --help`.  So we need to patch src/cp.c in coreutils to change it.

Comment 11 Kamil Dudka 2023-04-27 12:55:07 UTC
The upstream changes are now included in coreutils-9.2-1.fc39 and newer.

Comment 12 Ondrej 2023-04-28 12:57:36 UTC
Hmm, unfortunately it does not work for me.
Just upgraded freshly to F39, coreutils-9.3-1.fc39.x86_64
and "ls -l" does not detect NFSv4 ACLs any longer

Comment 13 Ondrej 2023-04-28 13:53:04 UTC
But cp -p appears to handle NFSv4 ACLs just fine. So there must be smth wrong with the file-has-acl.c...?

Comment 15 Ondrej 2023-04-28 20:55:49 UTC
Well the problem is actually in getxattr() call which no longer returns ENOTSUP on Fedora39 as it did on previous systems.
I have sent a patch to Gnulib with a workaround, but I am not that excited with the patch as it yields into one more getxattr() call -> the function will be bit slower.

Comment 16 Kamil Dudka 2023-05-16 12:00:10 UTC
There is an ongoing discussion upstream: https://lists.gnu.org/archive/html/bug-gnulib/2023-05/msg00092.html

Comment 17 Lukáš Zaoral 2023-08-28 09:01:24 UTC
I'll wait for the imminent new release of coreutils and update it in rawhide.  Then, I'll backport the related gnulib changes to Fedora 39 to fix this issue.

Comment 18 Holger Hoffstätte 2023-08-31 08:39:26 UTC
Hi, as we have been tracking the same bug in Gentoo I just wanted to chime in and say that this does not seem to be fixed in the recently released coreutils-9.4 running kernel 6.4.x (both NFS server & client); I get the same error as with e.g. 9.3. Sorry for the terse message but that is all the info I have right now.

Comment 19 Holger Hoffstätte 2023-08-31 10:32:18 UTC
So apparently 9.4 is still broken because the patch from https://debbugs.gnu.org/cgi/bugreport.cgi?bug=63212 was not applied. Makes sense :). The good news is that 9.3 with that patch seems to fix the bug and work fine (lightly tested only).
Unfortunately it does not apply at all to 9.4, but I guess that's a different problem.

Comment 20 Holger Hoffstätte 2023-08-31 10:38:55 UTC
(In reply to Holger Hoffstätte from comment #19)
> The good news is that 9.3 with that patch seems to fix the bug and work fine (lightly tested only).

Apologies, I spoke too soon: even with that patch cp -a still complains.

Comment 21 Ondrej 2023-08-31 11:14:59 UTC
Note: the patch you mentioned has been superseded by (much for more effective one) patch that Paul submitted to git eventually.
So I suggest you try to compile the latest coreutils version from git.

Comment 22 Pádraig Brady 2023-08-31 14:36:09 UTC
Note coreutils 9.4 was only just released, and so probably contains the latest patches from Paul

Comment 23 Holger Hoffstätte 2023-08-31 15:10:16 UTC
I probably should have mentioned in #20 that I already tried 9.4 and it does not fix the problem; cp -a from NFS still shows the warning.

Comment 24 Ondrej 2023-08-31 17:10:31 UTC
What is your issue actually? Note that subject of this BZ is to preserve NFSv4 ACLs on filesystems that can preserve them - which, on Linux, is again only NFSv4 filesystem.

Comment 25 Holger Hoffstätte 2023-08-31 18:06:40 UTC
(In reply to Ondrej from comment #24)
> What is your issue actually? Note that subject of this BZ is to preserve
> NFSv4 ACLs on filesystems that can preserve them - which, on Linux, is again
> only NFSv4 filesystem.

Thanks Ondrej - the issue is that I've been led here after a series of
unfortunate miscommunications (half-baked error report leading to confusing
email threads..). Your comment made it clear that the warning I'm getting when
copying from NFS to ext4/xfs (which seemed to appear in 9.2?) is expected and
correct. I can confirm that this warning goes away with 9.4 and when copying
fom NFS to another NFS mount, which is the expected behaviour.

I've now edited /etc/xattr.conf to skip system.nfsv4acl and indeed no longer
get a warning when copying to a non-NFS destination either.
Not sure if that is the best way but at least it's the least confusing.

Comment 26 Ondrej 2023-09-01 09:06:44 UTC
> I've now edited /etc/xattr.conf to skip system.nfsv4acl and indeed no longer
> get a warning when copying to a non-NFS destination either.
> Not sure if that is the best way but at least it's the least confusing.

No, I would not say it's the best approach. But as you mention it, I think we have a (possible) bug there in qcopy-acl.c.
Thing is that cp -rp should not produce any warning when copying from NFS in case there is just a trivial ACLs on the file - which is most probably your case.
We could possibly reuse our new & shiny file-has-acl() function for that.

I do not know - Gnulib mantainers - any idea here?

Comment 27 Lukáš Zaoral 2023-09-11 13:08:27 UTC
Pagure PR: https://src.fedoraproject.org/rpms/coreutils/pull-request/13

Ondrej, could you please check that the linked PR fixes the issue for you on Fedora 39?  Thanks!

Comment 28 Ondrej 2023-09-12 07:43:06 UTC
Seems to work OK as per Fedora release 40 (Rawhide) & 
Name        : coreutils
Version     : 9.3
Release     : 2.fc39

There is an ongoing problem with "ls -l" not displaying NFSv4 ACLs but I guess it will be resolved once bug #2237410 is fixed

Comment 29 Fedora Update System 2023-09-12 08:20:23 UTC
FEDORA-2023-a4b80be287 has been submitted as an update to Fedora 39. https://bodhi.fedoraproject.org/updates/FEDORA-2023-a4b80be287

Comment 30 Fedora Update System 2023-09-12 21:38:29 UTC
FEDORA-2023-a4b80be287 has been pushed to the Fedora 39 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2023-a4b80be287`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2023-a4b80be287

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 31 Fedora Update System 2023-09-21 00:15:45 UTC
FEDORA-2023-a4b80be287 has been pushed to the Fedora 39 stable repository.
If problem still persists, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.