[PATCH 0/6] fix some problems with v4root handling in mountd

[PATCH 0/6] fix some problems with v4root handling in mountd

[Neil Brown]

First I'd like to say that I love seeing comments that explain the
code..  What way when the code does something different to the
comment, I can be sure I'm on to something :-)

If I have exports:

 /  *(rw,no_root_squash)
 /home 192.168.1.2(rw,no_root_squash)

Then I want to export 'root' to everything (on my side of a firewall)
but '/home' only to one machine.
However this doesn't work for host 192.168.1.2.
If I try to mount '/' with NFSv3, it fails.
If I try to mount '/' with NFSv4, then it works but we only see
"home" in the mounted "/".

The problem is that the export of "/home" causes a V4ROOT export
of "/" to be created for 192.168.1.2 and as MCL_SUBNETWORK comes
before MCL_WILDCARD, the V4ROOT export wins and the other wildcard
export isn't even tried.

So the following patches:
 1/ fix the NFSv4 mount case so that we really see '/' and not a pseudo
    '/'.  This is the case where the comment says the right thing
    but the code doesn't.

 2/ fix 'mountd' lookup for NFSv2/v3 so they successfully mount
    the real export, and don't fail because only the V4ROOT export
    was found,

 3/ Fix a similar ordering problem in nfsd_fh.  Without this an
    NFSv4 client might see the V4ROOT export for '/' rather than
    the explicitly exported export.

 4/ Set a useful ttl on V4ROOT exports so they don't need to
    constantly be refreshed, an
  
 5/ fix a buglet which didn't cause any of these problems but
    certainly looks wrong.

Enjoy :-)

NeilBrown

---

Neil Brown (6):
      v4_root_add_parents: remove a possible buffer overflow.
      v4root: set the time-to-live for V4ROOT exports to the DEFAULT.
      v4set_root: force "fsid=0" for all exports of '/'
      nfsd_fh: if two exports are possible, choose the one without V4ROOT
      auth_authenticate_newcache: prefer non-V4ROOT export over V4ROOT exports.
      lookup_export: really prefer nonV4ROOT exports.


 utils/mountd/auth.c   |    7 +++----
 utils/mountd/cache.c  |   21 +++++++++++++++++----
 utils/mountd/v4root.c |   12 ++++++++++--
 3 files changed, 30 insertions(+), 10 deletions(-)

-- 
Signature

[PATCH 6/6] v4_root_add_parents: remove a possible buffer overflow.

[Neil Brown]

The loop in v4root_add_parents() is a little odd.
The first time through, 'ptr' points immediately "beyond"
a '/' character (the first).  For every other iterration it points
directly "at" a '/' character.
Such inconsistency is error prone and infact there is an error.
If "path" is precisely "/", then the first call to
   ptr = strchr(ptr, '/')

will be given a 'ptr' which is beyond the '\0' at the end of
"path".  This could potentially contain anything and the strchr()
could search well beyond a buffer (though this depends on exactly how
the string is set up which depends on separate code).

So change the loop to have 'ptr' always point at a '/', and
handle the special case of "/" explicitly.

Signed-off-by: NeilBrown <neilb@suse.de>
---

 utils/mountd/v4root.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/utils/mountd/v4root.c b/utils/mountd/v4root.c
index 57ee0b2..708eb61 100644
--- a/utils/mountd/v4root.c
+++ b/utils/mountd/v4root.c
@@ -150,13 +150,13 @@ static int v4root_add_parents(nfs_export *exp)
 				"pseudo export for '%s'", exp->m_export.e_path);
 		return -ENOMEM;
 	}
-	for (ptr = path + 1; ptr; ptr = strchr(ptr, '/')) {
+	for (ptr = path; ptr; ptr = strchr(ptr, '/')) {
 		int ret;
 		char saved;
 
 		saved = *ptr;
 		*ptr = '\0';
-		ret = pseudofs_update(hostname, path, exp);
+		ret = pseudofs_update(hostname, *path ? path : "/", exp);
 		if (ret)
 			return ret;
 		*ptr = saved;

[PATCH 1/6] lookup_export: really prefer nonV4ROOT exports.

[Neil Brown]

lookup_export() claims to "Always prefer non-V4ROOT mounts" (meaning
"exports") but actually prefers V4ROOT exports - once it has 'found'
one it will never replace it.

So fix that inversion, and add code so that it proactively prefers a
non-V4ROOT whether it is found before or after a V4ROOT.

Signed-off-by: NeilBrown <neilb@suse.de>
---

 utils/mountd/cache.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index ac9cdbd..b01c0bd 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -772,10 +772,14 @@ lookup_export(char *dom, char *path, struct addrinfo *ai)
 				found_type = i;
 				continue;
 			}
-
-			/* Always prefer non-V4ROOT mounts */
-			if (found->m_export.e_flags & NFSEXP_V4ROOT)
+			/* Always prefer non-V4ROOT exports */
+			if (exp->m_export.e_flags & NFSEXP_V4ROOT)
 				continue;
+			if (found->m_export.e_flags & NFSEXP_V4ROOT) {
+				found = exp;
+				found_type = i;
+				continue;
+			}
 
 			/* If one is a CROSSMOUNT, then prefer the longest path */
 			if (((found->m_export.e_flags & NFSEXP_CROSSMOUNT) ||

[PATCH 3/6] nfsd_fh: if two exports are possible, choose the one without V4ROOT

[Neil Brown]

When nfsd_fh it looking for an export for a particular
client and file-handle, it might find two exports for the same path:
one with NFSEXP_V4ROOT, one with out.

As nfsd_fh calls cache_export_ent to give the export information to
the kernel it much choose the same export that auth_authenticate
chooses for get_rootfh which it also passes cache_export_ent (via
cache_export).
i.e. it must choose the non-V4ROOT on where possible.

Also change
   strcmp(foo, bar)
to
   strcmp(foo, bar) == 0
because I have a pathological fear of the former.

Signed-off-by: NeilBrown <neilb@suse.de>
---

 utils/mountd/cache.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index b01c0bd..0af6404 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -596,11 +596,20 @@ static void nfsd_fh(FILE *f)
 				found_path = strdup(path);
 				if (found_path == NULL)
 					goto out;
-			} else if (strcmp(found->e_path, exp->m_export.e_path)
+			} else if (strcmp(found->e_path, exp->m_export.e_path) != 0
 				   && !subexport(found, &exp->m_export))
 			{
 				xlog(L_WARNING, "%s and %s have same filehandle for %s, using first",
 				     found_path, path, dom);
+			} else {
+				/* same path, if one is V4ROOT, choose the other */
+				if (found->e_flags & NFSEXP_V4ROOT) {
+					found = &exp->m_export;
+					free(found_path);
+					found_path = strdup(path);
+					if (found_path == NULL)
+						goto out;
+				}
 			}
 		}
 	}

[PATCH 4/6] v4set_root: force "fsid=0" for all exports of '/'

[Neil Brown]

When "fsid=0" is not explicitly given in /etc/exports,
v4set_root creates a pseudo (NFSEXP_V4ROOT) export for '/'
with fsid 0 so that an NFSv4 client can find the root.

However if '/' is explicitly exported to the client, then that
explicit export must be used, and it will not have fsid=0.
So we must impose fsid=0 on all exports of '/'.
Without this, if '/' is exported to a client, that client will
not be able to mount '/' with NFSv4.

Signed-off-by: NeilBrown <neilb@suse.de>
---

 utils/mountd/v4root.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/utils/mountd/v4root.c b/utils/mountd/v4root.c
index 81f813b..b4fdcce 100644
--- a/utils/mountd/v4root.c
+++ b/utils/mountd/v4root.c
@@ -192,6 +192,13 @@ v4root_set()
 				 */
 				continue;
 
+			if (strcmp(exp->m_export.e_path, "/") == 0 &&
+			    !(exp->m_export.e_flags & NFSEXP_FSID)) {
+				/* Force '/' to be exported as fsid == 0*/
+				exp->m_export.e_flags |= NFSEXP_FSID;
+				exp->m_export.e_fsid = 0;
+			}
+
 			v4root_add_parents(exp);
 			/* XXX: error handling! */
 		}

Re: [PATCH 4/6] v4set_root: force "fsid=0" for all exports of '/'

[J. Bruce Fields]

On Tue, Apr 24, 2012 at 03:46:38PM +1000, Neil Brown wrote:
> When "fsid=0" is not explicitly given in /etc/exports,
> v4set_root creates a pseudo (NFSEXP_V4ROOT) export for '/'
> with fsid 0 so that an NFSv4 client can find the root.
> 
> However if '/' is explicitly exported to the client, then that
> explicit export must be used, and it will not have fsid=0.
> So we must impose fsid=0 on all exports of '/'.
> Without this, if '/' is exported to a client, that client will
> not be able to mount '/' with NFSv4.

I'd also like to teach the kernel to do lookups of "/" when it wants the
pseudoroot, and then try searching for an fsid=0 export only if "/"
doesn't work.  Long term I'd rather not have the reliance on the fsid=0
convention.  (Though maybe I'm being silly, as we're likely stuck with
it.)

Anyway, I'm fine with this patch regardless.

--b.

> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> 
>  utils/mountd/v4root.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/utils/mountd/v4root.c b/utils/mountd/v4root.c
> index 81f813b..b4fdcce 100644
> --- a/utils/mountd/v4root.c
> +++ b/utils/mountd/v4root.c
> @@ -192,6 +192,13 @@ v4root_set()
>  				 */
>  				continue;
>  
> +			if (strcmp(exp->m_export.e_path, "/") == 0 &&
> +			    !(exp->m_export.e_flags & NFSEXP_FSID)) {
> +				/* Force '/' to be exported as fsid == 0*/
> +				exp->m_export.e_flags |= NFSEXP_FSID;
> +				exp->m_export.e_fsid = 0;
> +			}
> +
>  			v4root_add_parents(exp);
>  			/* XXX: error handling! */
>  		}
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] auth_authenticate_newcache: prefer non-V4ROOT export over V4ROOT exports.

[Neil Brown]

Currently auth_authenticate_internal finds an export and if it
is a V4ROOT export, it reports that there is no match.  Unlike
lookup_export() it doesn't keep looking for an acceptable export.

So remove the test from auth_authenticate_internal(), and add it to
auth_authenticate_newcache(), where the search can be allowed to
continue.

Signed-off-by: NeilBrown <neilb@suse.de>
---

 utils/mountd/auth.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
index ccc849a..3843756 100644
--- a/utils/mountd/auth.c
+++ b/utils/mountd/auth.c
@@ -159,6 +159,9 @@ auth_authenticate_newcache(const struct sockaddr *caller,
 				continue;
 			if (use_ipaddr && !client_check(exp->m_client, ai))
 				continue;
+			if (exp->m_export.e_flags & NFSEXP_V4ROOT)
+				/* not acceptable for v[23] export */
+				continue;
 			break;
 		}
 	*error = not_exported;
@@ -187,10 +190,6 @@ auth_authenticate_internal(const struct sockaddr *caller, const char *path,
 			return NULL;
 		}
 	}
-	if (exp->m_export.e_flags & NFSEXP_V4ROOT) {
-		*error = no_entry;
-		return NULL;
-	}
 	if (!(exp->m_export.e_flags & NFSEXP_INSECURE_PORT) &&
 		     nfs_get_port(caller) >= IPPORT_RESERVED) {
 		*error = illegal_port;

[PATCH 5/6] v4root: set the time-to-live for V4ROOT exports to the DEFAULT.

[Neil Brown]

e_ttl is set to the default in init_exportent().
However V4ROOT exports never see init_exportent() as they are created
with dupexportent from a template.  So e_ttl does not get set and
export entries expire immediately.
This results in an upcall to mountd every time a V4ROOT directory
in accessed.

So set e_ttl in the template.

Signed-off-by: NeilBrown <neilb@suse.de>
---

 utils/mountd/v4root.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/utils/mountd/v4root.c b/utils/mountd/v4root.c
index b4fdcce..57ee0b2 100644
--- a/utils/mountd/v4root.c
+++ b/utils/mountd/v4root.c
@@ -46,6 +46,7 @@ static nfs_export pseudo_root = {
 		.e_nsqgids = 0,
 		.e_fsid = 0,
 		.e_mountpoint = NULL,
+		.e_ttl = DEFAULT_TTL,
 	},
 	.m_exported = 0,
 	.m_xtabent = 1,

Re: [PATCH 0/6] fix some problems with v4root handling in mountd

[J. Bruce Fields]

On Tue, Apr 24, 2012 at 03:46:38PM +1000, Neil Brown wrote:
> First I'd like to say that I love seeing comments that explain the
> code..  What way when the code does something different to the
> comment, I can be sure I'm on to something :-)

ACK to all of these from me, thanks!

--b.

> 
> If I have exports:
> 
>  /  *(rw,no_root_squash)
>  /home 192.168.1.2(rw,no_root_squash)
> 
> Then I want to export 'root' to everything (on my side of a firewall)
> but '/home' only to one machine.
> However this doesn't work for host 192.168.1.2.
> If I try to mount '/' with NFSv3, it fails.
> If I try to mount '/' with NFSv4, then it works but we only see
> "home" in the mounted "/".
> 
> The problem is that the export of "/home" causes a V4ROOT export
> of "/" to be created for 192.168.1.2 and as MCL_SUBNETWORK comes
> before MCL_WILDCARD, the V4ROOT export wins and the other wildcard
> export isn't even tried.
> 
> So the following patches:
>  1/ fix the NFSv4 mount case so that we really see '/' and not a pseudo
>     '/'.  This is the case where the comment says the right thing
>     but the code doesn't.
> 
>  2/ fix 'mountd' lookup for NFSv2/v3 so they successfully mount
>     the real export, and don't fail because only the V4ROOT export
>     was found,
> 
>  3/ Fix a similar ordering problem in nfsd_fh.  Without this an
>     NFSv4 client might see the V4ROOT export for '/' rather than
>     the explicitly exported export.
> 
>  4/ Set a useful ttl on V4ROOT exports so they don't need to
>     constantly be refreshed, an
>   
>  5/ fix a buglet which didn't cause any of these problems but
>     certainly looks wrong.
> 
> Enjoy :-)
> 
> NeilBrown
> 
> ---
> 
> Neil Brown (6):
>       v4_root_add_parents: remove a possible buffer overflow.
>       v4root: set the time-to-live for V4ROOT exports to the DEFAULT.
>       v4set_root: force "fsid=0" for all exports of '/'
>       nfsd_fh: if two exports are possible, choose the one without V4ROOT
>       auth_authenticate_newcache: prefer non-V4ROOT export over V4ROOT exports.
>       lookup_export: really prefer nonV4ROOT exports.
> 
> 
>  utils/mountd/auth.c   |    7 +++----
>  utils/mountd/cache.c  |   21 +++++++++++++++++----
>  utils/mountd/v4root.c |   12 ++++++++++--
>  3 files changed, 30 insertions(+), 10 deletions(-)
> 
> -- 
> Signature
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] fix some problems with v4root handling in mountd

[Steve Dickson]

On 04/24/2012 01:46 AM, Neil Brown wrote:
> First I'd like to say that I love seeing comments that explain the
> code..  What way when the code does something different to the
> comment, I can be sure I'm on to something :-)
> 
> If I have exports:
> 
>  /  *(rw,no_root_squash)
>  /home 192.168.1.2(rw,no_root_squash)
> 
> Then I want to export 'root' to everything (on my side of a firewall)
> but '/home' only to one machine.
> However this doesn't work for host 192.168.1.2.
> If I try to mount '/' with NFSv3, it fails.
> If I try to mount '/' with NFSv4, then it works but we only see
> "home" in the mounted "/".
> 
> The problem is that the export of "/home" causes a V4ROOT export
> of "/" to be created for 192.168.1.2 and as MCL_SUBNETWORK comes
> before MCL_WILDCARD, the V4ROOT export wins and the other wildcard
> export isn't even tried.
> 
> So the following patches:
>  1/ fix the NFSv4 mount case so that we really see '/' and not a pseudo
>     '/'.  This is the case where the comment says the right thing
>     but the code doesn't.
> 
>  2/ fix 'mountd' lookup for NFSv2/v3 so they successfully mount
>     the real export, and don't fail because only the V4ROOT export
>     was found,
> 
>  3/ Fix a similar ordering problem in nfsd_fh.  Without this an
>     NFSv4 client might see the V4ROOT export for '/' rather than
>     the explicitly exported export.
> 
>  4/ Set a useful ttl on V4ROOT exports so they don't need to
>     constantly be refreshed, an
>   
>  5/ fix a buglet which didn't cause any of these problems but
>     certainly looks wrong.
Committed all 6... thanks!

steved.

> 
> Enjoy :-)
> 
> NeilBrown
> 
> ---
> 
> Neil Brown (6):
>       v4_root_add_parents: remove a possible buffer overflow.
>       v4root: set the time-to-live for V4ROOT exports to the DEFAULT.
>       v4set_root: force "fsid=0" for all exports of '/'
>       nfsd_fh: if two exports are possible, choose the one without V4ROOT
>       auth_authenticate_newcache: prefer non-V4ROOT export over V4ROOT exports.
>       lookup_export: really prefer nonV4ROOT exports.
> 
> 
>  utils/mountd/auth.c   |    7 +++----
>  utils/mountd/cache.c  |   21 +++++++++++++++++----
>  utils/mountd/v4root.c |   12 ++++++++++--
>  3 files changed, 30 insertions(+), 10 deletions(-)
> 

Re: [PATCH 0/6] fix some problems with v4root handling in mountd

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 207 bytes --]

On Tue, 01 May 2012 15:33:11 -0400 Steve Dickson <SteveD@redhat.com> wrote:

> Committed all 6... thanks!
> 
> steved.
> 

Thanks.
Any idea when 1.2.6-final might be expected?

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 0/6] fix some problems with v4root handling in mountd

[Steve Dickson]

On 05/01/2012 09:23 PM, NeilBrown wrote:
> On Tue, 01 May 2012 15:33:11 -0400 Steve Dickson <SteveD@redhat.com> wrote:
> 
>> Committed all 6... thanks!
>>
>> steved.
>>
> 
> Thanks.
> Any idea when 1.2.6-final might be expected?
I'm was waiting on Bruce's use_ipaddr race patches to pull
the trigger but I see they were posted last night... 

Depending on how the testing goes I'll hopeful to have the
1.2.6 release out sometime next week... 

steved.

Re: [PATCH 0/6] fix some problems with v4root handling in mountd

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 620 bytes --]

On Wed, 02 May 2012 06:32:20 -0400 Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> On 05/01/2012 09:23 PM, NeilBrown wrote:
> > On Tue, 01 May 2012 15:33:11 -0400 Steve Dickson <SteveD@redhat.com> wrote:
> > 
> >> Committed all 6... thanks!
> >>
> >> steved.
> >>
> > 
> > Thanks.
> > Any idea when 1.2.6-final might be expected?
> I'm was waiting on Bruce's use_ipaddr race patches to pull
> the trigger but I see they were posted last night... 
> 
> Depending on how the testing goes I'll hopeful to have the
> 1.2.6 release out sometime next week... 

Execellent, thanks.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]