Problems with crossmnt since 1.2.1

Problems with crossmnt since 1.2.1

[Iustin Pop]

Hi,

Since nfs-utils 1.2.1, there are some problems with crossmnt usage. See
Debian bug http://bugs.debian.org/567546, but in short the problem seems
to be that sub-mounts (/a/b) take the top-level mount (/a) options
instead of their own, due to a bug in how mountd generates the crossmnt
subexports.

I checked that reverting the write_secinfo changes in commit
bc0a6ab03089fc1ea4fea26ed9635c2cc918b01b fix the issue, but that might
only be a side effect, not the actual cause.

A short test:
- have /a and /a/b exported, with different flags (e.g. ro on /a, rw on
  /a/b)
- restart the mountd, clear exports, etc.
- try a mount on the client of /a/b, it gets readonly
- umount & remount, it's now r/w
- however, clearing the kernel export table (exportfs -f), makes the
  next mount again get read-only 

Disabling crossmnt fixes the issue completely, so I would venture to
guess that the subexports creation code has some issue, but I don't know
enough of this to be able to debug it.

thanks,
iustin

Re: Problems with crossmnt since 1.2.1

[J. Bruce Fields]

On Sun, Feb 28, 2010 at 11:06:43AM +0100, Iustin Pop wrote:
> Since nfs-utils 1.2.1, there are some problems with crossmnt usage. See
> Debian bug http://bugs.debian.org/567546, but in short the problem seems
> to be that sub-mounts (/a/b) take the top-level mount (/a) options
> instead of their own, due to a bug in how mountd generates the crossmnt
> subexports.
> 
> I checked that reverting the write_secinfo changes in commit
> bc0a6ab03089fc1ea4fea26ed9635c2cc918b01b fix the issue, but that might
> only be a side effect, not the actual cause.
> 
> A short test:
> - have /a and /a/b exported, with different flags (e.g. ro on /a, rw on
>   /a/b)
> - restart the mountd, clear exports, etc.
> - try a mount on the client of /a/b, it gets readonly
> - umount & remount, it's now r/w
> - however, clearing the kernel export table (exportfs -f), makes the
>   next mount again get read-only 
> 
> Disabling crossmnt fixes the issue completely, so I would venture to
> guess that the subexports creation code has some issue, but I don't know
> enough of this to be able to debug it.

Thanks for the report.  What's the latest nfs-utils version you've
tested?

On a quick skim of the latest code I see one clear bug
(path[strlen(path)] will never be '/'!), but that should have caused
crossmnt to never get enforced at all rather than to override anything.

Could you retest the latest from the upstream git, with this patch
applied, and see if the problem is still present?

Then if it is I'll try to go reproduce it myself....

--b.

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index d63e10a..ff27bbf 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -619,7 +619,7 @@ static int is_subdirectory(char *subpath, char *path)
 	int l = strlen(path);
 
 	return strcmp(subpath, path) == 0
-		|| (strncmp(subpath, path, l) == 0 && path[l] == '/');
+		|| (strncmp(subpath, path, l) == 0 && subpath[l] == '/');
 }
 
 static int path_matches(nfs_export *exp, char *path)

Re: Problems with crossmnt since 1.2.1

[Iustin Pop]

On Mon, Mar 01, 2010 at 03:40:10PM -0500, J. Bruce Fields wrote:
> On Sun, Feb 28, 2010 at 11:06:43AM +0100, Iustin Pop wrote:
> > Since nfs-utils 1.2.1, there are some problems with crossmnt usage. See
> > Debian bug http://bugs.debian.org/567546, but in short the problem seems
> > to be that sub-mounts (/a/b) take the top-level mount (/a) options
> > instead of their own, due to a bug in how mountd generates the crossmnt
> > subexports.
> > 
> > I checked that reverting the write_secinfo changes in commit
> > bc0a6ab03089fc1ea4fea26ed9635c2cc918b01b fix the issue, but that might
> > only be a side effect, not the actual cause.
> > 
> > A short test:
> > - have /a and /a/b exported, with different flags (e.g. ro on /a, rw on
> >   /a/b)
> > - restart the mountd, clear exports, etc.
> > - try a mount on the client of /a/b, it gets readonly
> > - umount & remount, it's now r/w
> > - however, clearing the kernel export table (exportfs -f), makes the
> >   next mount again get read-only 
> > 
> > Disabling crossmnt fixes the issue completely, so I would venture to
> > guess that the subexports creation code has some issue, but I don't know
> > enough of this to be able to debug it.
> 
> Thanks for the report.  What's the latest nfs-utils version you've
> tested?

1.2.2 - which is broken, as is 1.2.1. 1.2.0 works, because of the above
commit which, again, I presume un-hides the problem.

Also there are reports of older versions being broken.

> On a quick skim of the latest code I see one clear bug
> (path[strlen(path)] will never be '/'!), but that should have caused
> crossmnt to never get enforced at all rather than to override anything.
> 
> Could you retest the latest from the upstream git, with this patch
> applied, and see if the problem is still present?

I've tested right now with
git://git.linux-nfs.org/projects/steved/nfs-utils.git (I hope this is the right
upstream repo) plus the patch - still happens, no change.

One thing that is interesting is that only the first mount gets the
wrong options, if the client unmounts and remounts (at this point the
exports are in the kernel's cache) the options are right. As long as the
kernel cache is populated, the options are right, if one clears the
cache, the first mount will be wrong. Maybe this helps, I find it an
interesting behaviour.

> Then if it is I'll try to go reproduce it myself....

For reference, here is the exports file I use for tests:
/srv/nfs       client(sec=sys,ro,sync,fsid=0,subtree_check,crossmnt)
/srv/nfs/homes client(sec=sys,rw,sync,subtree_check)

And here is the fstab entry on the client:
server:/srv/nfs/homes /home     nfs     noauto,hard,bg,sec=sys,proto=tcp 0 0

The rest of the tools on server and client are Debian's nfs-utils 1.2.1
(they have just a few, very trivial and unrelated, patches). The problem
manifests (in my case) with both nfsv3/sec=sys and nfsv4/sec=krb*.

Thanks!
iustin

Re: Problems with crossmnt since 1.2.1

[J. Bruce Fields]

On Mon, Mar 01, 2010 at 10:13:06PM +0100, Iustin Pop wrote:
> On Mon, Mar 01, 2010 at 03:40:10PM -0500, J. Bruce Fields wrote:
> > On Sun, Feb 28, 2010 at 11:06:43AM +0100, Iustin Pop wrote:
> > > Since nfs-utils 1.2.1, there are some problems with crossmnt usage. See
> > > Debian bug http://bugs.debian.org/567546, but in short the problem seems
> > > to be that sub-mounts (/a/b) take the top-level mount (/a) options
> > > instead of their own, due to a bug in how mountd generates the crossmnt
> > > subexports.
> > > 
> > > I checked that reverting the write_secinfo changes in commit
> > > bc0a6ab03089fc1ea4fea26ed9635c2cc918b01b fix the issue, but that might
> > > only be a side effect, not the actual cause.
> > > 
> > > A short test:
> > > - have /a and /a/b exported, with different flags (e.g. ro on /a, rw on
> > >   /a/b)
> > > - restart the mountd, clear exports, etc.
> > > - try a mount on the client of /a/b, it gets readonly
> > > - umount & remount, it's now r/w
> > > - however, clearing the kernel export table (exportfs -f), makes the
> > >   next mount again get read-only 
> > > 
> > > Disabling crossmnt fixes the issue completely, so I would venture to
> > > guess that the subexports creation code has some issue, but I don't know
> > > enough of this to be able to debug it.
> > 
> > Thanks for the report.  What's the latest nfs-utils version you've
> > tested?
> 
> 1.2.2 - which is broken, as is 1.2.1. 1.2.0 works, because of the above
> commit which, again, I presume un-hides the problem.
> 
> Also there are reports of older versions being broken.
> 
> > On a quick skim of the latest code I see one clear bug
> > (path[strlen(path)] will never be '/'!), but that should have caused
> > crossmnt to never get enforced at all rather than to override anything.
> > 
> > Could you retest the latest from the upstream git, with this patch
> > applied, and see if the problem is still present?
> 
> I've tested right now with
> git://git.linux-nfs.org/projects/steved/nfs-utils.git (I hope this is the right
> upstream repo) plus the patch - still happens, no change.
> 
> One thing that is interesting is that only the first mount gets the
> wrong options, if the client unmounts and remounts (at this point the
> exports are in the kernel's cache) the options are right. As long as the
> kernel cache is populated, the options are right, if one clears the
> cache, the first mount will be wrong. Maybe this helps, I find it an
> interesting behaviour.
> 
> > Then if it is I'll try to go reproduce it myself....
> 
> For reference, here is the exports file I use for tests:
> /srv/nfs       client(sec=sys,ro,sync,fsid=0,subtree_check,crossmnt)
> /srv/nfs/homes client(sec=sys,rw,sync,subtree_check)

Is /srv/nfs/homes a mountpoint, or are both of these on the same
filesystem?

--b.

> 
> And here is the fstab entry on the client:
> server:/srv/nfs/homes /home     nfs     noauto,hard,bg,sec=sys,proto=tcp 0 0
> 
> The rest of the tools on server and client are Debian's nfs-utils 1.2.1
> (they have just a few, very trivial and unrelated, patches). The problem
> manifests (in my case) with both nfsv3/sec=sys and nfsv4/sec=krb*.
> 
> Thanks!
> iustin

Re: Problems with crossmnt since 1.2.1

[Iustin Pop]

On Mon, Mar 01, 2010 at 04:39:52PM -0500, J. Bruce Fields wrote:
> On Mon, Mar 01, 2010 at 10:13:06PM +0100, Iustin Pop wrote:
> > On Mon, Mar 01, 2010 at 03:40:10PM -0500, J. Bruce Fields wrote:
> > > On Sun, Feb 28, 2010 at 11:06:43AM +0100, Iustin Pop wrote:
> > > > Since nfs-utils 1.2.1, there are some problems with crossmnt usage. See
> > > > Debian bug http://bugs.debian.org/567546, but in short the problem seems
> > > > to be that sub-mounts (/a/b) take the top-level mount (/a) options
> > > > instead of their own, due to a bug in how mountd generates the crossmnt
> > > > subexports.
> > > > 
> > > > I checked that reverting the write_secinfo changes in commit
> > > > bc0a6ab03089fc1ea4fea26ed9635c2cc918b01b fix the issue, but that might
> > > > only be a side effect, not the actual cause.
> > > > 
> > > > A short test:
> > > > - have /a and /a/b exported, with different flags (e.g. ro on /a, rw on
> > > >   /a/b)
> > > > - restart the mountd, clear exports, etc.
> > > > - try a mount on the client of /a/b, it gets readonly
> > > > - umount & remount, it's now r/w
> > > > - however, clearing the kernel export table (exportfs -f), makes the
> > > >   next mount again get read-only 
> > > > 
> > > > Disabling crossmnt fixes the issue completely, so I would venture to
> > > > guess that the subexports creation code has some issue, but I don't know
> > > > enough of this to be able to debug it.
> > > 
> > > Thanks for the report.  What's the latest nfs-utils version you've
> > > tested?
> > 
> > 1.2.2 - which is broken, as is 1.2.1. 1.2.0 works, because of the above
> > commit which, again, I presume un-hides the problem.
> > 
> > Also there are reports of older versions being broken.
> > 
> > > On a quick skim of the latest code I see one clear bug
> > > (path[strlen(path)] will never be '/'!), but that should have caused
> > > crossmnt to never get enforced at all rather than to override anything.
> > > 
> > > Could you retest the latest from the upstream git, with this patch
> > > applied, and see if the problem is still present?
> > 
> > I've tested right now with
> > git://git.linux-nfs.org/projects/steved/nfs-utils.git (I hope this is the right
> > upstream repo) plus the patch - still happens, no change.
> > 
> > One thing that is interesting is that only the first mount gets the
> > wrong options, if the client unmounts and remounts (at this point the
> > exports are in the kernel's cache) the options are right. As long as the
> > kernel cache is populated, the options are right, if one clears the
> > cache, the first mount will be wrong. Maybe this helps, I find it an
> > interesting behaviour.
> > 
> > > Then if it is I'll try to go reproduce it myself....
> > 
> > For reference, here is the exports file I use for tests:
> > /srv/nfs       client(sec=sys,ro,sync,fsid=0,subtree_check,crossmnt)
> > /srv/nfs/homes client(sec=sys,rw,sync,subtree_check)
> 
> Is /srv/nfs/homes a mountpoint, or are both of these on the same
> filesystem?

It is a mountpoint, in all my tests. I'll try tomorrow to run a test with them
being on the same filesystem.

iustin

Re: Problems with crossmnt since 1.2.1

[J. Bruce Fields]

On Mon, Mar 01, 2010 at 10:41:16PM +0100, Iustin Pop wrote:
> On Mon, Mar 01, 2010 at 04:39:52PM -0500, J. Bruce Fields wrote:
> > On Mon, Mar 01, 2010 at 10:13:06PM +0100, Iustin Pop wrote:
> > > For reference, here is the exports file I use for tests:
> > > /srv/nfs       client(sec=sys,ro,sync,fsid=0,subtree_check,crossmnt)
> > > /srv/nfs/homes client(sec=sys,rw,sync,subtree_check)
> > 
> > Is /srv/nfs/homes a mountpoint, or are both of these on the same
> > filesystem?
> 
> It is a mountpoint, in all my tests. I'll try tomorrow to run a test with them
> being on the same filesystem.

No, no point, the no-mountpoint case is the case we already *know*
doesn't work.

So, OK, I'll try this here.

--b.

Re: Problems with crossmnt since 1.2.1

[J. Bruce Fields]

On Mon, Mar 01, 2010 at 10:13:06PM +0100, Iustin Pop wrote:
> On Mon, Mar 01, 2010 at 03:40:10PM -0500, J. Bruce Fields wrote:
> > On Sun, Feb 28, 2010 at 11:06:43AM +0100, Iustin Pop wrote:
> > > Since nfs-utils 1.2.1, there are some problems with crossmnt usage. See
> > > Debian bug http://bugs.debian.org/567546, but in short the problem seems
> > > to be that sub-mounts (/a/b) take the top-level mount (/a) options
> > > instead of their own, due to a bug in how mountd generates the crossmnt
> > > subexports.
> > > 
> > > I checked that reverting the write_secinfo changes in commit
> > > bc0a6ab03089fc1ea4fea26ed9635c2cc918b01b fix the issue, but that might
> > > only be a side effect, not the actual cause.
> > > 
> > > A short test:
> > > - have /a and /a/b exported, with different flags (e.g. ro on /a, rw on
> > >   /a/b)
> > > - restart the mountd, clear exports, etc.
> > > - try a mount on the client of /a/b, it gets readonly
> > > - umount & remount, it's now r/w
> > > - however, clearing the kernel export table (exportfs -f), makes the
> > >   next mount again get read-only 
> > > 
> > > Disabling crossmnt fixes the issue completely, so I would venture to
> > > guess that the subexports creation code has some issue, but I don't know
> > > enough of this to be able to debug it.
> > 
> > Thanks for the report.  What's the latest nfs-utils version you've
> > tested?
> 
> 1.2.2 - which is broken, as is 1.2.1. 1.2.0 works, because of the above
> commit which, again, I presume un-hides the problem.
> 
> Also there are reports of older versions being broken.
> 
> > On a quick skim of the latest code I see one clear bug
> > (path[strlen(path)] will never be '/'!), but that should have caused
> > crossmnt to never get enforced at all rather than to override anything.
> > 
> > Could you retest the latest from the upstream git, with this patch
> > applied, and see if the problem is still present?
> 
> I've tested right now with
> git://git.linux-nfs.org/projects/steved/nfs-utils.git (I hope this is the right
> upstream repo) plus the patch - still happens, no change.
> 
> One thing that is interesting is that only the first mount gets the
> wrong options, if the client unmounts and remounts (at this point the
> exports are in the kernel's cache) the options are right. As long as the
> kernel cache is populated, the options are right, if one clears the
> cache, the first mount will be wrong. Maybe this helps, I find it an
> interesting behaviour.
> 
> > Then if it is I'll try to go reproduce it myself....
> 
> For reference, here is the exports file I use for tests:
> /srv/nfs       client(sec=sys,ro,sync,fsid=0,subtree_check,crossmnt)
> /srv/nfs/homes client(sec=sys,rw,sync,subtree_check)
> 
> And here is the fstab entry on the client:
> server:/srv/nfs/homes /home     nfs     noauto,hard,bg,sec=sys,proto=tcp 0 0
> 
> The rest of the tools on server and client are Debian's nfs-utils 1.2.1
> (they have just a few, very trivial and unrelated, patches). The problem
> manifests (in my case) with both nfsv3/sec=sys and nfsv4/sec=krb*.

The loop in utils/mountd/cache.c:cache_export_ent() looks extremely
suspicious: it seems to be populating the kernel's export cache with
parameters taken from the parent export.  I can't see why that's
necessary at all--those exports can be looked up on demand later when
they're needed.  Something as simple as the following might help?
(Totally untested!)

--b.

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index d63e10a..7d87812 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -619,7 +619,7 @@ static int is_subdirectory(char *subpath, char *path)
 	int l = strlen(path);
 
 	return strcmp(subpath, path) == 0
-		|| (strncmp(subpath, path, l) == 0 && path[l] == '/');
+		|| (strncmp(subpath, path, l) == 0 && subpath[l] == '/');
 }
 
 static int path_matches(nfs_export *exp, char *path)
@@ -817,46 +817,6 @@ int cache_export_ent(char *domain, struct exportent *exp, char *path)
 		     " fsid= required", exp->e_path);
 	}
 
-	while (err == 0 && (exp->e_flags & NFSEXP_CROSSMOUNT) && path) {
-		/* really an 'if', but we can break out of
-		 * a 'while' more easily */
-		/* Look along 'path' for other filesystems
-		 * and export them with the same options
-		 */
-		struct stat stb;
-		int l = strlen(exp->e_path);
-		int dev;
-
-		if (strlen(path) <= l || path[l] != '/' ||
-		    strncmp(exp->e_path, path, l) != 0)
-			break;
-		if (stat(exp->e_path, &stb) != 0)
-			break;
-		dev = stb.st_dev;
-		while(path[l] == '/') {
-			char c;
-			/* errors for submount should fail whole filesystem */
-			int err2;
-
-			l++;
-			while (path[l] != '/' && path[l])
-				l++;
-			c = path[l];
-			path[l] = 0;
-			err2 = lstat(path, &stb);
-			path[l] = c;
-			if (err2 < 0)
-				break;
-			if (stb.st_dev == dev)
-				continue;
-			dev = stb.st_dev;
-			path[l] = 0;
-			dump_to_cache(f, domain, path, exp);
-			path[l] = c;
-		}
-		break;
-	}
-
 	fclose(f);
 	return err;
 }

Re: Problems with crossmnt since 1.2.1

[Iustin Pop]

On Mon, Mar 01, 2010 at 10:20:02PM -0500, J. Bruce Fields wrote:
> On Mon, Mar 01, 2010 at 10:13:06PM +0100, Iustin Pop wrote:
> > On Mon, Mar 01, 2010 at 03:40:10PM -0500, J. Bruce Fields wrote:
> > > On Sun, Feb 28, 2010 at 11:06:43AM +0100, Iustin Pop wrote:
> > > > Since nfs-utils 1.2.1, there are some problems with crossmnt usage. See
> > > > Debian bug http://bugs.debian.org/567546, but in short the problem seems
> > > > to be that sub-mounts (/a/b) take the top-level mount (/a) options
> > > > instead of their own, due to a bug in how mountd generates the crossmnt
> > > > subexports.
> > > > 
> > > > I checked that reverting the write_secinfo changes in commit
> > > > bc0a6ab03089fc1ea4fea26ed9635c2cc918b01b fix the issue, but that might
> > > > only be a side effect, not the actual cause.
> > > > 
> > > > A short test:
> > > > - have /a and /a/b exported, with different flags (e.g. ro on /a, rw on
> > > >   /a/b)
> > > > - restart the mountd, clear exports, etc.
> > > > - try a mount on the client of /a/b, it gets readonly
> > > > - umount & remount, it's now r/w
> > > > - however, clearing the kernel export table (exportfs -f), makes the
> > > >   next mount again get read-only 
> > > > 
> > > > Disabling crossmnt fixes the issue completely, so I would venture to
> > > > guess that the subexports creation code has some issue, but I don't know
> > > > enough of this to be able to debug it.
> > > 
> > > Thanks for the report.  What's the latest nfs-utils version you've
> > > tested?
> > 
> > 1.2.2 - which is broken, as is 1.2.1. 1.2.0 works, because of the above
> > commit which, again, I presume un-hides the problem.
> > 
> > Also there are reports of older versions being broken.
> > 
> > > On a quick skim of the latest code I see one clear bug
> > > (path[strlen(path)] will never be '/'!), but that should have caused
> > > crossmnt to never get enforced at all rather than to override anything.
> > > 
> > > Could you retest the latest from the upstream git, with this patch
> > > applied, and see if the problem is still present?
> > 
> > I've tested right now with
> > git://git.linux-nfs.org/projects/steved/nfs-utils.git (I hope this is the right
> > upstream repo) plus the patch - still happens, no change.
> > 
> > One thing that is interesting is that only the first mount gets the
> > wrong options, if the client unmounts and remounts (at this point the
> > exports are in the kernel's cache) the options are right. As long as the
> > kernel cache is populated, the options are right, if one clears the
> > cache, the first mount will be wrong. Maybe this helps, I find it an
> > interesting behaviour.
> > 
> > > Then if it is I'll try to go reproduce it myself....
> > 
> > For reference, here is the exports file I use for tests:
> > /srv/nfs       client(sec=sys,ro,sync,fsid=0,subtree_check,crossmnt)
> > /srv/nfs/homes client(sec=sys,rw,sync,subtree_check)
> > 
> > And here is the fstab entry on the client:
> > server:/srv/nfs/homes /home     nfs     noauto,hard,bg,sec=sys,proto=tcp 0 0
> > 
> > The rest of the tools on server and client are Debian's nfs-utils 1.2.1
> > (they have just a few, very trivial and unrelated, patches). The problem
> > manifests (in my case) with both nfsv3/sec=sys and nfsv4/sec=krb*.
> 
> The loop in utils/mountd/cache.c:cache_export_ent() looks extremely
> suspicious: it seems to be populating the kernel's export cache with
> parameters taken from the parent export.  I can't see why that's
> necessary at all--those exports can be looked up on demand later when
> they're needed.  Something as simple as the following might help?
> (Totally untested!)

Sorry for the delayed response. I can confirm this solves my test case. I get
the right mount options in both the "mount directly the sub-directory" case,
and in the "mount parent and access the sub-directory via the crossmnt feature"
case.

thanks!
iustin

Re: Problems with crossmnt since 1.2.1

[J. Bruce Fields]

On Sun, Mar 07, 2010 at 08:54:49PM +0100, Iustin Pop wrote:
> Sorry for the delayed response. I can confirm this solves my test case. I get
> the right mount options in both the "mount directly the sub-directory" case,
> and in the "mount parent and access the sub-directory via the crossmnt feature"

Excellent, thanks.  Steve, could you apply the following patches?

Also available from

	git pull git://linux-nfs.org/~bfields/nfs-utils.git crossmnt-fixes

--b.

[PATCH 1/3] mountd: fix path comparison for v4 crossmnt

[J. Bruce Fields]

This was obviously wrong, since path[strlen(path)] == '\0' should always
be true.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 utils/mountd/cache.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index d63e10a..ff27bbf 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -619,7 +619,7 @@ static int is_subdirectory(char *subpath, char *path)
 	int l = strlen(path);
 
 	return strcmp(subpath, path) == 0
-		|| (strncmp(subpath, path, l) == 0 && path[l] == '/');
+		|| (strncmp(subpath, path, l) == 0 && subpath[l] == '/');
 }
 
 static int path_matches(nfs_export *exp, char *path)
-- 
1.6.3.3

[PATCH 2/3] mountd: trivial: name parameters for clarity

[J. Bruce Fields]

Part of the reason for the previous bug was confusion between "subpath"
and "path"; which is the shorter path, and which the longer?

"child" and "parent" seem less ambiguous.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 utils/mountd/cache.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index ff27bbf..7dec468 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -614,12 +614,12 @@ static int dump_to_cache(FILE *f, char *domain, char *path, struct exportent *ex
 	return qword_eol(f);
 }
 
-static int is_subdirectory(char *subpath, char *path)
+static int is_subdirectory(char *child, char *parent)
 {
-	int l = strlen(path);
+	int l = strlen(parent);
 
-	return strcmp(subpath, path) == 0
-		|| (strncmp(subpath, path, l) == 0 && subpath[l] == '/');
+	return strcmp(child, parent) == 0
+		|| (strncmp(child, parent, l) == 0 && child[l] == '/');
 }
 
 static int path_matches(nfs_export *exp, char *path)
-- 
1.6.3.3

[PATCH 3/3] mountd: fix crossmnt options in v2/v3 case

[J. Bruce Fields]

The extra cache entries added here all get the options of the parent
export.  This is incorrect, since once of the children may be explicitly
exported with different options, and the parent crossmnt shouldn't
override the explicit child export.

What's more, this is unnecessary, since in the newcache case we'll
request these on demand when we need them.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 utils/mountd/cache.c |   40 ----------------------------------------
 1 files changed, 0 insertions(+), 40 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 7dec468..200e179 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -817,46 +817,6 @@ int cache_export_ent(char *domain, struct exportent *exp, char *path)
 		     " fsid= required", exp->e_path);
 	}
 
-	while (err == 0 && (exp->e_flags & NFSEXP_CROSSMOUNT) && path) {
-		/* really an 'if', but we can break out of
-		 * a 'while' more easily */
-		/* Look along 'path' for other filesystems
-		 * and export them with the same options
-		 */
-		struct stat stb;
-		int l = strlen(exp->e_path);
-		int dev;
-
-		if (strlen(path) <= l || path[l] != '/' ||
-		    strncmp(exp->e_path, path, l) != 0)
-			break;
-		if (stat(exp->e_path, &stb) != 0)
-			break;
-		dev = stb.st_dev;
-		while(path[l] == '/') {
-			char c;
-			/* errors for submount should fail whole filesystem */
-			int err2;
-
-			l++;
-			while (path[l] != '/' && path[l])
-				l++;
-			c = path[l];
-			path[l] = 0;
-			err2 = lstat(path, &stb);
-			path[l] = c;
-			if (err2 < 0)
-				break;
-			if (stb.st_dev == dev)
-				continue;
-			dev = stb.st_dev;
-			path[l] = 0;
-			dump_to_cache(f, domain, path, exp);
-			path[l] = c;
-		}
-		break;
-	}
-
 	fclose(f);
 	return err;
 }
-- 
1.6.3.3

Re: [PATCH 3/3] mountd: fix crossmnt options in v2/v3 case

[Neil Brown]

On Sun,  7 Mar 2010 15:08:01 -0500
"J. Bruce Fields" <bfields@citi.umich.edu> wrote:

> The extra cache entries added here all get the options of the parent
> export.  This is incorrect, since once of the children may be explicitly
> exported with different options, and the parent crossmnt shouldn't
> override the explicit child export.
> 
> What's more, this is unnecessary, since in the newcache case we'll
> request these on demand when we need them.

Are you sure that removing this doesn't break something else?
It was explicitly added in commit 323d1c4d69b65ab36d, apparently for a good
reason.

Also, it shouldn't be causing the problem described as cache_export_ent
should only be called where 'exp' is the lowest (furthest from the root)
export for any directory in 'path'.  So any child mounts that are found
should not be explicitly exported.


This code is needed to handle a case where you have
/a /a/b /a/b/c all mount points, /a is exported with crossmnt,
and a mount request comes in for /a/b/c (or /a/b).
mountd needs to get the filehandle for /a/b/c, so that filesystem must be
exported to the kernel.  We cannot really on upcalls filling in that
information as doing so would cause mountd to deadlock - it asks the kernel
for a filehandle, the kernel asks it for export information, and mountd is
single-threaded...

Are you sure that removing this actually fixes a problem?

NeilBrown


> 
> Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> ---
>  utils/mountd/cache.c |   40 ----------------------------------------
>  1 files changed, 0 insertions(+), 40 deletions(-)
> 
> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
> index 7dec468..200e179 100644
> --- a/utils/mountd/cache.c
> +++ b/utils/mountd/cache.c
> @@ -817,46 +817,6 @@ int cache_export_ent(char *domain, struct exportent *exp, char *path)
>  		     " fsid= required", exp->e_path);
>  	}
>  
> -	while (err == 0 && (exp->e_flags & NFSEXP_CROSSMOUNT) && path) {
> -		/* really an 'if', but we can break out of
> -		 * a 'while' more easily */
> -		/* Look along 'path' for other filesystems
> -		 * and export them with the same options
> -		 */
> -		struct stat stb;
> -		int l = strlen(exp->e_path);
> -		int dev;
> -
> -		if (strlen(path) <= l || path[l] != '/' ||
> -		    strncmp(exp->e_path, path, l) != 0)
> -			break;
> -		if (stat(exp->e_path, &stb) != 0)
> -			break;
> -		dev = stb.st_dev;
> -		while(path[l] == '/') {
> -			char c;
> -			/* errors for submount should fail whole filesystem */
> -			int err2;
> -
> -			l++;
> -			while (path[l] != '/' && path[l])
> -				l++;
> -			c = path[l];
> -			path[l] = 0;
> -			err2 = lstat(path, &stb);
> -			path[l] = c;
> -			if (err2 < 0)
> -				break;
> -			if (stb.st_dev == dev)
> -				continue;
> -			dev = stb.st_dev;
> -			path[l] = 0;
> -			dump_to_cache(f, domain, path, exp);
> -			path[l] = c;
> -		}
> -		break;
> -	}
> -
>  	fclose(f);
>  	return err;
>  }

Re: [PATCH 3/3] mountd: fix crossmnt options in v2/v3 case

[J. Bruce Fields]

On Mon, Mar 08, 2010 at 08:25:16AM +1100, Neil Brown wrote:
> On Sun,  7 Mar 2010 15:08:01 -0500
> "J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> 
> > The extra cache entries added here all get the options of the parent
> > export.  This is incorrect, since once of the children may be explicitly
> > exported with different options, and the parent crossmnt shouldn't
> > override the explicit child export.
> > 
> > What's more, this is unnecessary, since in the newcache case we'll
> > request these on demand when we need them.
> 
> Are you sure that removing this doesn't break something else?

Maybe so--thanks for looking at this.

> It was explicitly added in commit 323d1c4d69b65ab36d, apparently for a good
> reason.
> 
> Also, it shouldn't be causing the problem described as cache_export_ent
> should only be called where 'exp' is the lowest (furthest from the root)
> export for any directory in 'path'.

Hm.  Is nfsd_fh() following that rule?

> So any child mounts that are found should not be explicitly exported.
>
> This code is needed to handle a case where you have
> /a /a/b /a/b/c all mount points, /a is exported with crossmnt,
> and a mount request comes in for /a/b/c (or /a/b).
> mountd needs to get the filehandle for /a/b/c, so that filesystem must be
> exported to the kernel.

Yes, I didn't look carefully enough.  I think I must have assumed the
first dump_to_cache did that.  But wouldn't it be sufficient to just do:

	dump_to_cache(f, domain, path, e_path);

(as in the below) instead of also running through all the parents?

> We cannot really on upcalls filling in that
> information as doing so would cause mountd to deadlock - it asks the kernel
> for a filehandle, the kernel asks it for export information, and mountd is
> single-threaded...

Don't we have this problem already, then?  The export cache really is
just a cache, and we should be prepared for a given entry to be purged
at any time.

--b.

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 7dec468..f682a9b 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -810,53 +810,13 @@ int cache_export_ent(char *domain, struct exportent *exp, char *path)
 	if (!f)
 		return -1;
 
-	err = dump_to_cache(f, domain, exp->e_path, exp);
+	err = dump_to_cache(f, domain, path, exp);
 	if (err) {
 		xlog(L_WARNING,
 		     "Cannot export %s, possibly unsupported filesystem or"
 		     " fsid= required", exp->e_path);
 	}
 
-	while (err == 0 && (exp->e_flags & NFSEXP_CROSSMOUNT) && path) {
-		/* really an 'if', but we can break out of
-		 * a 'while' more easily */
-		/* Look along 'path' for other filesystems
-		 * and export them with the same options
-		 */
-		struct stat stb;
-		int l = strlen(exp->e_path);
-		int dev;
-
-		if (strlen(path) <= l || path[l] != '/' ||
-		    strncmp(exp->e_path, path, l) != 0)
-			break;
-		if (stat(exp->e_path, &stb) != 0)
-			break;
-		dev = stb.st_dev;
-		while(path[l] == '/') {
-			char c;
-			/* errors for submount should fail whole filesystem */
-			int err2;
-
-			l++;
-			while (path[l] != '/' && path[l])
-				l++;
-			c = path[l];
-			path[l] = 0;
-			err2 = lstat(path, &stb);
-			path[l] = c;
-			if (err2 < 0)
-				break;
-			if (stb.st_dev == dev)
-				continue;
-			dev = stb.st_dev;
-			path[l] = 0;
-			dump_to_cache(f, domain, path, exp);
-			path[l] = c;
-		}
-		break;
-	}
-
 	fclose(f);
 	return err;
 }

Re: [PATCH 3/3] mountd: fix crossmnt options in v2/v3 case

[Neil Brown]

On Sun, 7 Mar 2010 16:58:26 -0500
"J. Bruce Fields" <bfields@citi.umich.edu> wrote:

> On Mon, Mar 08, 2010 at 08:25:16AM +1100, Neil Brown wrote:
> > On Sun,  7 Mar 2010 15:08:01 -0500
> > "J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> > 
> > > The extra cache entries added here all get the options of the parent
> > > export.  This is incorrect, since once of the children may be explicitly
> > > exported with different options, and the parent crossmnt shouldn't
> > > override the explicit child export.
> > > 
> > > What's more, this is unnecessary, since in the newcache case we'll
> > > request these on demand when we need them.
> > 
> > Are you sure that removing this doesn't break something else?
> 
> Maybe so--thanks for looking at this.
> 
> > It was explicitly added in commit 323d1c4d69b65ab36d, apparently for a good
> > reason.
> > 
> > Also, it shouldn't be causing the problem described as cache_export_ent
> > should only be called where 'exp' is the lowest (furthest from the root)
> > export for any directory in 'path'.
> 
> Hm.  Is nfsd_fh() following that rule?

Hmmm... maybe not.  However when it doesn't it should log a warning
"%s and %s have same filehandle for %s, using first"

Maybe we need better choice between options there.  Maybe we should choose
the longest e_path??


> 
> > So any child mounts that are found should not be explicitly exported.
> >
> > This code is needed to handle a case where you have
> > /a /a/b /a/b/c all mount points, /a is exported with crossmnt,
> > and a mount request comes in for /a/b/c (or /a/b).
> > mountd needs to get the filehandle for /a/b/c, so that filesystem must be
> > exported to the kernel.
> 
> Yes, I didn't look carefully enough.  I think I must have assumed the
> first dump_to_cache did that.  But wouldn't it be sufficient to just do:
> 
> 	dump_to_cache(f, domain, path, e_path);
s/e_path/exp/
> 
> (as in the below) instead of also running through all the parents?

That sounds credible.. I wonder why I didn't think of that at the time.

My thoughts stray to lookups of ".." and whether they could use the wrong
export.  I don't think that can in any sane configuration.

> 
> > We cannot really on upcalls filling in that
> > information as doing so would cause mountd to deadlock - it asks the kernel
> > for a filehandle, the kernel asks it for export information, and mountd is
> > single-threaded...
> 
> Don't we have this problem already, then?  The export cache really is
> just a cache, and we should be prepared for a given entry to be purged
> at any time.
> 

Yes, it is a cache, but things don't get pushed out when it is 'full', so you
can expect items to stay out their expiry time.  So if you add something
with  an expiry 30 minutes in the future, you can usually expect it to still
be there in 30 seconds.

If someone ran "exportfs -f" at exactly the wrong time you might be able to
deadlock mountd (I'd have to double-check to be sure), but that is very
different from mountd acting in a way which leads directly to a deadlock.

NeilBrown


> --b.
> 
> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
> index 7dec468..f682a9b 100644
> --- a/utils/mountd/cache.c
> +++ b/utils/mountd/cache.c
> @@ -810,53 +810,13 @@ int cache_export_ent(char *domain, struct exportent *exp, char *path)
>  	if (!f)
>  		return -1;
>  
> -	err = dump_to_cache(f, domain, exp->e_path, exp);
> +	err = dump_to_cache(f, domain, path, exp);
>  	if (err) {
>  		xlog(L_WARNING,
>  		     "Cannot export %s, possibly unsupported filesystem or"
>  		     " fsid= required", exp->e_path);
>  	}

Re: [PATCH 3/3] mountd: fix crossmnt options in v2/v3 case

[J. Bruce Fields]

On Mon, Mar 08, 2010 at 10:10:14AM +1100, Neil Brown wrote:
> On Sun, 7 Mar 2010 16:58:26 -0500
> "J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> > Don't we have this problem already, then?  The export cache really is
> > just a cache, and we should be prepared for a given entry to be purged
> > at any time.
> > 
> 
> Yes, it is a cache, but things don't get pushed out when it is 'full', so you
> can expect items to stay out their expiry time.

At least for some caches we may want some sort of pruning when they get
to full.

If we do that, then we may want to vary the behavior from cache to
cache, hence keeping the export cache protected from this deadlock.
Though I'd rather avoid that kind of special case if we could.

> So if you add something
> with  an expiry 30 minutes in the future, you can usually expect it to still
> be there in 30 seconds.
> 
> If someone ran "exportfs -f"

or "exportfs -r"?

> at exactly the wrong time you might be able to deadlock mountd (I'd
> have to double-check to be sure), but that is very different from
> mountd acting in a way which leads directly to a deadlock.

Yeah, agreed.  It's still a bug--people are supposed to be able to do
that without deadlock--but, sure, it may be unbelievably hard to hit.

So, I need to look harder at this, thanks for the review.  (May take a
few days, though, so if someone else wants to volunteer, feel free.)

I wonder how best to eliminate the deadlock:

	- Allow fh downcall to return -EAGAIN, let mountd change that to
	  a JUKEBOX or dropped rpc?
	- Require mountd run two processes, with one dedicated to
	  upcalls?
	- Separate mountd entirely into an upcall-handling daemon and an
	  rpc daemon?

Occasionally we seem to hear from a security-conscious administrator
who's running v4-only and is irritated that they have to firewall off
mountd instead of just being able to kill it entirely.  The latter might
reassure them, I suppose.

--b.

Re: [PATCH 3/3] mountd: fix crossmnt options in v2/v3 case

[J. Bruce Fields]

On Mon, Mar 08, 2010 at 01:21:48PM -0500, J. Bruce Fields wrote:
> So, I need to look harder at this, thanks for the review.

By the way, Steve, the first two patches should still be fine, if you
want to apply those.

--b.

Re: [PATCH 3/3] mountd: fix crossmnt options in v2/v3 case

[Chuck Lever]

On 03/08/2010 01:21 PM, J. Bruce Fields wrote:
> Occasionally we seem to hear from a security-conscious administrator
> who's running v4-only and is irritated that they have to firewall off
> mountd instead of just being able to kill it entirely.  The latter might
> reassure them, I suppose.

I think Jeff had the idea of having mountd simply not set up its RPC 
listeners in that case.  That looks easy to do.

-- 
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 3/3] mountd: fix crossmnt options in v2/v3 case

[J. Bruce Fields]

On Mon, Mar 08, 2010 at 01:30:23PM -0500, Chuck Lever wrote:
> On 03/08/2010 01:21 PM, J. Bruce Fields wrote:
>> Occasionally we seem to hear from a security-conscious administrator
>> who's running v4-only and is irritated that they have to firewall off
>> mountd instead of just being able to kill it entirely.  The latter might
>> reassure them, I suppose.
>
> I think Jeff had the idea of having mountd simply not set up its RPC  
> listeners in that case.  That looks easy to do.

Sure, makes sense.

But we might decide we want separate processes for servicing MOUNT
requests and export upcalls anyway.

In which case, call one rpc.mountd, the other nfsd-cache-helper, don't
bother running "rpc.mountd" in the v4-only case, and, yay, we never have
to answer the "why do I still have to run rpc.mountd?" question again.

I dunno.

--b.

Re: [PATCH 3/3] mountd: fix crossmnt options in v2/v3 case

[Chuck Lever]

On 03/08/2010 01:41 PM, J. Bruce Fields wrote:
> On Mon, Mar 08, 2010 at 01:30:23PM -0500, Chuck Lever wrote:
>> On 03/08/2010 01:21 PM, J. Bruce Fields wrote:
>>> Occasionally we seem to hear from a security-conscious administrator
>>> who's running v4-only and is irritated that they have to firewall off
>>> mountd instead of just being able to kill it entirely.  The latter might
>>> reassure them, I suppose.
>>
>> I think Jeff had the idea of having mountd simply not set up its RPC
>> listeners in that case.  That looks easy to do.
>
> Sure, makes sense.
>
> But we might decide we want separate processes for servicing MOUNT
> requests and export upcalls anyway.
>
> In which case, call one rpc.mountd, the other nfsd-cache-helper, don't
> bother running "rpc.mountd" in the v4-only case, and, yay, we never have
> to answer the "why do I still have to run rpc.mountd?" question again.

Heh.

Separating these facilities would make mountd's RPC service 
implementation more standard.  Right now, it has to select() over the 
RPC listeners _and_ the cache upcall fds.  That means it has it's own 
"svc_run()" function, and thus doesn't use the RPC library's.

But it might get complicated if we want to retain functionality in 
mountd for kernels that don't have the new export cache...

-- 
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 3/3] mountd: fix crossmnt options in v2/v3 case

[Steve Dickson]

On 03/08/2010 01:41 PM, J. Bruce Fields wrote:
> On Mon, Mar 08, 2010 at 01:30:23PM -0500, Chuck Lever wrote:
>> On 03/08/2010 01:21 PM, J. Bruce Fields wrote:
>>> Occasionally we seem to hear from a security-conscious administrator
>>> who's running v4-only and is irritated that they have to firewall off
>>> mountd instead of just being able to kill it entirely.  The latter might
>>> reassure them, I suppose.
>>
>> I think Jeff had the idea of having mountd simply not set up its RPC  
>> listeners in that case.  That looks easy to do.
> 
> Sure, makes sense.
> 
> But we might decide we want separate processes for servicing MOUNT
> requests and export upcalls anyway.
Yes... separating the upcalls from the network call would make things
much more similar... IMHO...

> 
> In which case, call one rpc.mountd, the other nfsd-cache-helper, don't
> bother running "rpc.mountd" in the v4-only case, and, yay, we never have
> to answer the "why do I still have to run rpc.mountd?" question again.
Who says mountd has to be a longed lived daemon 100% of the time... 
It could used a start point for both the nfsv4listner process and
the RPC listener (i.e. mountd itself). Then mountd could realize its
a nfsv4-only environment and simply die (once the nfsv4lister is started).

Just a thought...

steved.

Re: [PATCH 2/3] mountd: trivial: name parameters for clarity

[Steve Dickson]

On 03/07/2010 03:08 PM, J. Bruce Fields wrote:
> Part of the reason for the previous bug was confusion between "subpath"
> and "path"; which is the shorter path, and which the longer?
> 
> "child" and "parent" seem less ambiguous.
> 
> Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> ---
>  utils/mountd/cache.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
> index ff27bbf..7dec468 100644
> --- a/utils/mountd/cache.c
> +++ b/utils/mountd/cache.c
> @@ -614,12 +614,12 @@ static int dump_to_cache(FILE *f, char *domain, char *path, struct exportent *ex
>  	return qword_eol(f);
>  }
>  
> -static int is_subdirectory(char *subpath, char *path)
> +static int is_subdirectory(char *child, char *parent)
>  {
> -	int l = strlen(path);
> +	int l = strlen(parent);
>  
> -	return strcmp(subpath, path) == 0
> -		|| (strncmp(subpath, path, l) == 0 && subpath[l] == '/');
> +	return strcmp(child, parent) == 0
> +		|| (strncmp(child, parent, l) == 0 && child[l] == '/');
>  }
>  
>  static int path_matches(nfs_export *exp, char *path)
Committed...

steved

Re: [PATCH 1/3] mountd: fix path comparison for v4 crossmnt

[Steve Dickson]

On 03/07/2010 03:07 PM, J. Bruce Fields wrote:
> This was obviously wrong, since path[strlen(path)] == '\0' should always
> be true.
> 
> Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> ---
>  utils/mountd/cache.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
> index d63e10a..ff27bbf 100644
> --- a/utils/mountd/cache.c
> +++ b/utils/mountd/cache.c
> @@ -619,7 +619,7 @@ static int is_subdirectory(char *subpath, char *path)
>  	int l = strlen(path);
>  
>  	return strcmp(subpath, path) == 0
> -		|| (strncmp(subpath, path, l) == 0 && path[l] == '/');
> +		|| (strncmp(subpath, path, l) == 0 && subpath[l] == '/');
>  }
>  
>  static int path_matches(nfs_export *exp, char *path)
Committed...

steved.

Re: Problems with crossmnt since 1.2.1

[Steve Dickson]

On 03/07/2010 03:06 PM, J. Bruce Fields wrote:
> On Sun, Mar 07, 2010 at 08:54:49PM +0100, Iustin Pop wrote:
>> Sorry for the delayed response. I can confirm this solves my test case. I get
>> the right mount options in both the "mount directly the sub-directory" case,
>> and in the "mount parent and access the sub-directory via the crossmnt feature"
> 
> Excellent, thanks.  Steve, could you apply the following patches?
> 
> Also available from
> 
> 	git pull git://linux-nfs.org/~bfields/nfs-utils.git crossmnt-fixes
I think I will wait on commit 72f6d0 until it hashed out... 

steved.