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
       [not found] <6127852.nNyiNAGI2d@nimes>
@ 2024-04-13 16:39 ` Pádraig Brady
  2024-04-13 19:29   ` Bruno Haible
       [not found]   ` <57792d8c-59d1-efbe-067a-886239cc3a4e@draigBrady.com>
  0 siblings, 2 replies; 6+ messages in thread
From: Pádraig Brady @ 2024-04-13 16:39 UTC (permalink / raw)
  To: Bruno Haible, 70214; +Cc: Andreas Gruenbacher, linux-cifs

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread