Subject: bug#33644: [PATCH] cp --preserve=xattr: preserve NFSv4 ACL extended attributes

From: Pádraig Brady <[email-masked]>
Date: Mon, 11 Feb 2019 10:30:42 -0800

On 11/02/19 03:50, Kamil Dudka wrote:
> On Monday, February 11, 2019 6:07:18 AM CET Pádraig Brady wrote:
>> On 06/12/18 05:08, Kamil Dudka wrote:
>>> ... which cannot be preserved by other means
>>>
>>> Bug: https://bugzilla.redhat.com/1031423#c4
>>> ---
>>>
>>>  src/copy.c | 22 +++++++++++++++++-----
>>>  1 file changed, 17 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/copy.c b/src/copy.c
>>> index 3221b9997..754c5e1aa 100644
>>> --- a/src/copy.c
>>> +++ b/src/copy.c
>>> @@ -640,6 +640,17 @@ copy_attr_free (struct error_context *ctx _GL_UNUSED,
>>>
>>>  {
>>>  }
>>>
>>> +/* Include NFSv4 ACL extended attributes, which cannot be preserved by
>>> +   other means.  Otherwise honor attributes configured for exclusion
>>> +   in /etc/xattr.conf.  Return zero to skip.  */
>>> +static int
>>> +check_not_nfs4_acl (const char *name, struct error_context *ctx)
>>> +{
>>> +  return attr_copy_check_permissions(name, ctx)
>>> +         || !STRNCMP_LIT (name, "system.nfs4_acl")
>>> +         || !STRNCMP_LIT (name, "system.nfs4acl");
>>> +}
>>> +
>>>
>>>  /* Exclude SELinux extended attributes that are otherwise handled,
>>>  
>>>     and are problematic to copy again.  Also honor attributes
>>>     configured for exclusion in /etc/xattr.conf.
>>>
>>> @@ -649,7 +660,7 @@ static int
>>>
>>>  check_selinux_attr (const char *name, struct error_context *ctx)
>>>  {
>>>  
>>>    return STRNCMP_LIT (name, "security.selinux")
>>>
>>> -         && attr_copy_check_permissions (name, ctx);
>>> +         && check_not_nfs4_acl (name, ctx);
>>>
>>>  }
>>>  
>>>  /* If positive SRC_FD and DST_FD descriptors are passed,
>>>
>>> @@ -663,6 +674,9 @@ copy_attr (char const *src_path, int src_fd,
>>>
>>>    bool all_errors = (!x->data_copy_required ||
>>>    x->require_preserve_xattr);
>>>    bool some_errors = (!all_errors && !x->reduce_diagnostics);
>>>    bool selinux_done = (x->preserve_security_context ||
>>>    x->set_security_context);> 
>>> +  int (*check) (const char *, struct error_context *) = (selinux_done)
>>> +    ? check_selinux_attr
>>> +    : check_not_nfs4_acl;
>>>
>>>    struct error_context ctx =
>>>    {
>>>    
>>>      .error = all_errors ? copy_attr_allerror : copy_attr_error,
>>>
>>> @@ -670,12 +684,10 @@ copy_attr (char const *src_path, int src_fd,
>>>
>>>      .quote_free = copy_attr_free
>>>    
>>>    };
>>>    if (0 <= src_fd && 0 <= dst_fd)
>>>
>>> -    ret = attr_copy_fd (src_path, src_fd, dst_path, dst_fd,
>>> -                        selinux_done ? check_selinux_attr : NULL,
>>> +    ret = attr_copy_fd (src_path, src_fd, dst_path, dst_fd, check,
>>>
>>>                          (all_errors || some_errors ? &ctx : NULL));
>>>    
>>>    else
>>>
>>> -    ret = attr_copy_file (src_path, dst_path,
>>> -                          selinux_done ? check_selinux_attr : NULL,
>>> +    ret = attr_copy_file (src_path, dst_path, check,
>>>
>>>                            (all_errors || some_errors ? &ctx : NULL));
>>>    
>>>    return ret == 0;
>>
>> This patch is confusing to read, though looks functional.
> 
> I can submit deduplication of the `selinux_done ? check_selinux_attr : NULL` 
> code as a separate patch if you prefer it.
> 
>> It's clearer of you rename check_not_nfs4_acl() to
>> check_but_allow_nfs4_acl().
> 
> Fine by me.
> 
>> So in summary, any xattr in /etc/xattr.conf is _not_ copied.
>> You want to essentially ignore the nfs4 entries in that config file.
>> So why not just remove the entries from that file?
> 
> See how xattr.conf is documented:
> 
>     # Actions:
>     #   permissions - copy when trying to preserve permissions.
>     #   skip - do not copy.
> 
> The fact that coreutils handles `persmissions` equally as `skip` is IMO a 
> problem of coreutils, not a problem of xattr.conf.
> 
>> Is that something that could be done in attr.git?
> 
> I think that the information in xattr.conf is correct.  system.nfs4_acl is 
> really an attribute one wants to copy when trying to preserve permissions.

Right. What I was getting at was attr_copy_file() from libattr seems
to skip all entries in xattr.conf by default. I need to dig in to
see what's preserving system.posix_acl_access (these might be
implicitly generated upon attr reading for example).
My question was why does coreutils need to explicitly handle
the nfs4 acls if it doesn't need to handle the posix ones.

I'm not saying the patch is wrong at all,
I'm just not seeing the full picture.

> 
>> Why would one want to treat nfs4 attrs differently to the posix_acl_access
>> attrs?
> 
> It was written in the commit message.  One can use `cp --preserve=mode`
> to preserve POSIX ACLs whereas the only way to preserve NFSv4 ACLs was
> `cp --preserve=xattr`.
> 
>> thanks,
>> Pádraig.
> 
> On Monday, February 11, 2019 6:21:49 AM CET Pádraig Brady wrote:
>> BTW is there anything interesting behind this paywall I can't access?
>> https://access.redhat.com/solutions/115043
> 
> It just says that `cp a b` does not preserve NFSv4 ACLs whereas `cp -a a b`,
> `cp --preserve=all a b`, or `cp --preserve=xattr a b` does.  Unfortunately, 
> this is currently true only for Red Hat Enterprise Linux.

I'll dig in some more.
thanks,
Pádraig