Re: [PATCH] fix NFSv4 acl detection on F39

[Paul Eggert]

On 2023-05-15 04:50, Ondrej Valousek wrote:
> 1. The calculation of the 'listbufsize' is incorrect in your patch. It will 
> _not_work as you expected and won't limit the number of syscalls (which is why 
> we came up with this patch, right?). Check with my original proposal, we really 
> need to check for 'system.nfs4' xattr name presence here

What's wrong with the Gnulib file-has-acl.c calculation? It does look at 
the system.nfs4_acl xattr - is there some "system.nfs4" xattr I don't 
know about? I don't see any "system.nfs4" xattr in your original 
proposal <https://lists.gnu.org/r/bug-gnulib/2023-05/msg00006.html>.

Anyway the main goal here was to limit the typical number of syscalls to 
one (the listxattr call) in the typical case where there are no ACLs. I 
do see now that if an adversary is fiddling with ACLs while file_has_acl 
is running, file_has_acl could in theory loop forever. I fixed that as 
described below. I doubt whether that is what you were referring to, 
though, so if you could clarify what you meant by "will _not_ work as 
you expected" please let us know.

> 2. It mistakenly detects an ACL presence on files which do not have any ACL on 
> NFSv4 filesystem. Digging further it seems that kernel in F39 behaves 
> differently to the previous kernels:
>
> F38:
> # getfattr -m . /path_to_nfs4_file
> # file: path_to_nfs4_file
> system.nfs4_acl                                    <---- only single xattr 
> detected
>
> F39:
> # getfattr -m . /path_to_nfs4_file
> # file: path_to_nfs4_file
> system.nfs4_acl
> system.posix_acl_default
> /* SOMETIMES even shows this */
> system.posix_acl_default

When you write "SOMETIMES even shows this" do you mean that there are 
two instances of "system.posix_acl_default" in the output of getfattr 
-m? (Surely that would be a bug.) Or is there a typo there, and the 
first instance of "system.posix_acl_default" is actually 
"system.posix_acl_access"? If that is indeed a typo, the patch described 
below should fix things.

By the way, where is this documented? If it's not documented, could you 
please file a bug report with whoever's in charge of it, so that it gets 
documented? As things stand I feel like we're making coreutils changes 
somewhat in the dark. For example, I don't know why the macro 
XATTR_NAME_NFSV4_ACL (defined in linux/fs/nfs/nfs4proc.c) is not made 
visible to users in /usr/include/linux/xattr.h.


> I faintly recall there was an activity in to move POSIX acls calculation from 
> userspace to kernel (now Jeff in CC will hopefully clarify this), but it seems 
> to me that presence of the system.posix* attributes no longer signals the 
> presence of the actual ACLs, so our code thinks that POSIX acls are present 
> instead (which makes no sense on NFSv4).

If I understand you correctly, we should ignore the presense of POSIX 
ACLs if there are NFSv4 ACLs present. I installed the attached patch 
into Gnulib to implement this. This patch also fixes the DoS issue 
mentioned above, and improves performance slightly when some syscalls 
return nonpositive values.

If this patch isn't right, please let me know what's wrong with it. Thanks.

0001-file-has-acl-improve-port-to-Fedora-39.patch
Description: Text Data