Re: bug#70214: 'install' fails to copy regular file to autofs/cifs, due to ACL or xattr handling

Re: bug#70214: 'install' fails to copy regular file to autofs/cifs, due to ACL or xattr handling

[Pádraig Brady]

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

On 05/04/2024 10:48, Bruno Haible wrote:
> Hi,
> 
> The 'install' program from coreutils-9.5 fails to copy a regular file
> from an ext4 mount to an autofs/cifs mount.
> 
> The same operation, with 'cp -a', works fine.
> 
> Also, it works fine when coreutils was built with the configure options
> "--disable-acl --disable-xattr".
> 
> How to reproduce
> ================
> 
> 1) On the machine sparcdev.matoro.tk (Linux 6.8.2), I built coreutils-9.5
> from source,
>    - once with default options, in build-sparc64/,
>    - once with "--disable-acl --disable-xattr", in build-sparc64-no-acl/.
> 
> 2) Create a regular file on an ext4 mount:
> 
> $ echo hi > /var/tmp/foo3941
> $ ls -lZ /var/tmp/foo3941
> -rw-r----- 1 g-haible g-haible ? 3 Apr  4 13:29 /var/tmp/foo3941
> $ getfacl /var/tmp/foo3941
> getfacl: Removing leading '/' from absolute path names
> # file: var/tmp/foo3941
> # owner: g-haible
> # group: g-haible
> user::rw-
> group::r--
> other::---
> $ df -m /var/tmp/
> Filesystem     1M-blocks   Used Available Use% Mounted on
> /dev/root         560245 123140    408574  24% /
> $ mount | grep ' / '
> /dev/sda2 on / type ext4 (rw,noatime)
> 
> 3) Details about the destination directory:
> 
> $ echo $HOME
> /media/guest-homedirs/haible
> $ mount | grep /media/guest-homedirs/haible
> /etc/autofs/auto.guest-homedirs on /media/guest-homedirs/haible type autofs (rw,relatime,fd=7,pgrp=2325,timeout=60,minproto=5,maxproto=5,direct,pipe_ino=46092)
> //syslog.matoro.tk/guest-haible on /media/guest-homedirs/haible type cifs (rw,nosuid,relatime,vers=1.0,cache=strict,username=nobody,uid=30014,forceuid,gid=30014,forcegid,addr=fd05:0000:0000:0000:0000:0000:0000:0001,soft,unix,posixpaths,serverino,mapposix,acl,rsize=1048576,wsize=65536,bsize=1048576,retrans=1,echo_interval=60,actimeo=1,closetimeo=1)
> 
> 4) The operation that fails:
> 
> $ build-sparc64/src/ginstall -c /var/tmp/foo3941 $HOME/foo3941; echo $?
> build-sparc64/src/ginstall: setting permissions for '/media/guest-homedirs/haible/foo3941': Permission denied
> 1
> $ build-sparc64-no-acl/src/ginstall -c /var/tmp/foo3941 $HOME/foo3941; echo $?
> 0
> 
> 5) The same thing with 'cp -a' succeeds:
> 
> $ build-sparc64/src/cp -a /var/tmp/foo3941 $HOME/foo3941; echo $?
> 0
> $ build-sparc64-no-acl/src/cp -a /var/tmp/foo3941 $HOME/foo3941; echo $?
> 0
> 
> 6) 'strace' shows a failing call to fsetxattr:
> 
> $ strace build-sparc64/src/ginstall -c /var/tmp/foo3941 $HOME/foo3941

> fsetxattr(4, "system.posix_acl_access", "\2\0\0\0\1\0\6\0\377\377\377\377\4\0\0\0\377\377\377\377 \0\0\0\377\377\377\377", 28, 0) = -1 EACCES (Permission denied)
> fchmod(4, 0600)                         = 0

> Notes
> =====
> 
> The 'cp' program does *not* use fsetxattr() calls on the destination file
> descriptor and therefore does not fail:
> 
> $ strace build-sparc64/src/cp -a /var/tmp/foo3941 $HOME/foo3941

> flistxattr(3, NULL, 0)                  = 0
> flistxattr(3, 0x7feff9860a0, 0)         = 0
> fchmod(4, 0100640)                      = 0
> flistxattr(3, NULL, 0)                  = 0
> flistxattr(3, 0x7feff9860c0, 0)         = 0

> As you can see, it uses 4 flistxattr() calls on the source file descriptor,
> apparently detecting that it's a regular file without ACLs, and proceeds to
> do a simple fchmod() call on the destination file descriptor.
> 
> Probably the same logic is needed in the 'install' program.

install(1) defaults to mode 600 for new files, and uses set_acl() with that
(since 2007 https://github.com/coreutils/coreutils/commit/f634e8844 )
The psuedo code that install(1) uses is:

copy_reg()
   if (x->set_mode) /* install */
     set_acl(dest, x->mode /* 600 */)
       ctx->acl = acl_from_mode ( /* 600 */)
       acl_set_fd (ctx->acl) /* fails EACCES */
       if (! acls_set)
          must_chmod = true;
       if (must_chmod)
         saved_errno = EACCES;
         chmod (ctx->mode /* 600 */)
         if (save_errno)
           return -1;

This issue only only seems to be on CIFS.
I'm seeing lot of weird behavior with ACLs there:

   acl_set_fd (acl_from_mode (600)) -> EACCES
   acl_set_fd (acl_from_mode (755)) -> EINVAL
   getxattr ("system.posix_acl_access") -> EOPNOTSUPP

Note we ignore EINVAL and EOPNOTSUPP errors in set_acl(),
and it's just the EACCES that's problematic.
Note this is quite similar to https://debbugs.gnu.org/65599
where Paul also noticed EACCES with fsetxattr() (and others) on CIFS.

The attached is a potential solution which I tested as working
on the same matoro system that Bruno used.

I think I'll apply that after thinking a bit more about it.

cheers,
Pádraig.

[-- Attachment #2: gnulib-set-acl-cifs.patch --]
[-- Type: text/x-patch, Size: 1876 bytes --]

From d828d9656c3bd1ddf0fcddb578ddb2ed9a4d3701 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <P@draigBrady.com>
Date: Sat, 13 Apr 2024 17:13:02 +0100
Subject: [PATCH] acl-permissions: avoid erroneous failure on CIFS

* lib/set-permissions.c (set_acls): On Linux also discount
EACESS as a valid errno with FD operations, as CIFS was seen to
return that erroneously in some cases.
---
 ChangeLog             | 7 +++++++
 lib/set-permissions.c | 8 +++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index c72165e268..fd094d1091 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2024-04-13  Pádraig Brady  <P@draigBrady.com>
+
+	acl-permissions: avoid erroneous failure on CIFS
+	* lib/set-permissions.c (set_acls): On Linux (and others), also discount
+	EACESS as a valid errno with FD operations, as CIFS was seen to
+	return that erroneously in some cases.
+
 2024-04-13  Bruno Haible  <bruno@clisp.org>
 
 	gnulib-tool.py: Code tweak.
diff --git a/lib/set-permissions.c b/lib/set-permissions.c
index 83a355faa5..7f8e55f5cd 100644
--- a/lib/set-permissions.c
+++ b/lib/set-permissions.c
@@ -520,7 +520,13 @@ set_acls (struct permission_context *ctx, const char *name, int desc,
             ret = acl_set_file (name, ACL_TYPE_ACCESS, ctx->acl);
           if (ret != 0)
             {
-              if (! acl_errno_valid (errno))
+              if (! acl_errno_valid (errno)
+#   if defined __linux__
+                  /* Special case EACCES for fd operations as CIFS
+                     was seen to erroneously return that in some cases.  */
+                  || (HAVE_ACL_SET_FD && desc != -1 && errno == EACCES)
+#   endif
+                 )
                 {
                   ctx->acls_not_supported = true;
                   if (from_mode || acl_access_nontrivial (ctx->acl) == 0)
-- 
2.44.0

Re: bug#70214: 'install' fails to copy regular file to autofs/cifs, due to ACL or xattr handling

[Bruno Haible]

Hi Pádraig,

I wrote:
> > 5) The same thing with 'cp -a' succeeds:
> > 
> > $ build-sparc64/src/cp -a /var/tmp/foo3941 $HOME/foo3941; echo $?
> > 0
> > $ build-sparc64-no-acl/src/cp -a /var/tmp/foo3941 $HOME/foo3941; echo $?
> > 0

You wrote:
> The psuedo code that install(1) uses is:
> 
> copy_reg()
>    if (x->set_mode) /* install */
>      set_acl(dest, x->mode /* 600 */)
>        ctx->acl = acl_from_mode ( /* 600 */)
>        acl_set_fd (ctx->acl) /* fails EACCES */
>        if (! acls_set)
>           must_chmod = true;
>        if (must_chmod)
>          saved_errno = EACCES;
>          chmod (ctx->mode /* 600 */)
>          if (save_errno)
>            return -1;

And, for comparison, what is the pseudo-code that 'cp -a' uses?
I would guess that there must be a relevant difference between both.

Bruno

Re: bug#70214: 'install' fails to copy regular file to autofs/cifs, due to ACL or xattr handling

[Pádraig Brady]

On 13/04/2024 20:29, Bruno Haible wrote:
> Hi Pádraig,
> 
> I wrote:
>>> 5) The same thing with 'cp -a' succeeds:
>>>
>>> $ build-sparc64/src/cp -a /var/tmp/foo3941 $HOME/foo3941; echo $?
>>> 0
>>> $ build-sparc64-no-acl/src/cp -a /var/tmp/foo3941 $HOME/foo3941; echo $?
>>> 0
> 
> You wrote:
>> The psuedo code that install(1) uses is:
>>
>> copy_reg()
>>     if (x->set_mode) /* install */
>>       set_acl(dest, x->mode /* 600 */)
>>         ctx->acl = acl_from_mode ( /* 600 */)
>>         acl_set_fd (ctx->acl) /* fails EACCES */
>>         if (! acls_set)
>>            must_chmod = true;
>>         if (must_chmod)
>>           saved_errno = EACCES;
>>           chmod (ctx->mode /* 600 */)
>>           if (save_errno)
>>             return -1;
> 
> And, for comparison, what is the pseudo-code that 'cp -a' uses?
> I would guess that there must be a relevant difference between both.

The cp pseudo code is:

copy_reg()
   if (preserve_xattr)
     copy_attr()
       ret = attr_copy_fd()
       if (ret == -1 && require_preserve_xattr /*false*/)
         return failure;
   if (preserve_mode)
     copy_acl()
       qcopy_acl()
         #if USE_XATTR /* true */
           fchmod() /* chmod before setting ACLs as doing after may reset */
           return attr_copy_fd() /* successful if no ACLs in source */
         #endif

If however you add ACLs in the source, you induce a similar failure:

$ setfacl -m u:nobody:r /var/tmp/foo3942
$ src/cp -a /var/tmp/foo3942 foo3942; echo $?
src/cp: preserving permissions for ‘foo3942’: Permission denied
1

The corresponding strace is:

fchmod(4, 0100640)                      = 0
flistxattr(3, NULL, 0)                  = 24
flistxattr(3, "system.posix_acl_access\0", 24) = 24
fgetxattr(3, "system.posix_acl_access", NULL, 0) = 44
fgetxattr(3, "system.posix_acl_access", "\2\0...\4", 44) = 44
fsetxattr(4, "system.posix_acl_access", "\2\0...\4", 44, 0) = -1 EACCES (Permission denied)

BTW I was wondering about the need for install(1) to set_acl() at all,
rather than just using chmod.
The following comment in lib/set-permissions.c may be pertinent:

/* If we can't set an acl which we expect to be able to set, try setting
    the permissions to ctx->mode. Due to possible inherited permissions,
    we cannot simply chmod */

BTW this is all under kernel version:

$ uname -r
6.8.5-gentoo-sparc64

With these cifs options:

$ mount | grep cifs
//syslog.matoro.tk/guest-pixelbeat on /media/guest-homedirs/pixelbeat type cifs
(rw,nosuid,relatime,vers=1.0,cache=strict,username=nobody,uid=30017,forceuid,
gid=30017,forcegid,addr=fd05:0000:0000:0000:0000:0000:0000:0001,
soft,unix,posixpaths,serverino,mapposix,acl,
rsize=1048576,wsize=65536,bsize=1048576,retrans=1,echo_interval=60,actimeo=1,closetimeo=1)

cheers,
Pádraig

Re: bug#70214: 'install' fails to copy regular file to autofs/cifs, due to ACL or xattr handling

[Andreas Grünbacher]

Hello,

Am So., 14. Apr. 2024 um 00:43 Uhr schrieb Pádraig Brady <P@draigbrady.com>:
> On 13/04/2024 20:29, Bruno Haible wrote:
> > Hi Pádraig,
> >
> > I wrote:
> >>> 5) The same thing with 'cp -a' succeeds:
> >>>
> >>> $ build-sparc64/src/cp -a /var/tmp/foo3941 $HOME/foo3941; echo $?
> >>> 0
> >>> $ build-sparc64-no-acl/src/cp -a /var/tmp/foo3941 $HOME/foo3941; echo $?
> >>> 0
> >
> > You wrote:
> >> The psuedo code that install(1) uses is:
> >>
> >> copy_reg()
> >>     if (x->set_mode) /* install */
> >>       set_acl(dest, x->mode /* 600 */)
> >>         ctx->acl = acl_from_mode ( /* 600 */)
> >>         acl_set_fd (ctx->acl) /* fails EACCES */
> >>         if (! acls_set)
> >>            must_chmod = true;
> >>         if (must_chmod)
> >>           saved_errno = EACCES;
> >>           chmod (ctx->mode /* 600 */)
> >>           if (save_errno)
> >>             return -1;
> >
> > And, for comparison, what is the pseudo-code that 'cp -a' uses?
> > I would guess that there must be a relevant difference between both.
>
> The cp pseudo code is:
>
> copy_reg()
>    if (preserve_xattr)
>      copy_attr()
>        ret = attr_copy_fd()
>        if (ret == -1 && require_preserve_xattr /*false*/)
>          return failure;
>    if (preserve_mode)
>      copy_acl()
>        qcopy_acl()
>          #if USE_XATTR /* true */
>            fchmod() /* chmod before setting ACLs as doing after may reset */
>            return attr_copy_fd() /* successful if no ACLs in source */
>          #endif
>
> If however you add ACLs in the source, you induce a similar failure:
>
> $ setfacl -m u:nobody:r /var/tmp/foo3942
> $ src/cp -a /var/tmp/foo3942 foo3942; echo $?
> src/cp: preserving permissions for ‘foo3942’: Permission denied
> 1
>
> The corresponding strace is:
>
> fchmod(4, 0100640)                      = 0
> flistxattr(3, NULL, 0)                  = 24
> flistxattr(3, "system.posix_acl_access\0", 24) = 24
> fgetxattr(3, "system.posix_acl_access", NULL, 0) = 44
> fgetxattr(3, "system.posix_acl_access", "\2\0...\4", 44) = 44
> fsetxattr(4, "system.posix_acl_access", "\2\0...\4", 44, 0) = -1 EACCES (Permission denied)

Why does CIFS think EACCES is an appropriate error to return here? The
fchmod() succeeds, so changing the file permissions via fsetxattr()
should really succeed as well.

Thanks,
Andreas

Re: bug#70214: 'install' fails to copy regular file to autofs/cifs, due to ACL or xattr handling

[Pádraig Brady]

On 15/04/2024 15:37, Andreas Grünbacher wrote:
> Hello,
> 
> Am So., 14. Apr. 2024 um 00:43 Uhr schrieb Pádraig Brady <P@draigbrady.com>:
>> On 13/04/2024 20:29, Bruno Haible wrote:
>>> Hi Pádraig,
>>>
>>> I wrote:
>>>>> 5) The same thing with 'cp -a' succeeds:
>>>>>
>>>>> $ build-sparc64/src/cp -a /var/tmp/foo3941 $HOME/foo3941; echo $?
>>>>> 0
>>>>> $ build-sparc64-no-acl/src/cp -a /var/tmp/foo3941 $HOME/foo3941; echo $?
>>>>> 0
>>>
>>> You wrote:
>>>> The psuedo code that install(1) uses is:
>>>>
>>>> copy_reg()
>>>>      if (x->set_mode) /* install */
>>>>        set_acl(dest, x->mode /* 600 */)
>>>>          ctx->acl = acl_from_mode ( /* 600 */)
>>>>          acl_set_fd (ctx->acl) /* fails EACCES */
>>>>          if (! acls_set)
>>>>             must_chmod = true;
>>>>          if (must_chmod)
>>>>            saved_errno = EACCES;
>>>>            chmod (ctx->mode /* 600 */)
>>>>            if (save_errno)
>>>>              return -1;
>>>
>>> And, for comparison, what is the pseudo-code that 'cp -a' uses?
>>> I would guess that there must be a relevant difference between both.
>>
>> The cp pseudo code is:
>>
>> copy_reg()
>>     if (preserve_xattr)
>>       copy_attr()
>>         ret = attr_copy_fd()
>>         if (ret == -1 && require_preserve_xattr /*false*/)
>>           return failure;
>>     if (preserve_mode)
>>       copy_acl()
>>         qcopy_acl()
>>           #if USE_XATTR /* true */
>>             fchmod() /* chmod before setting ACLs as doing after may reset */
>>             return attr_copy_fd() /* successful if no ACLs in source */
>>           #endif
>>
>> If however you add ACLs in the source, you induce a similar failure:
>>
>> $ setfacl -m u:nobody:r /var/tmp/foo3942
>> $ src/cp -a /var/tmp/foo3942 foo3942; echo $?
>> src/cp: preserving permissions for ‘foo3942’: Permission denied
>> 1
>>
>> The corresponding strace is:
>>
>> fchmod(4, 0100640)                      = 0
>> flistxattr(3, NULL, 0)                  = 24
>> flistxattr(3, "system.posix_acl_access\0", 24) = 24
>> fgetxattr(3, "system.posix_acl_access", NULL, 0) = 44
>> fgetxattr(3, "system.posix_acl_access", "\2\0...\4", 44) = 44
>> fsetxattr(4, "system.posix_acl_access", "\2\0...\4", 44, 0) = -1 EACCES (Permission denied)
> 
> Why does CIFS think EACCES is an appropriate error to return here? The
> fchmod() succeeds, so changing the file permissions via fsetxattr()
> should really succeed as well.

Right, it seems like a CIFS bug (already CC'd)
Even if it returned ENOTSUP (like getxattr(...posix...) does) it would be ok as we handle that.
It would be good to avoid it though.

You confirmed privately to me that the set_acl() is to clear any default ACLs
that may have been added to the newly created file, so the posted solution in
https://bugs.gnu.org/70214#11 that only does the set_acl() iff file_has_acl()
should avoid the CIFS issue.  It would be a bit less efficient if there were
ACLs, but a bit more efficient in the common case where there weren't ACLs.

cheers,
Pádraig

Re: bug#70214: 'install' fails to copy regular file to autofs/cifs, due to ACL or xattr handling

[Pádraig Brady]

On 13/04/2024 18:42, Pádraig Brady wrote:
> On 13/04/2024 17:39, Pádraig Brady wrote:
>> install(1) defaults to mode 600 for new files, and uses set_acl() with that
>> (since 2007 https://github.com/coreutils/coreutils/commit/f634e8844 )
>> The psuedo code that install(1) uses is:
>>
>> copy_reg()
>>      if (x->set_mode) /* install */
>>        set_acl(dest, x->mode /* 600 */)
>>          ctx->acl = acl_from_mode ( /* 600 */)
>>          acl_set_fd (ctx->acl) /* fails EACCES */
>>          if (! acls_set)
>>             must_chmod = true;
>>          if (must_chmod)
>>            saved_errno = EACCES;
>>            chmod (ctx->mode /* 600 */)
>>            if (save_errno)
>>              return -1;
>>
>> This issue only only seems to be on CIFS.
>> I'm seeing lot of weird behavior with ACLs there:
>>
>>      acl_set_fd (acl_from_mode (600)) -> EACCES
>>      acl_set_fd (acl_from_mode (755)) -> EINVAL
>>      getxattr ("system.posix_acl_access") -> EOPNOTSUPP
>>
>> Note we ignore EINVAL and EOPNOTSUPP errors in set_acl(),
>> and it's just the EACCES that's problematic.
>> Note this is quite similar to https://debbugs.gnu.org/65599
>> where Paul also noticed EACCES with fsetxattr() (and others) on CIFS.
>>
>> The attached is a potential solution which I tested as working
>> on the same matoro system that Bruno used.
>>
>> I think I'll apply that after thinking a bit more about it.
> 
> Actually that probably isn't appropriate,
> as fsetxattr() can validly return EACESS
> even though not documented in the man page at least.
> The following demonstrates that:
> .
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <sys/xattr.h>
> #include <fcntl.h>
> #include <attr/libattr.h>
> #include <stdio.h>
> #include <unistd.h>
> 
> int main(void)
> {
>       int wfd;
>       /* Note S_IWUSR is not set.  */
>       if ((wfd=open("writable", O_CREAT|O_WRONLY|O_EXCL, S_IRUSR)) == -1)
>           fprintf(stderr, "open('writable') error [%m]\n");
>       if (write(wfd, "data", 1) == -1)
>           fprintf(stderr, "write() error [%m]\n");
>       if (fsetxattr(wfd, "user.test", "test", 4, 0) == -1)
>           fprintf(stderr, "fsetxattr() error [%m]\n");
> }
> 
> Another solution might be to file_has_acl() in copy.c
> and skip if "not supported" or nothing is returned.
> The following would do that, but I'm not sure about this
> (as I'm not sure about ACLs in general TBH).
> Note listxattr() returns 0 on CIFS here,
> while getxattr ("system.posix_acl_access") returns EOPNOTSUPP,
> and file_has_acl() uses listxattr() first.
> 
> diff --git a/src/copy.c b/src/copy.c
> index d584a27eb..2145d89d5 100644
> --- a/src/copy.c
> +++ b/src/copy.c
> @@ -1673,8 +1673,13 @@ set_dest_mode:
>        }
>      else if (x->set_mode)
>        {
> -      if (set_acl (dst_name, dest_desc, x->mode) != 0)
> -        return_val = false;
> +      errno = 0;
> +      int n = file_has_acl (dst_name, &sb);
> +      if (0 < n || (errno && ! (is_ENOTSUP (errno) || errno == ENOSYS)))
> +        {
> +          if (set_acl (dst_name, dest_desc, x->mode) != 0)
> +            return_val = false;
> +        }
>        }
> 
> 
> BTW I'm surprised this wasn't reported previously for CIFS,
> so I due to this bug and https://debbugs.gnu.org/65599
> I suspect a recentish change in CIFS wrt EACCES.

Thinking more about this, the solution presented above wouldn't work,
and I think this needs to be addressed in CIFS.
I.e. we may still need to reset the mode even if the file has no ACLs,
as generally only dirs get default ACLs copied, as demonstrated below:

$ mkdir acl$ setfacl -d -m o::rw acl
$ getfacl acl
# file: acl
# owner: padraig
# group: padraig
user::rwx
group::r-x
other::r-x
default:user::rwx
default:group::r-x
default:other::rw-

$ touch acl/file
$ ls -l acl/file
-rw-r--rw-. 1 padraig padraig 0 May  9 13:11 acl/file
$ getfacl acl/file
# file: acl/file
# owner: padraig
# group: padraig
user::rw-
group::r--
other::rw-

cheers,
Pádraig.