Bug 720325 - "/bin/ls -l" does a getxattr() through libacl that should be an lgetxattr()
"/bin/ls -l" does a getxattr() through libacl that should be an lgetxattr()
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: coreutils (Show other bugs)
6.1
All Linux
medium Severity medium
: rc
: ---
Assigned To: Ondrej Vasik
qe-baseos-daemons
:
Depends On: 692823 692982 720318 733978
Blocks: 719607 733979
  Show dependency treegraph
 
Reported: 2011-07-11 07:55 EDT by Ian Kent
Modified: 2011-12-06 12:12 EST (History)
14 users (show)

See Also:
Fixed In Version: coreutils-8.4-16.el6
Doc Type: Bug Fix
Doc Text:
Prior to this update, the acl_extended_file() function could cause unnecessary mounts of autofs when using the ls command on a directory with autofs mounted. This update adds a new acl function, acl_extended_file_nofollow(), to prevent unnecessary autofs mounts.
Story Points: ---
Clone Of: 692823
: 733979 (view as bug list)
Environment:
Last Closed: 2011-12-06 12:12:44 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
fixup for the regression caused by the original patch V2 (2.26 KB, patch)
2011-10-02 04:27 EDT, Kamil Dudka
ovasik: review+
Details | Diff

  None (edit)
Comment 2 RHEL Product and Program Management 2011-07-11 08:18:14 EDT
This request was evaluated by Red Hat Product Management for
inclusion in the current release of Red Hat Enterprise Linux.
Because the affected component is not scheduled to be updated
in the current release, Red Hat is unfortunately unable to
address this request at this time. Red Hat invites you to
ask your support representative to propose this request, if
appropriate and relevant, in the next release of Red Hat
Enterprise Linux. If you would like it considered as an
exception in the current release, please ask your support
representative.
Comment 6 Kamil Dudka 2011-07-25 09:27:21 EDT
upstream commit:

http://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=95f7c57
Comment 9 Kamil Dudka 2011-08-17 01:22:38 EDT
coreutils needs to be rebuilt against libacl-devel >= 2.2.49-6:

checking for acl_extended_file... yes
checking for acl_extended_file_nofollow... no

spotted while examining bug #720318 comment #9
Comment 13 Eliska Slobodova 2011-09-19 10:22:29 EDT
    Technical note added. If any revisions are required, please edit the "Technical Notes" field
    accordingly. All revisions will be proofread by the Engineering Content Services team.
    
    New Contents:
Prior to this update, the acl_extended_file() function could cause unnecessary mounts of autofs when using the ls command on a directory with autofs mounted. This update adds a new acl function, acl_extended_file_nofollow(), to prevent unnecessary autofs mounts.
Comment 16 Ondrej Moriš 2011-10-02 04:19:01 EDT
I would prefer respin as well, moreover, I can confirm that attached patch resolves acl issue mentioned 11607#c28 and, at the same time, keeps coreutils selftest passing (however I noticed that some tests are SKIPPED).
Comment 17 Kamil Dudka 2011-10-02 04:27:10 EDT
Created attachment 525910 [details]
fixup for the regression caused by the original patch V2
Comment 19 Ondrej Vasik 2011-10-02 04:45:17 EDT
Comment on attachment 525910 [details]
fixup for the regression caused by the original patch V2

Thanks for the patch Kamil, looks almost sane, only #if HAVE_ACL_EXTENDED_FILE_NOFOLLOW should be # if HAVE_ACL_EXTENDED_FILE_NOFOLLOW - for the coding style conformance.
Comment 20 Kamil Dudka 2011-10-02 04:49:36 EDT
Indeed, will reflect that in the upstream submission next week.
Comment 21 Kamil Dudka 2011-10-02 05:12:43 EDT
(In reply to comment #20)
> Indeed, will reflect that in the upstream submission next week.

Also the acl_extended_file_wrap function should be declared static...
Comment 22 Kamil Dudka 2011-10-03 06:34:31 EDT
sent upstream:

http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/28538
Comment 23 Kamil Dudka 2011-10-03 13:34:02 EDT
Ian, do we need to treat symlinks to mount points anyhow specially in this fix?  Or is it just fine to issue getxattr syscalls on them?

There was a discussion about them on the upstream mailing list:

http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/28538/focus=28555
Comment 24 Ian Kent 2011-10-03 23:25:13 EDT
(In reply to comment #23)
> Ian, do we need to treat symlinks to mount points anyhow specially in this fix?
>  Or is it just fine to issue getxattr syscalls on them?

I don't think so because the semantic behaviour is (was) supposed
to follow that of symlinks. So any time there's a follow implied
by a system call an automount should be transparently (to user
space) mounted. 

> 
> There was a discussion about them on the upstream mailing list:
> 
> http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/28538/focus=28555

>> But still still does not fix the problem of case f). Here, with your current
>> patch, "ls -lLd symlink-to-mountpoint" will call acl_extended_file(), which
>> will call getxattr(), which will activate an autofs mount.

Which, with the intended semantics, is what I think should
happen. In this case causing everything within the directory
to be mounted would be considered a deliberate request by the
user.

>> IMO, there are two solutions to this:
>>  - Either convince the kernel people to introduce a different flag, keeping
>>    LOOKUP_FOLLOW for symlinks only,

There has been some spirited debate over this change upstream
recently related to the overloading of the LOOKUP_FOLLOW flag.

After some opposition we ended up with a LOOKUP_AUTOMOUNT flag
but, because it was felt that changing semantic behaviour would
be too problematic for user space (even though this is the only
case we have seen in over six months), the semantics have been
changed back to what they were previously from 3.1.0 (which is
obviously too late for RHEL-6.2).

I still believe the semantic behaviour change is the right
choice for automounting and will, at some point, be looking
to change it back again. But that will mean addition of things
like IS_AUTO(mode) etc. as well as related package modifications
and overcoming the existing opposition to the change.

Sorry for the inconvenience this has caused.
Ian
Comment 25 Ian Kent 2011-10-03 23:38:54 EDT
> Well, symlinks to mount points were not mentioned in that bug.  I would need 
> to ask kernel guys whether symlinks to mount points are worth to consider in 
> this fix at all.

I don't think this is really a consideration.

The point of the change was to be able to long list directory
contents of an automount managed directory. In that case
stat()ing a symlink to a mount point a mount would occur which
is what should happen IMHO.

The directory listing case should consistently use the nofollow
variants of system calls and it was agreed that behaviour was
the right way to do it in the beginning of this bug.
Comment 26 Ian Kent 2011-10-04 00:15:32 EDT
>   1) understand better which file system operations triggered the autofs
>      mounts, I can't really beleive that calling getxattr on a non-symlink
>      will trigger autofs mounts.

Ummm ... what!

If it isn't a symlink then it might be a directory and it might
be an automount point. So IMO it should be mounted so the caller
gets the information of the file system it is expecting without
it changing under it.

This is the no-win situation that has plagued us for a long
time. For example,

With the semantics that I believe are needed:
getxattr(), followed by open(), followed by getxattr() will
return the same attributes for both getxattr() calls (good
behaviour).

With the pre-existing semantics:
getxattr(), followed by open(), followed by getxattr() can
return different attributes for getxattr() (bad behaviour).

With either semantics:
lgetxattr(), followed by open(), followed by lgetxattr() can
return different attributes for getxattr() (bad unavoidable
behaviour).

So the issues go on and on. My real goal, and the only
realistic one, is to minimize the bad behaviour since we can't
just always automount every time we hit a mount point due to
mount storm type problems.

Ian
Comment 27 Ian Kent 2011-10-04 00:19:36 EDT
(In reply to comment #26)
> 
> With either semantics:
> lgetxattr(), followed by open(), followed by lgetxattr() can
> return different attributes for getxattr() (bad unavoidable
> behaviour).

Those last two lines should read:
return different attributes for lgetxattr() (bad unavoidable
behaviour).
Comment 33 errata-xmlrpc 2011-12-06 12:12:44 EST
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHBA-2011-1693.html

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