Bug 692823 - "/bin/ls -l" does a getxattr() through libacl that should be an lgetxattr()
Summary: "/bin/ls -l" does a getxattr() through libacl that should be an lgetxattr()
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: coreutils
Version: 14
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Ondrej Vasik
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 692982 720318 733978
Blocks: 719607 720325 733979
TreeView+ depends on / blocked
 
Reported: 2011-04-01 10:15 UTC by David Howells
Modified: 2011-08-29 02:58 UTC (History)
11 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
: 692982 720325 (view as bug list)
Environment:
Last Closed: 2011-07-29 13:14:55 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
proposed fix (1.81 KB, patch)
2011-04-06 12:05 UTC, Kamil Dudka
ovasik: review+
Details | Diff

Description David Howells 2011-04-01 10:15:09 UTC
Description of problem:

/bin/ls -l on an automount directory of some nature peforms the following ops on each file:

lstat("doc", {st_mode=S_IFDIR|S_ISGID|0775, st_size=4096, ...}) = 0
lgetxattr("doc", "security.selinux", "unconfined_u:object_r:default_t:s0", 255) = 35
getxattr("doc", "system.posix_acl_access", 0x0, 0) = -1 EOPNOTSUPP (Operation not supported)

and possibly also:

getxattr("/etc", "system.posix_acl_default", 0x0, 0) = -1 ENODATA (No data available)

depending on the circumstances.

The first two calls (lstat and lgetxattr) don't follow symlinks and mounts, but the second is a getxattr call which _does_ follow symlinks and mounts.  The latter can cause automount points to mount, which the former won't, at least from linux-2.6.38-rc1 onwards.

These should be consistent, and so I believe the getxattr calls should be lgetxattr calls.  Without this, the first two calls may refer to a different object to the second pair of calls.

Version-Release number of selected component (if applicable):

F-13 and F-14 coreutils both show this.  coreutils-8.5-7.fc14.x86_64 is what's on my desktop.

How reproducible:

100%

Steps to Reproduce:
1. strace -e trace=lstat,getxattr,lgetxattr ls -l /media
  
Actual results:

lstat("/media", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
lgetxattr("/media", "security.selinux", "system_u:object_r:mnt_t:s0", 255) = 27
getxattr("/media", "system.posix_acl_access", 0x0, 0) = -1 ENODATA (No data available)
getxattr("/media", "system.posix_acl_default", 0x0, 0) = -1 ENODATA (No data available)

Expected results:

The getxattrs should be lgetxattrs.

Additional info:

This is a problem that occurs in conjunction with the acl package - that doesn't provide a way of calling lgetxattr() through it.

Comment 1 Ondrej Vasik 2011-04-01 10:25:24 UTC
coreutils (gnulib) just uses acl_extended_file from libacl - which actually does the getxattr calls. Reassigning to acl package.

Comment 2 Kamil Dudka 2011-04-01 10:46:46 UTC
There is not much we can do with acl_extended_file().  Symlinks are always followed when setting/getting ACLs, because they can have no ACLs themselves.  The only choice I see is to either not call acl_extended_file() for symlinks, or use acl_extended_fd(), which is not applicable on symlinks by definition.

Comment 3 Ondrej Vasik 2011-04-01 11:16:10 UTC
File_has_acl() gnulib function already calls libacl acl_extended_file() only for non-symlinks ...
But as discussed of bugzilla, it really looks it can't be fixed in libacl and coreutils is probably the right place.

Comment 5 David Howells 2011-04-01 13:48:48 UTC
The files in question are not symlinks; they are directories with follow_link semantics in the kernel (or the equivalent in 2.6.38+).

You also cannot simply look at the filesystem the target file is on and ask "is it autofs?".  The filesystem may be nfs4 (referrals), cifs (DFS junction point) or afs (volume mount point), and more may follow.  Currently, you cannot tell if it's an automount point or not.

I would think there needs to be a version of acl_extended_file() that calls lgetxattr().  If that doesn't work because the file is a symlink and thus has no ACL, then ls can use acl_extended_file() if -L was passed to it or if it's going to dereference it anyway.

Comment 6 Kamil Dudka 2011-04-01 13:57:23 UTC
(In reply to comment #5)
> You also cannot simply look at the filesystem the target file is on and ask "is
> it autofs?".  The filesystem may be nfs4 (referrals), cifs (DFS junction point)
> or afs (volume mount point), and more may follow.  Currently, you cannot tell
> if it's an automount point or not.

It should be possible as long as they are not yet mounted.  Please attach the output of this:

$ find -xautofs -printf "%p\t%F\n"

> I would think there needs to be a version of acl_extended_file() that calls
> lgetxattr().

There is no call of l?etxattr() in the whole acl library.  Does any other ACL implementation have such ability?

Comment 7 David Howells 2011-04-01 14:56:23 UTC
> There is no call of l?etxattr() in the whole acl library.  Does any other ACL
> implementation have such ability?

MacOS X's getxattr() can take a XATTR_NOFOLLOW flag that has the same effect as using lgetxattr().

Also, isn't acl_extended_file() a Linux extension?  Would it be that hard to add an acl_extended_file_nofollow() or something?

Comment 8 David Howells 2011-04-01 15:14:39 UTC
> > You also cannot simply look at the filesystem the target file is on and ask
> > "is it autofs?".  The filesystem may be nfs4 (referrals), cifs (DFS junction
> > point) or afs (volume mount point), and more may follow.  Currently, you
> > cannot tell if it's an automount point or not.
>
> It should be possible as long as they are not yet mounted.  Please attach the
> output of this:
>
> $ find -xautofs -printf "%p\t%F\n"

So, if I enable CONFIG_AFS=m, CONFIG_FSCACHE=m and CONFIG_DNS_RESOLVER=y, then do:

insmod /tmp/fscache.ko 
insmod /tmp/af-rxrpc.ko 
insmod /tmp/kafs.ko rootcell=grand.central.org
mount -t afs "#grand.central.org:root.cell." /afs

that will mount grand.central.org's cell root on /afs.  I can then do:

[root@andromeda ~]# cat /proc/mounts | grep afs
#grand.central.org:root.cell. /afs afs rw,relatime,autocell 0 0
[root@andromeda ~]# find /afs/doc -xautofs -printf "%p\t%F\n"
/afs/doc        unknown
/afs/doc/protocol       unknown
/afs/doc/protocol/rx    unknown
/afs/doc/protocol/rx/rxkad-2b.html      unknown
/afs/doc/protocol/rx/rxkad-2b.pod       unknown
/afs/doc/protocol/rx/rxkad-2b.txt       unknown
[root@andromeda ~]# cat /proc/mounts | grep afs
#grand.central.org:root.cell. /afs afs rw,relatime,autocell 0 0
#root.doc. /afs/doc afs rw,relatime,cell=grand.central.org 0 0
#doc.rx. /afs/doc/protocol/rx afs rw,relatime,cell=grand.central.org 0 0
[root@andromeda ~]# 

As you can see, -xautofs doesn't restrict anything.  I suspect this is because it does a text comparison on the filesystem type (I see it opens /proc/mounts and /etc/mtab - at least once per file examined by the looks of it).

If you actually wanted me to try this on autofs filesystem, then I'll have to set one up.

Comment 9 Kamil Dudka 2011-04-01 15:39:48 UTC
(In reply to comment #7)
> Also, isn't acl_extended_file() a Linux extension?  Would it be that hard to
> add an acl_extended_file_nofollow() or something?

Andreas, is addition of such a new function to libacl acceptable by upstream?
The problem here is that acl_extended_file() triggers a mount even when not necessary.

Comment 10 Ian Kent 2011-04-01 15:47:21 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > You also cannot simply look at the filesystem the target file is on and ask "is
> > it autofs?".  The filesystem may be nfs4 (referrals), cifs (DFS junction point)
> > or afs (volume mount point), and more may follow.  Currently, you cannot tell
> > if it's an automount point or not.
> 
> It should be possible as long as they are not yet mounted.  Please attach the
> output of this:
> 
> $ find -xautofs -printf "%p\t%F\n"
> 

There is nothing to be gained from this output.
It will have no effect on CIFS, NFS or AFS automount points in
2.6.38+ kernels.

This extension was added to find specifically for using find
with the autofs fs and find should be causing automount points
to mount.

Comment 11 Ian Kent 2011-04-01 15:54:31 UTC
(In reply to comment #5)
> The files in question are not symlinks; they are directories with follow_link
> semantics in the kernel (or the equivalent in 2.6.38+).
> 
> You also cannot simply look at the filesystem the target file is on and ask "is
> it autofs?".  The filesystem may be nfs4 (referrals), cifs (DFS junction point)
> or afs (volume mount point), and more may follow.  Currently, you cannot tell
> if it's an automount point or not.
> 
> I would think there needs to be a version of acl_extended_file() that calls
> lgetxattr().  If that doesn't work because the file is a symlink and thus has
> no ACL, then ls can use acl_extended_file() if -L was passed to it or if it's
> going to dereference it anyway.

This is a sensible approach.

Just because it is present doesn't mean it has to be used and
applications that need to use it on a symlink (or vfs-automount)
mount point will get a no data return and possibly move on to
following the symlink, which is when the ACL for that object
needs to be checked.

Comment 12 Andreas Gruenbacher 2011-04-01 17:35:51 UTC
I don't see what we can do to fix this other than not using acl_extended_file() in file_has_acl().  We could either add and use acl_extended_file_nofollow() there or put that code directly into gnulib; the code required is quite trivial and just as Linux specific as acl_extended_file().

Which of the two would you prefer?

Comment 13 Kamil Dudka 2011-04-01 18:01:01 UTC
I'd prefer to create a new function in libacl such that we maintain all the code aware of the acl->xattr mapping at a single place.  If such a new function is acceptable, I'll send a patch to acl-devel.

Comment 14 Andreas Gruenbacher 2011-04-01 21:02:17 UTC
That should work, thanks.

I'm adding Jim; this should be relevant for him, too.

Comment 15 Kamil Dudka 2011-04-01 21:19:39 UTC
cloned for acl - bug 692982

The upstream patch should be ready for review next week.

Comment 16 Kamil Dudka 2011-04-06 12:05:22 UTC
Created attachment 490246 [details]
proposed fix

Comment 17 Ondrej Vasik 2011-04-06 14:12:05 UTC
Comment on attachment 490246 [details]
proposed fix

Looks sane, please propose it to gnulib upstream. Only small thing, maybe you should add the declaration into acl-internal.h (as is done for the other HAVE_ACL* macros.

Comment 18 Kamil Dudka 2011-04-06 14:57:53 UTC
Done.  Thanks for review.

http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/25899

Comment 19 Kamil Dudka 2011-07-08 11:40:46 UTC
David has just reminded me this bug.  Unfortunately, there has been no reply on the upstream mailing list.  I will rebase the patch and repost it upstream as soon as I return from my vacation.  In the meantime the updates of libacl for F14+ can go through Fedora QA.

Comment 20 Andreas Gruenbacher 2011-07-11 10:41:30 UTC
Ondrej, I don't see how this is related to gnulib.  The new acl_extended_file_nofollow() function is in libacl. Coreutils just need to start using it, with autoconf test and all. At the shared library level, acl_extended_file_nofollow requires version ACL_1.2 of libacl, but rpm will figure out that dependency automatically.

Comment 21 Ondrej Vasik 2011-07-11 10:52:32 UTC
Andreas, see comment #18 - relation to gnulib is that coreutils (ls) uses gnulib functions for that - and Kamil's patch will solve it once accepted upstream - then I'll do coreutils update in Fedora.

Comment 22 Andreas Gruenbacher 2011-07-11 11:30:41 UTC
Okay. Let's have Jim add the gnulib and coreutils parts.

Comment 23 Jim Meyering 2011-07-22 13:28:43 UTC
I've just reviewed and tested that patch, made some minor changes
and pushed the result to gnulib:

  http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/25899/focus=27644

With that, it will be in the next upstream release of coreutils.

Comment 24 Ondrej Vasik 2011-07-29 13:14:55 UTC
As it is now pushed - http://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=95f7c57ff4090a5dee062044d2c7b99879077808 - built rawhide as coreutils-8.12-3.fc17 . Closing RAWHIDE, will be part of next cumulative coreutils update for released Fedoras (where acl_extended_file_nofollow available in libacl)


Note You need to log in before you can comment on or make changes to this bug.