GNU bug report logs - #40509
Use of fsetxattr() in cp tickles an EXT leak (possibly unnecessarily so)

Previous Next

To reply to this bug, email your comments to 40509 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Gregg Leventhal <gleventhal <at> janestreet.com>
To: bug-coreutils <at> gnu.org
Subject: Use of fsetxattr() in cp tickles an EXT leak (possibly unnecessarily
 so)
Date: Wed, 8 Apr 2020 10:15:51 -0400

[Message part 1 (text/plain, inline)]

This bug <https://patchwork.criu.org/patch/7413/> in EXT4 leaks posix_acl
allocations causing unreclaimable slab allocations to grow unbounded in the
kmalloc-64 cache.

It turns out that most of the problems we are having due to that leak is
due to programs making heavy use of "cp -a" or any of the cp --preserve
arguments that call fsetxattr.




*$ echo example1 > file$ strace -e fgetxattr,fsetxattr -f cp -a file
file2fgetxattr(3, "system.posix_acl_access", 0x7ffef5a38ce0, 132) = -1
ENODATA (No data available)fsetxattr(4, "system.posix_acl_access",
"\2\0\0\0\1\0\6\0\377\377\377\377\4\0\6\0\377\377\377\377
\0\4\0\377\377\377\377", 28, 0) = 0*

Mainly, I am concerned about this behavior because there are no ACLs, we
don't use them, so what is it setting?
















* $ getfacl file2# file: file2# owner: gleventhal# group:
techuser::rw-group::rw-other::r--$ touch new$ getfacl new# file: new#
owner: gleventhal# group: techuser::rw-group::rw-other::r--*

This is an ext4 file system with kernel 3.10.0-1062.18.1.el7 - CentOS Linux
release 7.7.1908 (Core)

rsync doesn't make set/get xattr calls and purports to preserve ACLs with
-A.

Thanks for listening, please let me know if I can provide more
information.  I hope all of you are doing well during this trying time in
our civilization.

[Message part 2 (text/html, inline)]

Message #8 received at 40509 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Gregg Leventhal <gleventhal <at> janestreet.com>
Cc: 40509 <at> debbugs.gnu.org
Subject: Re: bug#40509: Use of fsetxattr() in cp tickles an EXT leak (possibly
 unnecessarily so)
Date: Wed, 8 Apr 2020 10:32:30 -0700

On 4/8/20 7:15 AM, Gregg Leventhal wrote:

> rsync doesn't make set/get xattr calls and purports to preserve ACLs with
> -A.

I'm not quite following your bug report, but it appears that you're saying that 
cp could somehow discover that it needn't use fgetxattr and fsetxattr on files 
that lack extended attributes, and for those files cp could stick with ordinary 
POSIX syscalls (e.g., umask, chmod) to give files proper permissions, and in 
that case 'cp' would presumably (a) operate more efficiently and (b) not trigger 
a bug in the EXT filesystem.

This sounds like a worthy suggestion, though of course it would be better to 
have a concrete proposal in the form of a coreutils patch, along with a few 
performance measurements. For starters, how does rsync do it?

Also, of course EXT should be fixed regardless of what coreutils does here.

Message #11 received at 40509 <at> debbugs.gnu.org (full text, mbox):

From: Gregg Leventhal <gleventhal <at> janestreet.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 40509 <at> debbugs.gnu.org
Subject: Re: bug#40509: Use of fsetxattr() in cp tickles an EXT leak (possibly
 unnecessarily so)
Date: Wed, 15 Apr 2020 10:11:53 -0400

[Message part 1 (text/plain, inline)]

Paul,
Your understanding is correct.  The problem in cp is that it uses
acl_get_fd or acl_get_file which will always return an ACL on EXT4,
therefore contriving an ACL to set on the target when it doesn't exist on
the source.
This causes an extraneous fsetxattr, and hits several arguably
unnecessary functions from libattr and libacl (since if there are no
xattrs, it doesn't make sense to check for ACLs when they're built on top
of xattrs)

Since ACLs cannot exist without xattrs on EXT4, I propose we simply check
for the existence of xattrs early on and bailout early if they're not found.

A simple patch that attempts to do this, and seems to work for the cp -a
case is below.  It basically checks for the size of the list of xattrs, and
if it's > 0 or ERANGE is returned (signifying that the buffer isn't large
enough to hold the entire list), then we know that we have xattrs,
otherwise bail out.

--- src/copy.c 2020-04-09 09:30:08.279516111 -0400
+++ src/copy.c 2020-04-09 15:21:47.939897542 -0400
@@ -16,11 +16,13 @@

 /* Extracted from cp.c and librarified by Jim Meyering.  */

+
 #include <config.h>
 #include <stdio.h>
 #include <assert.h>
 #include <sys/ioctl.h>
 #include <sys/types.h>
+#include <attr/xattr.h>
 #include <selinux/selinux.h>

 #if HAVE_HURD_H
@@ -536,6 +538,9 @@
   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);
+  ssize_t xattr_size;
+  size_t size = 0;
+  char *list;
   struct error_context ctx =
   {
     .error = all_errors ? copy_attr_allerror : copy_attr_error,
@@ -543,13 +548,28 @@
     .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,
-                        (all_errors || some_errors ? &ctx : NULL));
-  else
-    ret = attr_copy_file (src_path, dst_path,
+  {
+
+    xattr_size = flistxattr(src_fd, list, size);
+    if ( xattr_size || errno == ERANGE )
+    {
+      ret = attr_copy_fd (src_path, src_fd, dst_path, dst_fd,
                           selinux_done ? check_selinux_attr : NULL,
                           (all_errors || some_errors ? &ctx : NULL));
+    }
+    else
+      ret = (int)xattr_size;
+  }
+  else
+  {
+    xattr_size = listxattr(src_path, list, size);
+    if ( xattr_size || errno == ERANGE )
+      ret = attr_copy_file (src_path, dst_path,
+                            selinux_done ? check_selinux_attr : NULL,
+                            (all_errors || some_errors ? &ctx : NULL));
+    else
+      ret = (int)xattr_size;
+  }

   return ret == 0;
 }

If you agree with this direction, I can continue, addressing other affected
code paths (i.e --preserve=mode).  Either way, I am interested to hear your
thoughts.
It also might make sense to wrap this in a file system check and only do
this for EXT4, since I don't know if xattrs and acls are mutually inclusive
on all supported file systems.


*EXAMPLE of behavior difference between rsync -A and cp -a*
# Create non-empty source files
$ echo source file | tee cp_src rsync_src

# cp attempts to setxattr even though no extended attributes exist
$ strace -f cp -a cp_src cp_dest |& egrep -i 'acl|attr'
open("/lib64/libacl.so.1", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libattr.so.1", O_RDONLY|O_CLOEXEC) = 3
flistxattr(3, NULL, 0)                  = 0
flistxattr(3, 0x7fffee2db2c0, 0)        = 0
fgetxattr(3, "system.posix_acl_access", 0x7fffee2db1c0, 132) = -1 ENODATA
(No data available)
fsetxattr(4, "system.posix_acl_access",
"\2\0\0\0\1\0\6\0\377\377\377\377\4\0\6\0\377\377\377\377
\0\4\0\377\377\377\377", 28, 0) = 0

# Rsync doesn't attempt an extraneous fsetxattr
$ strace -f rsync -A rsync_src rsync_dest |& egrep -i 'acl|attr'
open("/lib64/libattr.so.1", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libacl.so.1", O_RDONLY|O_CLOEXEC) = 3
 getxattr("rsync_src", "system.posix_acl_access", 0x7ffe9e150290, 132) = -1
ENODATA (No data available)
 getxattr(".rsync_dest.HgYAGV", "system.posix_acl_access", 0x7ffe9e14f100,
132) = -1 ENODATA (No data available)


# Add ACLS (and by extension, xattrs) to the source files
$ setfacl -m user:gleventhal:rwx cp_src
$ setfacl -m user:gleventhal:rwx rsync_src

# cp behaves the same
$ strace -f cp -a cp_src cp_dest2 |& egrep -i 'acl|attr'
open("/lib64/libacl.so.1", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libattr.so.1", O_RDONLY|O_CLOEXEC) = 3
flistxattr(3, NULL, 0)                  = 24
flistxattr(3, "system.posix_acl_access\0", 24) = 24
open("/etc/xattr.conf", O_RDONLY)       = -1 ENOENT (No such file or
directory)
fgetxattr(3, "system.posix_acl_access", NULL, 0) = 44
fgetxattr(3, "system.posix_acl_access",
"\2\0\0\0\1\0\6\0\377\377\377\377\2\0\7\0\3300\0\0\4\0\6\0\377\377\377\377\20\0\7\0\377\377\377\377
\0\4\0\377\377\377\377", 44) = 44
fsetxattr(4, "system.posix_acl_access",
"\2\0\0\0\1\0\6\0\377\377\377\377\2\0\7\0\3300\0\0\4\0\6\0\377\377\377\377\20\0\7\0\377\377\377\377
\0\4\0\377\377\377\377", 44, 0) = 0
fgetxattr(3, "system.posix_acl_access",
"\2\0\0\0\1\0\6\0\377\377\377\377\2\0\7\0\3300\0\0\4\0\6\0\377\377\377\377\20\0\7\0\377\377\377\377
\0\4\0\377\377\377\377", 132) = 44
fsetxattr(4, "system.posix_acl_access",
"\2\0\0\0\1\0\6\0\377\377\377\377\2\0\7\0\3300\0\0\4\0\6\0\377\377\377\377\20\0\7\0\377\377\377\377
\0\4\0\377\377\377\377", 44, 0) = 0

# rsync now does the set, because xattrs actually exist
$ strace -f rsync -A rsync_src rsync_dest2 |& egrep -i 'acl|attr'
open("/lib64/libattr.so.1", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libacl.so.1", O_RDONLY|O_CLOEXEC) = 3
 getxattr("rsync_src", "system.posix_acl_access",
"\2\0\0\0\1\0\6\0\377\377\377\377\2\0\7\0\3300\0\0\4\0\6\0\377\377\377\377\20\0\7\0\377\377\377\377
\0\4\0\377\377\377\377", 132) = 44
 getxattr(".rsync_dest2.DjNiRs", "system.posix_acl_access", 0x7ffeac4780d0,
132) = -1 ENODATA (No data available)
 setxattr(".rsync_dest2.DjNiRs", "system.posix_acl_access",
"\2\0\0\0\1\0\6\0\377\377\377\377\2\0\7\0\3300\0\0\4\0\6\0\377\377\377\377\20\0\7\0\377\377\377\377
\0\4\0\377\377\377\377", 44, 0) = 0

On Wed, Apr 8, 2020 at 1:32 PM Paul Eggert <eggert <at> cs.ucla.edu> wrote:

> On 4/8/20 7:15 AM, Gregg Leventhal wrote:
>
> > rsync doesn't make set/get xattr calls and purports to preserve ACLs with
> > -A.
>
> I'm not quite following your bug report, but it appears that you're saying
> that
> cp could somehow discover that it needn't use fgetxattr and fsetxattr on
> files
> that lack extended attributes, and for those files cp could stick with
> ordinary
> POSIX syscalls (e.g., umask, chmod) to give files proper permissions, and
> in
> that case 'cp' would presumably (a) operate more efficiently and (b) not
> trigger
> a bug in the EXT filesystem.
>
> This sounds like a worthy suggestion, though of course it would be better
> to
> have a concrete proposal in the form of a coreutils patch, along with a
> few
> performance measurements. For starters, how does rsync do it?
>
> Also, of course EXT should be fixed regardless of what coreutils does here.
>

[Message part 2 (text/html, inline)]

Message #14 received at 40509 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Gregg Leventhal <gleventhal <at> janestreet.com>
Cc: 40509 <at> debbugs.gnu.org
Subject: Re: bug#40509: Use of fsetxattr() in cp tickles an EXT leak (possibly
 unnecessarily so)
Date: Wed, 15 Apr 2020 15:55:52 -0700

On 4/15/20 7:11 AM, Gregg Leventhal wrote:

> +    xattr_size = flistxattr(src_fd, list, size);
> +    if ( xattr_size || errno == ERANGE )

Surely this should be 'if (flistxattr (src_fd, NULL, 0) < 0 && errno == ERANGE)'.

> If you agree with this direction, I can continue, addressing other affected
> code paths (i.e --preserve=mode).

This sounds like a good thing to do. Before you spend a lot of time on it, 
though, would you be willing to assign copyright to your work product to the FSF 
so that we could install the patch? If so, I can send you email on how to fill 
out the paperwork; if not, we'd better arrange for someone else to write the fix.

Message #17 received at 40509 <at> debbugs.gnu.org (full text, mbox):

From: Gregg Leventhal <gleventhal <at> janestreet.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 40509 <at> debbugs.gnu.org
Subject: Re: bug#40509: Use of fsetxattr() in cp tickles an EXT leak (possibly
 unnecessarily so)
Date: Wed, 15 Apr 2020 19:22:24 -0400

[Message part 1 (text/plain, inline)]

Sure that’s fine (regarding copyright), just let me know what to fill out
please.

On Wed, Apr 15, 2020 at 6:55 PM Paul Eggert <eggert <at> cs.ucla.edu> wrote:

> On 4/15/20 7:11 AM, Gregg Leventhal wrote:
>
> > +    xattr_size = flistxattr(src_fd, list, size);
> > +    if ( xattr_size || errno == ERANGE )
>
> Surely this should be 'if (flistxattr (src_fd, NULL, 0) < 0 && errno ==
> ERANGE)'.
>
> > If you agree with this direction, I can continue, addressing other
> affected
> > code paths (i.e --preserve=mode).
>
> This sounds like a good thing to do. Before you spend a lot of time on it,
> though, would you be willing to assign copyright to your work product to
> the FSF
> so that we could install the patch? If so, I can send you email on how to
> fill
> out the paperwork; if not, we'd better arrange for someone else to write
> the fix.
>

[Message part 2 (text/html, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.