Bug 719607

Summary: automount does not work on "ls -ld /misc/cd" but it does work on "test -d /misc/cd"
Product: [Fedora] Fedora Reporter: Joachim Jaeckel <bugzilla-2004>
Component: autofsAssignee: Ian Kent <ikent>
Status: CLOSED WONTFIX QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: 15CC: bugzilla-2004, dhowells, ikent, jlayton, leonardo, mjt
Target Milestone: ---   
Target Release: ---   
Hardware: x86_64   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-08-07 20:20:23 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 692823, 692982, 720318, 720325, 733978, 733979    
Bug Blocks:    
Attachments:
Description Flags
Testprogram with lstat and stat
none
Unconditionally automount on negative dentries
none
Unconditionally automount on negative dentries V2 none

Description Joachim Jaeckel 2011-07-07 13:09:58 UTC
Created attachment 511691 [details]
Testprogram with lstat and stat

Description of problem:

automount does not mount when issuing e.g.
  ls -l /misc/cd
but it works when issuing
  test -d /misc/cd

With strace i see the difference between ls and test:
  ls calls lstat(2),
  test calls stat(2)

I wrote a small C program to reproduce this issue, see attachment.
  lstat("/misc/cd",...) fails
  stat("/misc/cd",...) trigges automount, /misc/cd will be mounted
  



Version-Release number of selected component (if applicable):
  autofs-5.0.5-38.fc15.x86_64
  kernel-2.6.38.8-32.fc15.x86_64


How reproducible:
  always

Steps to Reproduce:
1. activate default /etc/auto.misc in /etc/auto.master
2. service autofs restart
3. ls -ld /misc/cd
4. test -d /misc/cd

or:
3. Compile the test program: make automount_test
4. run the testprogram: ./automount_test
  
Actual results:
(restart autofs)
[jj@vega ~]]$ ls -ld /misc/cd
ls: cannot access /misc/cd: No such file or directory
[jj@vega ~]]$ test -d /misc/cd
[jj@vega ~]]$ ls -ld /misc/cd
dr-xr-xr-x 13 root root 4096 Apr 27 12:49 /misc/cd
[jj@vega ~]]$
(restart autofs)
[jj@vega automount]$ make automount_test
cc     automount_test.c   -o automount_test
[jj@vega automount]$ ./automount_test
lstat rc=-1
ls: cannot access /misc/cd: No such file or directory
stat rc=0
dr-xr-xr-x 13 root root 4096 Apr 27 12:49 /misc/cd
lstat rc=0
dr-xr-xr-x 13 root root 4096 Apr 27 12:49 /misc/cd
[jj@vega automount]$


Expected results:
  ls should trigger automount (worked in former versions)

Additional info:

Comment 1 Ian Kent 2011-07-07 15:10:52 UTC
(In reply to comment #0)
> Created attachment 511691 [details]
> Testprogram with lstat and stat
> 
> Description of problem:
> 
> automount does not mount when issuing e.g.
>   ls -l /misc/cd
> but it works when issuing
>   test -d /misc/cd
> 
> With strace i see the difference between ls and test:
>   ls calls lstat(2),
>   test calls stat(2)
> 
> I wrote a small C program to reproduce this issue, see attachment.
>   lstat("/misc/cd",...) fails
>   stat("/misc/cd",...) trigges automount, /misc/cd will be mounted

OK, that's the second time this behaviour has been reported
against 2.6.38+ kernels.

The semantics have changed a little in 2.6.38+ but, according 
to the code the opposite should happen which is puzzling me.

I must be missing some other case where the behaviour has
changed. Let me have a think about it and a look around.

> 
> Version-Release number of selected component (if applicable):
>   autofs-5.0.5-38.fc15.x86_64

Your using the default installed autofs configuration,
right?

>   kernel-2.6.38.8-32.fc15.x86_64
> 
> 
> How reproducible:
>   always
> 
> Steps to Reproduce:
> 1. activate default /etc/auto.misc in /etc/auto.master
> 2. service autofs restart
> 3. ls -ld /misc/cd

Adding a "/" on the end should cause the mount to occur.

> 4. test -d /misc/cd
> 
> or:
> 3. Compile the test program: make automount_test
> 4. run the testprogram: ./automount_test

And the same here.

Ian

Comment 2 Joachim Jaeckel 2011-07-07 15:37:41 UTC
> Your using the default installed autofs configuration,
right?
Yes.

> Adding a "/" on the end should cause the mount to occur.
No:
  ls -l /misc/cd/         # no mount occurs
But:
  ls -l /misc/cd/xyz      # mount occurs, xyz may be any characters

Simple way to see the lstat:
[root@vega ~]# service autofs restart
Restarting autofs (via systemctl):                         [  OK  ]
[root@vega ~]# strace ls -l /misc/cd 2>&1 | grep /misc/
execve("/bin/ls", ["ls", "-l", "/misc/cd"], [/* 28 vars */]) = 0
lstat("/misc/cd", 0x256d1b0)            = -1 ENOENT (No such file or directory)
write(2, "cannot access /misc/cd", 22cannot access /misc/cd)  = 22
[root@vega ~]# strace ls -l /misc/cd/ 2>&1 | grep /misc/
execve("/bin/ls", ["ls", "-l", "/misc/cd/"], [/* 28 vars */]) = 0
lstat("/misc/cd/", 0xe481b0)            = -1 ENOENT (No such file or directory)
write(2, "cannot access /misc/cd/", 23cannot access /misc/cd/) = 23
[root@vega ~]# strace ls -l /misc/cd/abc 2>&1 | grep /misc/
execve("/bin/ls", ["ls", "-l", "/misc/cd/abc"], [/* 28 vars */]) = 0
lstat("/misc/cd/abc", 0x26001b0)        = -1 ENOENT (No such file or directory)
write(2, "cannot access /misc/cd/abc", 26cannot access /misc/cd/abc) = 26

# last statement will mount, lstat fails because /abc doesnt exist on my CD

Joachim

Comment 3 David Howells 2011-07-07 15:57:16 UTC
Can you try the following:

    strace -etrace=lstat,stat,getxattr,lgetxattr ls -l /misc/cd

and paste the result?

Also, can you paste the result of the following:

    rpm -q libacl coreutils

What used to happen was that ls called into libacl to check the ACL - and that used getxattr() thereby causing the mount anyway.  This was found recently [bug 692823] and the fix may have gotten into Fedora 15, though not yet Fedora 14, in which case this will no longer cause an automount.  The idea is to prevent "ls -l /misc" causing an automount storm in /misc has a lot of mountpoints in it.

The problem is that "ls -l /misc" and "ls -l /misc/cd" both lstat, lgetxattr (and may getxattr if an old libacl) /misc/cd - so you can't tell them apart.

Comment 4 Joachim Jaeckel 2011-07-07 16:10:57 UTC
(In reply to comment #3)

>     strace -etrace=lstat,stat,getxattr,lgetxattr ls -l /misc/cd
[root@vega ~]# strace -etrace=lstat,stat,getxattr,lgetxattr ls -l /misc/cd
lstat("/misc/cd", 0x11341b0)            = -1 ENOENT (No such file or directory)
ls: cannot access /misc/cd: No such file or directory
[root@vega ~]# 

>     rpm -q libacl coreutils
libacl-2.2.49-9.fc15.x86_64
coreutils-8.10-2.fc15.x86_64


> What used to happen was that ls called into libacl to check the ACL - and that
> used getxattr() thereby causing the mount anyway.  This was found recently [bug
> 692823] and the fix may have gotten into Fedora 15, though not yet Fedora 14,
> in which case this will no longer cause an automount.  The idea is to prevent
> "ls -l /misc" causing an automount storm in /misc has a lot of mountpoints in
> it.
> 
> The problem is that "ls -l /misc" and "ls -l /misc/cd" both lstat, lgetxattr
> (and may getxattr if an old libacl) /misc/cd - so you can't tell them apart.

I have several mounts defined in /etc/auto.misc
But my /misc directory contains only subdirectories for the mount points wich are actually mounted by autofs.
When automount has dismounted all fs after timeout, /misc is empty.

Joachim

Comment 5 Ian Kent 2011-07-07 16:39:53 UTC
(In reply to comment #4)
> I have several mounts defined in /etc/auto.misc
> But my /misc directory contains only subdirectories for the mount points wich
> are actually mounted by autofs.
> When automount has dismounted all fs after timeout, /misc is empty.

Yes, in this case, with previous kernels, automount would
always mount when called with "/misc/<key>".

If the configuration had BROWSE_MODE="yes" or if that
configuration entry was commented out then "/misc/<key>" would
not cause a mount regardless of whether you used the l variant
of the system call or not.

Comment 6 Ian Kent 2011-07-07 16:43:18 UTC
(In reply to comment #2)
> > Your using the default installed autofs configuration,
> right?
> Yes.
> 
> > Adding a "/" on the end should cause the mount to occur.
> No:
>   ls -l /misc/cd/         # no mount occurs
> But:
>   ls -l /misc/cd/xyz      # mount occurs, xyz may be any characters
> 
> Simple way to see the lstat:
> [root@vega ~]# service autofs restart
> Restarting autofs (via systemctl):                         [  OK  ]
> [root@vega ~]# strace ls -l /misc/cd 2>&1 | grep /misc/
> execve("/bin/ls", ["ls", "-l", "/misc/cd"], [/* 28 vars */]) = 0
> lstat("/misc/cd", 0x256d1b0)            = -1 ENOENT (No such file or directory)
> write(2, "cannot access /misc/cd", 22cannot access /misc/cd)  = 22
> [root@vega ~]# strace ls -l /misc/cd/ 2>&1 | grep /misc/
> execve("/bin/ls", ["ls", "-l", "/misc/cd/"], [/* 28 vars */]) = 0
> lstat("/misc/cd/", 0xe481b0)            = -1 ENOENT (No such file or directory)
> write(2, "cannot access /misc/cd/", 23cannot access /misc/cd/) = 23
> [root@vega ~]# strace ls -l /misc/cd/abc 2>&1 | grep /misc/
> execve("/bin/ls", ["ls", "-l", "/misc/cd/abc"], [/* 28 vars */]) = 0
> lstat("/misc/cd/abc", 0x26001b0)        = -1 ENOENT (No such file or directory)
> write(2, "cannot access /misc/cd/abc", 26cannot access /misc/cd/abc) = 26
> 
> # last statement will mount, lstat fails because /abc doesnt exist on my CD

OK, I'll need to think about this a bit (and run some tests)
because not all of these cases are what I expect to happen with
the current kernel code.

Ian

Comment 7 Ian Kent 2011-07-08 03:32:09 UTC
David I seem to be misunderstanding the test in
fs/namei.c:follow_automount() so I've been looking at it.

Forgetting what the test actually does for a bit and thinking
about what we should do I've come up with this.

First the terminology, the "follow link" in system call
descriptions equates to "follow mount" for (us in)
vfs-automount.

So for system calls that do not follow the link (the l
calls) we should work on the mountpoint. So for this case
we shouldn't call ->d_automount().

In a semantically perfect world system calls that do
follow the link should call ->d_automount(). But we know
that isn't practical due to mount storms when long listing
the contents of directories that contain existing mount point
directories because user space will do a bunch of stat, getxattr
and the like, and, as we have seen maybe lgetxattr.

Assuming the above is what we want, returning to the test, I
think the test in follow_automount() is not quite right and
the above amounts to saying the && should actually be an ||
like:

       if (!(flags & LOOKUP_FOLLOW) ||
            !(flags & (LOOKUP_CONTINUE | LOOKUP_DIRECTORY |
                       LOOKUP_OPEN | LOOKUP_CREATE)))
                return -EISDIR;

which says, if we are not following a link return -EISDIR, if
we are following a link return -EISDIR if we are not evaluating
an intermediate path component or we are have not been called
by an open or create type system call, otherwise call
->d_automount().

I think this matches the previous kernel semantics for positive
dentrys. Have I still got this wrong somehow? If I haven't we can
close 692823 as NOTABUG with this change.

The other case, for negative dentrys, which is the cause of the
problem here, the situation is a bit different. For NFS, and I
believe CIFS (Jeff?), the xdev dentrys will always be positive
when we get to follow them since they must have a directory within
the mount to cross. What's the situation for AFS David?

But, in previous kernels, autofs would unconditionally trigger a
mount in ->lookup() which amounts to saying vfs-automount()
should always call ->d_automount() for negative dentrys (at least
for autofs). IOW the test should be:

       if (path->dentry) {
            if (!(flags & LOOKUP_FOLLOW) ||
                !(flags & (LOOKUP_CONTINUE | LOOKUP_DIRECTORY |
                           LOOKUP_OPEN | LOOKUP_CREATE)))
                    return -EISDIR;
            }
       }

or equivalent.

What do you guys think about my reasoning and would this break
AFS, CIFS or NFS?

Ian

Comment 8 Ian Kent 2011-07-08 06:32:28 UTC
(In reply to comment #7)

Oops!

> The other case, for negative dentrys, which is the cause of the
> problem here, the situation is a bit different. For NFS, and I
> believe CIFS (Jeff?), the xdev dentrys will always be positive
> when we get to follow them since they must have a directory within
> the mount to cross. What's the situation for AFS David?
> 
> But, in previous kernels, autofs would unconditionally trigger a
> mount in ->lookup() which amounts to saying vfs-automount()
> should always call ->d_automount() for negative dentrys (at least
> for autofs). IOW the test should be:
> 
>        if (path->dentry) {

That's path->dentry->d_inode, of course.

>             if (!(flags & LOOKUP_FOLLOW) ||
>                 !(flags & (LOOKUP_CONTINUE | LOOKUP_DIRECTORY |
>                            LOOKUP_OPEN | LOOKUP_CREATE)))
>                     return -EISDIR;
>             }

And this } isn't needed.

>        }

Comment 9 David Howells 2011-07-08 09:20:44 UTC
(In reply to comment #7)
> David I seem to be misunderstanding the test in
> fs/namei.c:follow_automount() so I've been looking at it.

To make it easier to understand, invert the logic and use gotos:

	if (flags & LOOKUP_FOLLOW)
		goto do_mount;
	if (flags & (LOOKUP_CONTINUE | LOOKUP_DIRECTORY |
		     LOOKUP_OPEN | LOOKUP_CREATE))
		goto do_mount;
	return -EISDIR;
    do_mount:

You have to remember that LOOKUP_FOLLOW is !AT_SYMLINK_NOFOLLOW, so you can read the original as: if AT_SYMLINK_NOFOLLOW _is_ set _and_ we are at the end of the path (!CONTINUE), we're not trying to open a directory (!DIRECTORY) and we're not trying to open/create a file (!OPEN/!CREATE), _then_ use the mountpoint dir - otherwise use the mounted directory.  This results in the following:

> ... because user space will do a bunch of stat, getxattr
> and the like, and, as we have seen maybe lgetxattr.

Just to note, stat/getxattr -> LOOKUP_FOLLOW; lstat/lgetxattr -> don't set it.

> Assuming the above is what we want, returning to the test, I
> think the test in follow_automount() is not quite right and
> the above amounts to saying the && should actually be an ||
> like:
> 
>        if (!(flags & LOOKUP_FOLLOW) ||
>             !(flags & (LOOKUP_CONTINUE | LOOKUP_DIRECTORY |
>                        LOOKUP_OPEN | LOOKUP_CREATE)))
>                 return -EISDIR;
> 
> which says, if we are not following a link return -EISDIR, if
> we are following a link return -EISDIR if we are not evaluating
> an intermediate path component or we are have not been called
> by an open or create type system call, otherwise call
> ->d_automount().

Unfortunately, it doesn't work like that.  What your change does is prevent _all_ automounts (even on intermediate components) if LOOKUP_FOLLOW is not set.

> But, in previous kernels, autofs would unconditionally trigger a
> mount in ->lookup() which amounts to saying vfs-automount()
> should always call ->d_automount() for negative dentrys (at least
> for autofs). IOW the test should be:
> 
>        if (path->dentry) {
>             if (!(flags & LOOKUP_FOLLOW) ||
>                 !(flags & (LOOKUP_CONTINUE | LOOKUP_DIRECTORY |
>                            LOOKUP_OPEN | LOOKUP_CREATE)))
>                     return -EISDIR;
>             }
>        }
> 
> or equivalent.

I think what you probably want is to further qualify the original statement:

	if (!(flags & LOOKUP_FOLLOW) && path->dentry->d_inode &&
	    !(flags & (LOOKUP_CONTINUE | LOOKUP_DIRECTORY |
		       LOOKUP_OPEN | LOOKUP_CREATE)))
		return -EISDIR;

Comment 10 David Howells 2011-07-08 09:33:24 UTC
Created attachment 511876 [details]
Unconditionally automount on negative dentries

Comment 11 Ian Kent 2011-07-08 09:41:26 UTC
(In reply to comment #9)
> > 
> > which says, if we are not following a link return -EISDIR, if
> > we are following a link return -EISDIR if we are not evaluating
> > an intermediate path component or we are have not been called
> > by an open or create type system call, otherwise call
> > ->d_automount().
> 
> Unfortunately, it doesn't work like that.  What your change does is prevent
> _all_ automounts (even on intermediate components) if LOOKUP_FOLLOW is not set.
> 

Yes, you're right.

> > But, in previous kernels, autofs would unconditionally trigger a
> > mount in ->lookup() which amounts to saying vfs-automount()
> > should always call ->d_automount() for negative dentrys (at least
> > for autofs). IOW the test should be:
> > 
> >        if (path->dentry) {
> >             if (!(flags & LOOKUP_FOLLOW) ||
> >                 !(flags & (LOOKUP_CONTINUE | LOOKUP_DIRECTORY |
> >                            LOOKUP_OPEN | LOOKUP_CREATE)))
> >                     return -EISDIR;
> >             }
> >        }
> > 
> > or equivalent.
> 
> I think what you probably want is to further qualify the original statement:
> 
>  if (!(flags & LOOKUP_FOLLOW) && path->dentry->d_inode &&
>      !(flags & (LOOKUP_CONTINUE | LOOKUP_DIRECTORY |
>          LOOKUP_OPEN | LOOKUP_CREATE)))
>   return -EISDIR;

Yes, that would resolve the problem here.

But, from the testing I did, there are cases where the logic is
inverted, aka. stat() causes a mount and lstat() does not, which
I thought is the opposite to what we were after.

Let me build a kernel with the patch above and test again, it
may just be the negative dentry that is causing the confusion.

Ian

Comment 12 Ian Kent 2011-07-11 05:43:19 UTC
(In reply to comment #11)
> > 
> > I think what you probably want is to further qualify the original statement:
> > 
> >  if (!(flags & LOOKUP_FOLLOW) && path->dentry->d_inode &&
> >      !(flags & (LOOKUP_CONTINUE | LOOKUP_DIRECTORY |
> >          LOOKUP_OPEN | LOOKUP_CREATE)))
> >   return -EISDIR;
> 
> Yes, that would resolve the problem here.
> 
> But, from the testing I did, there are cases where the logic is
> inverted, aka. stat() causes a mount and lstat() does not, which
> I thought is the opposite to what we were after.
> 
> Let me build a kernel with the patch above and test again, it
> may just be the negative dentry that is causing the confusion.

My thinking was backward, which I realized after looking at
bug 692823 again.

The recommend patch does seem to do the trick, as long as I
use updated coreutils (bug 692823) and acl (bug 692982) packages.

The test kernel I used should still be available here:
https://koji.fedoraproject.org/koji/taskinfo?taskID=3186328

Try it out and we'll work out from there if you need updated
coreutils and acl packages. You probably don't need them unless
you are using the browse autofs mount option and you have maps
that are largish is size.

Ian

Comment 13 Joachim Jaeckel 2011-07-11 08:51:20 UTC
> The test kernel I used should still be available here:
> https://koji.fedoraproject.org/koji/taskinfo?taskID=3186328
> 
> Try it out and we'll work out from there if you need updated
> coreutils and acl packages. You probably don't need them unless
> you are using the browse autofs mount option and you have maps
> that are largish is size.
> 
> Ian

Cannot test it this week because i am on holidays. Will be back Jul 19.

Joachim

Comment 14 David Howells 2011-07-11 13:10:05 UTC
Created attachment 512209 [details]
Unconditionally automount on negative dentries V2

Simply switch the automount negative dentry check to be the second if-statement as the the LOOKUP_* flag check is much more likely to jump.

Comment 15 Joachim Jaeckel 2011-07-19 16:37:18 UTC
> My thinking was backward, which I realized after looking at
> bug 692823 again.
> 
> The recommend patch does seem to do the trick, as long as I
> use updated coreutils (bug 692823) and acl (bug 692982) packages.
> 
> The test kernel I used should still be available here:
> https://koji.fedoraproject.org/koji/taskinfo?taskID=3186328
> 
> Try it out and we'll work out from there if you need updated
> coreutils and acl packages. You probably don't need them unless
> you are using the browse autofs mount option and you have maps
> that are largish is size.
> 
> Ian

For testing i installed the test kernel from the above url.

kernel-2.6.38.8-32.bz708039.2.fc15.x86_64
libacl-2.2.49-9.fc15.x86_64
coreutils-8.10-2.fc15.x86_64

Behaviour is as expected: automount is working again with ls -l /misc/cd

Joachim

Comment 16 Michael Tokarev 2011-08-16 07:07:16 UTC
I come across this issue too, after trying to bisect and realizing the first released "bad" kernel is 2.6.38.  I tested this patch on 2.6.38 and 3.0 and found that it works as expected, restoring autofs functionality as it were before.

Comment 17 Chuck Ebbert 2011-08-25 05:10:52 UTC
Fix will be in 2.6.40.3-2

Comment 18 Joachim Jaeckel 2011-09-10 16:33:09 UTC
Tested with actual Fedora 15, kernel-2.6.40.4-5.fc15.x86_64

Resolved.

Comment 19 Fedora End Of Life 2012-08-07 20:20:26 UTC
This message is a notice that Fedora 15 is now at end of life. Fedora
has stopped maintaining and issuing updates for Fedora 15. It is
Fedora's policy to close all bug reports from releases that are no
longer maintained. At this time, all open bugs with a Fedora 'version'
of '15' have been closed as WONTFIX.

(Please note: Our normal process is to give advanced warning of this
occurring, but we forgot to do that. A thousand apologies.)

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, feel free to reopen
this bug and simply change the 'version' to a later Fedora version.

Bug Reporter: Thank you for reporting this issue and we are sorry that
we were unable to fix it before Fedora 15 reached end of life. If you
would still like to see this bug fixed and are able to reproduce it
against a later version of Fedora, you are encouraged to click on
"Clone This Bug" (top right of this page) and open it against that
version of Fedora.

Although we aim to fix as many bugs as possible during every release's
lifetime, sometimes those efforts are overtaken by events. Often a
more recent Fedora release includes newer upstream software that fixes
bugs or makes them obsolete.

The process we are following is described here:
http://fedoraproject.org/wiki/BugZappers/HouseKeeping