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

[Kamil Dudka]

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.

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

Kamil