Revert the following change:
commit 0ec26fd0698285b31248e34bf1abb022c00f23d6
Author: Miklos Szeredi <[email protected]>
Date: Mon Sep 5 18:06:26 2011 +0200
Subject: vfs: automount should ignore LOOKUP_FOLLOW
which makes stat(), getxattr(), etc. behave as lstat(), lgetxattr(), etc. with
respect to automounting: it prevents them from triggering terminal automounts.
The problem with this is that this breaks nfs_follow_remote_path() used by NFS4
to find the root object to mount for the mount() syscall.
This can be tested by the following procedure:
(1) Export a directory from an NFS4 server that has something mounted upon it
and that isn't "/" - say, for example, "/scratch". It must have a
different FSID from "/".
(2) Mount this on the client.
mount sirius:/scratch /mnt
(3) List the contents of the client's mountpoint:
ls /mnt
(4) Examine /proc/fs/nfsfs/volumes. You should see one entry (assuming
nothing else is NFS mounted) :
NV SERVER PORT DEV FSID FSC
v4 200108b001940000021125fffe84efad 801 0:34 1:0 no
which corresponds to the filesystem mounted on /scratch on the server.
If, however, you see two entries:
NV SERVER PORT DEV FSID FSC
v4 200108b001940000021125fffe84efad 801 0:33 0:0 no
v4 200108b001940000021125fffe84efad 801 0:34 1:0 no
then it went wrong.
When it goes wrong, what happens is that nfs_follow_remote_path() walks from
the rootfh to the remote directory (/scratch) using vfs_path_lookup(). It
passes LOOKUP_FOLLOW to vfs_path_lookup() to tell it to transit terminal
symlinks and terminal automounts. Unfortunately, with the aforementioned
commit, LOOKUP_FOLLOW now expressly does not follow terminal automounts.
There are some alternatives:
(1) Fix up userspace to expect stat(), getxattr(), etc. to expect these to
cause path-terminal automounting. libacl and coreutils (ls) have been
fixed up upstream already but there are other things such as GUI
filemanagers.
(2) Passing LOOKUP_DIRECTORY from nfs_follow_remote_path() to
vfs_path_lookup() might work as presumably the mount root will be a
directory.
(3) Add a new flag, say LOOKUP_AUTOMOUNT, to force automounting. This could
be extended to userspace.
(4) Add a new flag, say LOOKUP_GETTING_ATTRS, to indicate that we're doing a
stat() or a getxattr() and suppress automounting on that basis.
Reported-by: Jeff Layton <[email protected]>
Signed-off-by: David Howells <[email protected]>
---
fs/namei.c | 33 ++++++++++++++++++---------------
1 files changed, 18 insertions(+), 15 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index f478836..c64b81d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -727,22 +727,25 @@ static int follow_automount(struct path *path, unsigned flags,
if ((flags & LOOKUP_NO_AUTOMOUNT) && !(flags & LOOKUP_PARENT))
return -EISDIR; /* we actually want to stop here */
- /* We don't want to mount if someone's just doing a stat -
- * unless they're stat'ing a directory and appended a '/' to
- * the name.
- *
- * We do, however, want to mount if someone wants to open or
- * create a file of any type under the mountpoint, wants to
- * traverse through the mountpoint or wants to open the
- * mounted directory. Also, autofs may mark negative dentries
- * as being automount points. These will need the attentions
- * of the daemon to instantiate them before they can be used.
+ /*
+ * We don't want to mount if someone's just doing a stat and they've
+ * set AT_SYMLINK_NOFOLLOW - unless they're stat'ing a directory and
+ * appended a '/' to the name.
*/
- if (!(flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY |
- LOOKUP_OPEN | LOOKUP_CREATE)) &&
- path->dentry->d_inode)
- return -EISDIR;
-
+ if (!(flags & LOOKUP_FOLLOW)) {
+ /* We do, however, want to mount if someone wants to open or
+ * create a file of any type under the mountpoint, wants to
+ * traverse through the mountpoint or wants to open the mounted
+ * directory.
+ * Also, autofs may mark negative dentries as being automount
+ * points. These will need the attentions of the daemon to
+ * instantiate them before they can be used.
+ */
+ if (!(flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY |
+ LOOKUP_OPEN | LOOKUP_CREATE)) &&
+ path->dentry->d_inode)
+ return -EISDIR;
+ }
current->total_link_count++;
if (current->total_link_count >= 40)
return -ELOOP;
On Thu, 22 Sep 2011 16:22:05 +0100
David Howells <[email protected]> wrote:
>
> One thing that does concern me a little about bringing back the old behaviour
> for stat and *xattr is that there is no reliable way to get at the directory
> mounted directly on the mountpoint (ie. mnt_root).
>
> The problem is that:
>
> (a) you have to know to trigger the automount with some other operation first
> and
>
> (b) the other operation is not atomic wrt to the following stat/xattr, and so
> the thing mounted on the automount point may be subject to expiration and
> auto-unmounting before the second operation can proceed.
>
> I'm not sure how much of a problem these are in practice. Certainly, I'd rate
> (b) as being less serious than (a) as the expiration timeouts are generally
> quite large. (b) can be suppressed with chdir() or open() also - then you are
> forcing retension of the vfsmount.
>
> The problem can be ameliorated for such as fstatat() by passing an AT_ flag to
> either suppress automounting or to force automounting, but there aren't any
> xattr calls, for example, that take this.
>
>
> I guess it comes down to defining two things:
>
> (1) What behaviour do we actually want?
>
> (2) What departure from previous behaviour are we willing to put up with?
>
>
> On the first, I would say definitely we want mass-stat'ers (such as ls) to not
> cause mass automounting. But for ls, that has been achieved in userspace;
> there are, however, other programs to consider.
>
> I would also say that we do not want lstat(), l*xattr() and co. to cause
> automounting - but maybe they should. Perhaps if you don't want to cause
> automounting, you should explicitly pass AT_NO_AUTOMOUNT, and all path-taking
> VFS calls should have variants that accept this flag.
>
> Do we want to have guaranteed access to the root of an automounted fs? Are we
> willing to add getxattrat and similar to get it?
>
I guess we recognize that we ought to allow userspace to have some
control over whether these syscalls will trigger an automount. So the
real question is -- what should the default be? Probably, we ought to
err on the side of caution and keep the default behavior the way it was
before 2.6.38.
--
Jeff Layton <[email protected]>
David Howells <[email protected]> writes:
> Revert the following change:
>
> commit 0ec26fd0698285b31248e34bf1abb022c00f23d6
> Author: Miklos Szeredi <[email protected]>
> Date: Mon Sep 5 18:06:26 2011 +0200
> Subject: vfs: automount should ignore LOOKUP_FOLLOW
>
> which makes stat(), getxattr(), etc. behave as lstat(), lgetxattr(), etc. with
> respect to automounting: it prevents them from triggering terminal automounts.
>
> The problem with this is that this breaks nfs_follow_remote_path() used by NFS4
> to find the root object to mount for the mount() syscall.
>
> This can be tested by the following procedure:
>
> (1) Export a directory from an NFS4 server that has something mounted upon it
> and that isn't "/" - say, for example, "/scratch". It must have a
> different FSID from "/".
>
> (2) Mount this on the client.
>
> mount sirius:/scratch /mnt
>
> (3) List the contents of the client's mountpoint:
>
> ls /mnt
>
> (4) Examine /proc/fs/nfsfs/volumes. You should see one entry (assuming
> nothing else is NFS mounted) :
>
> NV SERVER PORT DEV FSID FSC
> v4 200108b001940000021125fffe84efad 801 0:34 1:0 no
>
> which corresponds to the filesystem mounted on /scratch on the server.
>
> If, however, you see two entries:
>
> NV SERVER PORT DEV FSID FSC
> v4 200108b001940000021125fffe84efad 801 0:33 0:0 no
> v4 200108b001940000021125fffe84efad 801 0:34 1:0 no
>
> then it went wrong.
>
> When it goes wrong, what happens is that nfs_follow_remote_path() walks from
> the rootfh to the remote directory (/scratch) using vfs_path_lookup(). It
> passes LOOKUP_FOLLOW to vfs_path_lookup() to tell it to transit terminal
> symlinks and terminal automounts. Unfortunately, with the aforementioned
> commit, LOOKUP_FOLLOW now expressly does not follow terminal
> automounts.
What is exactly happening here? By not following automounts how do we
end up with two mounts instead of one? I would have guessed the other
way round.
> There are some alternatives:
>
> (1) Fix up userspace to expect stat(), getxattr(), etc. to expect these to
> cause path-terminal automounting. libacl and coreutils (ls) have been
> fixed up upstream already but there are other things such as GUI
> filemanagers.
This is a distro solution, not a kernel solution.
> (2) Passing LOOKUP_DIRECTORY from nfs_follow_remote_path() to
> vfs_path_lookup() might work as presumably the mount root will be a
> directory.
Sounds like a big hack.
>
> (3) Add a new flag, say LOOKUP_AUTOMOUNT, to force automounting. This could
> be extended to userspace.
This is sane. Athough I can't tell if this is the right solution for
the NFSv4 problem without actually understanding what is happening
there.
>
> (4) Add a new flag, say LOOKUP_GETTING_ATTRS, to indicate that we're doing a
> stat() or a getxattr() and suppress automounting on that basis.
What's the difference between LOOKUP_NO_AUTOMOUNT and
LOOKUP_GETTING_ATTRS?
Thanks,
Miklos
David Howells <[email protected]> wrote:
> There are some alternatives:
(5) Have stat(), getxattr(), etc. supply LOOKUP_NO_AUTOMOUNT unconditionally.
David
David Howells <[email protected]> writes:
> One thing that does concern me a little about bringing back the old behaviour
> for stat and *xattr is that there is no reliable way to get at the directory
> mounted directly on the mountpoint (ie. mnt_root).
>
> The problem is that:
>
> (a) you have to know to trigger the automount with some other operation first
> and
>
> (b) the other operation is not atomic wrt to the following stat/xattr, and so
> the thing mounted on the automount point may be subject to expiration and
> auto-unmounting before the second operation can proceed.
>
> I'm not sure how much of a problem these are in practice. Certainly, I'd rate
> (b) as being less serious than (a) as the expiration timeouts are generally
> quite large. (b) can be suppressed with chdir() or open() also - then you are
> forcing retension of the vfsmount.
>
> The problem can be ameliorated for such as fstatat() by passing an AT_ flag to
> either suppress automounting or to force automounting, but there aren't any
> xattr calls, for example, that take this.
>
>
> I guess it comes down to defining two things:
>
> (1) What behaviour do we actually want?
>
> (2) What departure from previous behaviour are we willing to put up with?
>
>
> On the first, I would say definitely we want mass-stat'ers (such as ls) to not
> cause mass automounting. But for ls, that has been achieved in userspace;
> there are, however, other programs to consider.
>
> I would also say that we do not want lstat(), l*xattr() and co. to cause
> automounting - but maybe they should. Perhaps if you don't want to cause
> automounting, you should explicitly pass AT_NO_AUTOMOUNT, and all path-taking
> VFS calls should have variants that accept this flag.
>
> Do we want to have guaranteed access to the root of an automounted fs? Are we
> willing to add getxattrat and similar to get it?
Perhaps we should allow getxattr and friends to work on O_PATH opens to
get the same guarantees? E.g:
fd = open(path, O_PATH);
fgetxattr(fd, ...);
This needs a patch like the following.
Miklos
----
diff --git a/fs/xattr.c b/fs/xattr.c
index f060663..6540324 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -328,7 +328,7 @@ SYSCALL_DEFINE5(fsetxattr, int, fd, const char __user *, name,
struct dentry *dentry;
int error = -EBADF;
- f = fget(fd);
+ f = fget_raw(fd);
if (!f)
return error;
dentry = f->f_path.dentry;
@@ -414,7 +414,7 @@ SYSCALL_DEFINE4(fgetxattr, int, fd, const char __user *, name,
struct file *f;
ssize_t error = -EBADF;
- f = fget(fd);
+ f = fget_raw(fd);
if (!f)
return error;
audit_inode(NULL, f->f_path.dentry);
@@ -486,7 +486,7 @@ SYSCALL_DEFINE3(flistxattr, int, fd, char __user *, list, size_t, size)
struct file *f;
ssize_t error = -EBADF;
- f = fget(fd);
+ f = fget_raw(fd);
if (!f)
return error;
audit_inode(NULL, f->f_path.dentry);
@@ -555,7 +555,7 @@ SYSCALL_DEFINE2(fremovexattr, int, fd, const char __user *, name)
struct dentry *dentry;
int error = -EBADF;
- f = fget(fd);
+ f = fget_raw(fd);
if (!f)
return error;
dentry = f->f_path.dentry;
On Thu, 2011-09-22 at 16:22 +0100, David Howells wrote:
>
> I would also say that we do not want lstat(), l*xattr() and co. to cause
> automounting - but maybe they should. Perhaps if you don't want to cause
> automounting, you should explicitly pass AT_NO_AUTOMOUNT, and all path-taking
> VFS calls should have variants that accept this flag.
I still think we had it right the first time.
After all we have always said automounts are directories that have
symlink semantics, which to my way of thinking is what we now have (or
had). Not only that, it provides a natural way of ensuring a mount
occurs or does not occur by using the well known no follow calls.
The "natural" part of this is automounts can be followed conceptually in
the same way that symlinks can be followed.
I
One thing that does concern me a little about bringing back the old behaviour
for stat and *xattr is that there is no reliable way to get at the directory
mounted directly on the mountpoint (ie. mnt_root).
The problem is that:
(a) you have to know to trigger the automount with some other operation first
and
(b) the other operation is not atomic wrt to the following stat/xattr, and so
the thing mounted on the automount point may be subject to expiration and
auto-unmounting before the second operation can proceed.
I'm not sure how much of a problem these are in practice. Certainly, I'd rate
(b) as being less serious than (a) as the expiration timeouts are generally
quite large. (b) can be suppressed with chdir() or open() also - then you are
forcing retension of the vfsmount.
The problem can be ameliorated for such as fstatat() by passing an AT_ flag to
either suppress automounting or to force automounting, but there aren't any
xattr calls, for example, that take this.
I guess it comes down to defining two things:
(1) What behaviour do we actually want?
(2) What departure from previous behaviour are we willing to put up with?
On the first, I would say definitely we want mass-stat'ers (such as ls) to not
cause mass automounting. But for ls, that has been achieved in userspace;
there are, however, other programs to consider.
I would also say that we do not want lstat(), l*xattr() and co. to cause
automounting - but maybe they should. Perhaps if you don't want to cause
automounting, you should explicitly pass AT_NO_AUTOMOUNT, and all path-taking
VFS calls should have variants that accept this flag.
Do we want to have guaranteed access to the root of an automounted fs? Are we
willing to add getxattrat and similar to get it?
David
On Thu, Sep 22, 2011 at 5:46 AM, David Howells <[email protected]> wrote:
>
> ?(4) Add a new flag, say LOOKUP_GETTING_ATTRS, to indicate that we're doing a
> ? ? stat() or a getxattr() and suppress automounting on that basis.
I'd prefer this one. Except I'd just call it LOOKUP_STAT to make it
less cumbersome.
And then just have lstat/stat/xattr set that bit, and make it clear
that that bit means "no automount".
Linus
Miklos Szeredi <[email protected]> wrote:
> > When it goes wrong, what happens is that nfs_follow_remote_path() walks from
> > the rootfh to the remote directory (/scratch) using vfs_path_lookup(). It
> > passes LOOKUP_FOLLOW to vfs_path_lookup() to tell it to transit terminal
> > symlinks and terminal automounts. Unfortunately, with the aforementioned
> > commit, LOOKUP_FOLLOW now expressly does not follow terminal
> > automounts.
>
> What is exactly happening here? By not following automounts how do we
> end up with two mounts instead of one? I would have guessed the other
> way round.
Linux's NFS4 client performs a VFS path walk to resolve the path in the mount
devname to the root fh of the new mount. This differs from NFS2/3 where the
client asks the server's mountd to do it.
With the situation I described, where you're attempting to directly mount a
directory that's the root of a mount on the server (but not the root of the
whole filesystem tree), you have a situation where the FSID changes as you
walk through the tree.
So, for example, assume "/" is on fsid 0 and "/scratch" is on fsid 1.
Now, when NFS sees a change of fsid, it interpolates an automount point on the
directory in which it saw the fsid change:
/ (FSID0)
|
|
|
scratch (S_AUTOMOUNT set)
(FSID0)
and, when triggered, d_automount
will then create a new superblock for the new fsid and that will get mounted
on the automount point:
/ (FSID0)
|
|
|
scratch ::::::::::::/(FSID1)
(FSID0) |
|-----|-----|-----|
| | | |
a b c d
However, during the NFS4 root lookup pathwalk, because the root of fsid 1 is
terminal in the path, with your patch it will not trigger automounting because
none of LOOKUP_PARENT, _DIRECTORY, _OPEN or _CREATE are set. LOOKUP_FOLLOW
was set and *that* used to be sufficient to force the automount.
So what happens is that scratch from fsid 0 gets mounted rather than / from
fsid 1 and this is an automount point. It will cause / from fsid 1 to be
mounted eventually when someone goes into it, but then you have to do two
unmounts to get rid of it instead of one.
David
On Thu, 22 Sep 2011 07:33:43 -0700
Linus Torvalds <[email protected]> wrote:
> On Thu, Sep 22, 2011 at 5:46 AM, David Howells <[email protected]> wrote:
> >
> > (4) Add a new flag, say LOOKUP_GETTING_ATTRS, to indicate that we're doing a
> > stat() or a getxattr() and suppress automounting on that basis.
>
> I'd prefer this one. Except I'd just call it LOOKUP_STAT to make it
> less cumbersome.
>
> And then just have lstat/stat/xattr set that bit, and make it clear
> that that bit means "no automount".
>
Maybe LOOKUP_NO_AUTOMOUNT would be better in that case?
--
Jeff Layton <[email protected]>
David Howells <[email protected]> writes:
> Miklos Szeredi <[email protected]> wrote:
>
>> > When it goes wrong, what happens is that nfs_follow_remote_path() walks from
>> > the rootfh to the remote directory (/scratch) using vfs_path_lookup(). It
>> > passes LOOKUP_FOLLOW to vfs_path_lookup() to tell it to transit terminal
>> > symlinks and terminal automounts. Unfortunately, with the aforementioned
>> > commit, LOOKUP_FOLLOW now expressly does not follow terminal
>> > automounts.
>>
>> What is exactly happening here? By not following automounts how do we
>> end up with two mounts instead of one? I would have guessed the other
>> way round.
>
> Linux's NFS4 client performs a VFS path walk to resolve the path in the mount
> devname to the root fh of the new mount. This differs from NFS2/3 where the
> client asks the server's mountd to do it.
>
> With the situation I described, where you're attempting to directly mount a
> directory that's the root of a mount on the server (but not the root of the
> whole filesystem tree), you have a situation where the FSID changes as you
> walk through the tree.
>
> So, for example, assume "/" is on fsid 0 and "/scratch" is on fsid 1.
>
> Now, when NFS sees a change of fsid, it interpolates an automount point on the
> directory in which it saw the fsid change:
>
> / (FSID0)
> |
> |
> |
> scratch (S_AUTOMOUNT set)
> (FSID0)
>
> and, when triggered, d_automount
> will then create a new superblock for the new fsid and that will get mounted
> on the automount point:
>
> / (FSID0)
> |
> |
> |
> scratch ::::::::::::/(FSID1)
> (FSID0) |
> |-----|-----|-----|
> | | | |
> a b c d
>
> However, during the NFS4 root lookup pathwalk, because the root of fsid 1 is
> terminal in the path, with your patch it will not trigger automounting because
> none of LOOKUP_PARENT, _DIRECTORY, _OPEN or _CREATE are set. LOOKUP_FOLLOW
> was set and *that* used to be sufficient to force the automount.
>
> So what happens is that scratch from fsid 0 gets mounted rather than / from
> fsid 1 and this is an automount point. It will cause / from fsid 1 to be
> mounted eventually when someone goes into it, but then you have to do two
> unmounts to get rid of it instead of one.
Thanks for the explanation, it's clear now.
Miklos