[PATCH] mountd: Fix is_subdirectory again.

[PATCH] mountd: Fix is_subdirectory again.

[NeilBrown]

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



Hi Steve,
 I just noticed

commit ebe2826ca571a3959c3b5c8e29686c621f2775cf
Author: Steve Dickson <steved@redhat.com>
Date:   Sat Mar 23 10:30:17 2013 -0400

    mountd: regression in crossmounts

Sorry I missed the email you presumably sent me to let me know
that I had caused a regression.

Here is the proper fix.

Thanks,
NeilBrown


Subject: [PATCH] mountd: Fix is_subdirectory again.

commit ebe2826ca571a (mountd: regression in crossmounts)
said it was:
    Reverting the logic of commit 8e2fb3fc until
    a better solution can be found for the original
    problem.

here is the "something better".

The problem was that is_subdirectory() would also succeed if the two
directories were the same.  This is needed for path_matches() which
needs to see if the child is same-or-descendant.

So this patch rearranges path_matches() to do the "are they the same"
test itself and only bother with is_subdirectory() if it they are not
the same.

So now is_subdirectory() can be strict, and so can be usable for
subexport(), which needs a strong 'in subdirectory - not the same' test.

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

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 737927c..517aa62 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -347,20 +347,26 @@ static char *next_mnt(void **v, char *p)
 
 static int is_subdirectory(char *child, char *parent)
 {
+	/* Check is child is strictly a subdirectory of
+	 * parent or a more distant descendant.
+	 */
 	size_t l = strlen(parent);
 
-	if (strcmp(parent, "/") == 0)
+	if (strcmp(parent, "/") == 0 && child[1] != 0)
 		return 1;
 
-	return strcmp(child, parent) == 0
-		|| (strncmp(child, parent, l) == 0 && child[l] == '/');
+	return (strncmp(child, parent, l) == 0 && child[l] == '/');
 }
 
 static int path_matches(nfs_export *exp, char *path)
 {
-	if (exp->m_export.e_flags & NFSEXP_CROSSMOUNT)
-		return is_subdirectory(path, exp->m_export.e_path);
-	return strcmp(path, exp->m_export.e_path) == 0;
+	/* Does the path match the export?  I.e. is it an
+	 * exact match, or does the export have CROSSMOUNT, and path
+	 * is a descendant?
+	 */
+	return strcmp(path, exp->m_export.e_path) == 0
+		|| ((exp->m_export.e_flags & NFSEXP_CROSSMOUNT)
+		    && is_subdirectory(path, exp->m_export.e_path));
 }
 
 static int
@@ -369,15 +375,13 @@ export_matches(nfs_export *exp, char *dom, char *path, struct addrinfo *ai)
 	return path_matches(exp, path) && client_matches(exp, dom, ai);
 }
 
-/* True iff e1 is a child of e2 and e2 has crossmnt set: */
+/* True iff e1 is a child of e2 (or descendant) and e2 has crossmnt set: */
 static bool subexport(struct exportent *e1, struct exportent *e2)
 {
 	char *p1 = e1->e_path, *p2 = e2->e_path;
-	size_t l2 = strlen(p2);
 
 	return e2->e_flags & NFSEXP_CROSSMOUNT
-		&& strncmp(p1, p2, l2) == 0
-		&& p1[l2] == '/';
+		&& is_subdirectory(p1, p2);
 }
 
 struct parsed_fsid {

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

Re: [PATCH] mountd: Fix is_subdirectory again.

[J. Bruce Fields]

On Thu, May 02, 2013 at 05:05:11PM +1000, NeilBrown wrote:
> 
> 
> Hi Steve,
>  I just noticed
> 
> commit ebe2826ca571a3959c3b5c8e29686c621f2775cf
> Author: Steve Dickson <steved@redhat.com>
> Date:   Sat Mar 23 10:30:17 2013 -0400
> 
>     mountd: regression in crossmounts
> 
> Sorry I missed the email you presumably sent me to let me know
> that I had caused a regression.

Yup.  The strategy around here is to bury people in email and trust they
have industrial-strength filtering.

And I could have added Neil on the cc: too and forgot, oops:

	http://marc.info/?l=linux-nfs&m=136404930104976&w=2

> Here is the proper fix.

I'm dense.  I still had to scratch my head a while:

So: in a case like the one steved gives:

	/home *(rw,fsid=0,crossmnt)
	/home/fs1 *(rw,crossmnt)
	/home/fs1/fs2/fs3 *(rw,nohide)

where nfsd_fh is asked to look for an export with fsid=0, it gets a
match on the export of /home.  But that loop there keeps going in case
it finds some better match.  In particular it considers any mountpoints
under /home as candidates, since /home has "crossmnt".  And they all
match too.  So it considers in sequence the possibilities:

	(found=/export of /home, found_path=/home)
	(found=/export of /home, found_path=/home/fs1)
	(found=/export of /home, found_path=/home/fs1/fs2/fs3)

The subexport() test--designed to favor crossmnt exports which are
"closer" to the target filesystem--passes in each case, so we end up
taking the last.

So we pass down the longest path to the kernel, it does an export upcall
for that path, the hilarity ensues.

Anyway:

	Acked-by: J. Bruce Fields <bfields@redhat.com>

But this all still gives me a mild headache:

	- it only works if we iterate through the submounts in the
	  correct order?
	- should fsid= be inherited by crossmnt anyway?
	- nfsd_fh() is too long, and the logic (with those extra loop
	  control variables declared static inside the CROSSMOUNT case)
	  could be more straightforward.

--b.

> 
> Thanks,
> NeilBrown
> 
> 
> Subject: [PATCH] mountd: Fix is_subdirectory again.
> 
> commit ebe2826ca571a (mountd: regression in crossmounts)
> said it was:
>     Reverting the logic of commit 8e2fb3fc until
>     a better solution can be found for the original
>     problem.
> 
> here is the "something better".
> 
> The problem was that is_subdirectory() would also succeed if the two
> directories were the same.  This is needed for path_matches() which
> needs to see if the child is same-or-descendant.
> 
> So this patch rearranges path_matches() to do the "are they the same"
> test itself and only bother with is_subdirectory() if it they are not
> the same.
> 
> So now is_subdirectory() can be strict, and so can be usable for
> subexport(), which needs a strong 'in subdirectory - not the same' test.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
> index 737927c..517aa62 100644
> --- a/utils/mountd/cache.c
> +++ b/utils/mountd/cache.c
> @@ -347,20 +347,26 @@ static char *next_mnt(void **v, char *p)
>  
>  static int is_subdirectory(char *child, char *parent)
>  {
> +	/* Check is child is strictly a subdirectory of
> +	 * parent or a more distant descendant.
> +	 */
>  	size_t l = strlen(parent);
>  
> -	if (strcmp(parent, "/") == 0)
> +	if (strcmp(parent, "/") == 0 && child[1] != 0)
>  		return 1;
>  
> -	return strcmp(child, parent) == 0
> -		|| (strncmp(child, parent, l) == 0 && child[l] == '/');
> +	return (strncmp(child, parent, l) == 0 && child[l] == '/');
>  }
>  
>  static int path_matches(nfs_export *exp, char *path)
>  {
> -	if (exp->m_export.e_flags & NFSEXP_CROSSMOUNT)
> -		return is_subdirectory(path, exp->m_export.e_path);
> -	return strcmp(path, exp->m_export.e_path) == 0;
> +	/* Does the path match the export?  I.e. is it an
> +	 * exact match, or does the export have CROSSMOUNT, and path
> +	 * is a descendant?
> +	 */
> +	return strcmp(path, exp->m_export.e_path) == 0
> +		|| ((exp->m_export.e_flags & NFSEXP_CROSSMOUNT)
> +		    && is_subdirectory(path, exp->m_export.e_path));
>  }
>  
>  static int
> @@ -369,15 +375,13 @@ export_matches(nfs_export *exp, char *dom, char *path, struct addrinfo *ai)
>  	return path_matches(exp, path) && client_matches(exp, dom, ai);
>  }
>  
> -/* True iff e1 is a child of e2 and e2 has crossmnt set: */
> +/* True iff e1 is a child of e2 (or descendant) and e2 has crossmnt set: */
>  static bool subexport(struct exportent *e1, struct exportent *e2)
>  {
>  	char *p1 = e1->e_path, *p2 = e2->e_path;
> -	size_t l2 = strlen(p2);
>  
>  	return e2->e_flags & NFSEXP_CROSSMOUNT
> -		&& strncmp(p1, p2, l2) == 0
> -		&& p1[l2] == '/';
> +		&& is_subdirectory(p1, p2);
>  }
>  
>  struct parsed_fsid {

Re: [PATCH] mountd: Fix is_subdirectory again.

[NeilBrown]

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

On Thu, 2 May 2013 11:16:34 -0400 "J. Bruce Fields" <bfields@fieldses.org>
wrote:

> On Thu, May 02, 2013 at 05:05:11PM +1000, NeilBrown wrote:
> > 
> > 
> > Hi Steve,
> >  I just noticed
> > 
> > commit ebe2826ca571a3959c3b5c8e29686c621f2775cf
> > Author: Steve Dickson <steved@redhat.com>
> > Date:   Sat Mar 23 10:30:17 2013 -0400
> > 
> >     mountd: regression in crossmounts
> > 
> > Sorry I missed the email you presumably sent me to let me know
> > that I had caused a regression.
> 
> Yup.  The strategy around here is to bury people in email and trust they
> have industrial-strength filtering.
> 
> And I could have added Neil on the cc: too and forgot, oops:
> 
> 	http://marc.info/?l=linux-nfs&m=136404930104976&w=2
> 
> > Here is the proper fix.
> 
> I'm dense.  I still had to scratch my head a while:
> 
> So: in a case like the one steved gives:
> 
> 	/home *(rw,fsid=0,crossmnt)
> 	/home/fs1 *(rw,crossmnt)
> 	/home/fs1/fs2/fs3 *(rw,nohide)
> 
> where nfsd_fh is asked to look for an export with fsid=0, it gets a
> match on the export of /home.  But that loop there keeps going in case
> it finds some better match.  In particular it considers any mountpoints
> under /home as candidates, since /home has "crossmnt".  And they all
> match too.  So it considers in sequence the possibilities:
> 
> 	(found=/export of /home, found_path=/home)
> 	(found=/export of /home, found_path=/home/fs1)
> 	(found=/export of /home, found_path=/home/fs1/fs2/fs3)
> 
> The subexport() test--designed to favor crossmnt exports which are
> "closer" to the target filesystem--passes in each case, so we end up
> taking the last.
> 
> So we pass down the longest path to the kernel, it does an export upcall
> for that path, the hilarity ensues.
> 
> Anyway:
> 
> 	Acked-by: J. Bruce Fields <bfields@redhat.com>

Thanks.

> 
> But this all still gives me a mild headache:
> 
> 	- it only works if we iterate through the submounts in the
> 	  correct order?

Does it?  nfsd_fh() is trying to find the export for a given file handle.
  For every export it tries that filesystem and (if CROSSMOUNT) ever
  filesystem mounted below there.
  It only considers filesystem for which the fsid matches.
  Of those, it chooses the exp which is 'lowest' in the hierarchy.

I don't think the result of that can be dependant on order.

> 	- should fsid= be inherited by crossmnt anyway?

No...
If you have an export with fsid=N,crossmnt and a filehandle arrives for
fsid=N, then when we first look at that export match_fsid() will report a
match and 'found' will be set.
nfsd_fh() will loop around and try again for the next entry in /etc/mtab
which is under that same filesystem.  It will get a match_fsid() as well but
as subexport() will report "no" (as it is the same export), no change will be
made to found.  It also will not report that "X and Y have same file handle
for Z, using first" as the e_paths will match.

So it works as is.  Maybe the code could be made clearer though.


> 	- nfsd_fh() is too long, and the logic (with those extra loop
> 	  control variables declared static inside the CROSSMOUNT case)
> 	  could be more straightforward.

You have a history of taking my too-long functions and breaking them up into
manageable bits (kernel commit 3211af1119174fbe8b).  Maybe you could try
again :-)

NeilBrown

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

Re: [PATCH] mountd: Fix is_subdirectory again.

[J. Bruce Fields]

On Mon, May 06, 2013 at 02:44:13PM +1000, NeilBrown wrote:
> On Thu, 2 May 2013 11:16:34 -0400 "J. Bruce Fields" <bfields@fieldses.org>
> wrote:
> 
> > On Thu, May 02, 2013 at 05:05:11PM +1000, NeilBrown wrote:
> > > 
> > > 
> > > Hi Steve,
> > >  I just noticed
> > > 
> > > commit ebe2826ca571a3959c3b5c8e29686c621f2775cf
> > > Author: Steve Dickson <steved@redhat.com>
> > > Date:   Sat Mar 23 10:30:17 2013 -0400
> > > 
> > >     mountd: regression in crossmounts
> > > 
> > > Sorry I missed the email you presumably sent me to let me know
> > > that I had caused a regression.
> > 
> > Yup.  The strategy around here is to bury people in email and trust they
> > have industrial-strength filtering.
> > 
> > And I could have added Neil on the cc: too and forgot, oops:
> > 
> > 	http://marc.info/?l=linux-nfs&m=136404930104976&w=2
> > 
> > > Here is the proper fix.
> > 
> > I'm dense.  I still had to scratch my head a while:
> > 
> > So: in a case like the one steved gives:
> > 
> > 	/home *(rw,fsid=0,crossmnt)
> > 	/home/fs1 *(rw,crossmnt)
> > 	/home/fs1/fs2/fs3 *(rw,nohide)
> > 
> > where nfsd_fh is asked to look for an export with fsid=0, it gets a
> > match on the export of /home.  But that loop there keeps going in case
> > it finds some better match.  In particular it considers any mountpoints
> > under /home as candidates, since /home has "crossmnt".  And they all
> > match too.  So it considers in sequence the possibilities:
> > 
> > 	(found=/export of /home, found_path=/home)
> > 	(found=/export of /home, found_path=/home/fs1)
> > 	(found=/export of /home, found_path=/home/fs1/fs2/fs3)
> > 
> > The subexport() test--designed to favor crossmnt exports which are
> > "closer" to the target filesystem--passes in each case, so we end up
> > taking the last.
> > 
> > So we pass down the longest path to the kernel, it does an export upcall
> > for that path, the hilarity ensues.
> > 
> > Anyway:
> > 
> > 	Acked-by: J. Bruce Fields <bfields@redhat.com>
> 
> Thanks.
> 
> > 
> > But this all still gives me a mild headache:
> > 
> > 	- it only works if we iterate through the submounts in the
> > 	  correct order?
> 
> Does it?  nfsd_fh() is trying to find the export for a given file handle.
>   For every export it tries that filesystem and (if CROSSMOUNT) ever
>   filesystem mounted below there.
>   It only considers filesystem for which the fsid matches.
>   Of those, it chooses the exp which is 'lowest' in the hierarchy.
> 
> I don't think the result of that can be dependant on order.

Well, my only point was that we depend on first trying the main export
and then the filesystems mounted below--as that's probably how it would
work under any reasonable organization of the code, I suppose that's not
so fragile.

> > 	- should fsid= be inherited by crossmnt anyway?
> 
> No...
> If you have an export with fsid=N,crossmnt and a filehandle arrives for
> fsid=N, then when we first look at that export match_fsid() will report a
> match and 'found' will be set.
> nfsd_fh() will loop around and try again for the next entry in /etc/mtab
> which is under that same filesystem.  It will get a match_fsid() as well but
> as subexport() will report "no" (as it is the same export), no change will be
> made to found.  It also will not report that "X and Y have same file handle
> for Z, using first" as the e_paths will match.

Oh, I see--you're right.

> So it works as is.  Maybe the code could be made clearer though.
> 
> 
> > 	- nfsd_fh() is too long, and the logic (with those extra loop
> > 	  control variables declared static inside the CROSSMOUNT case)
> > 	  could be more straightforward.
> 
> You have a history of taking my too-long functions and breaking them up into
> manageable bits (kernel commit 3211af1119174fbe8b).  Maybe you could try
> again :-)

Yeah, yeah, yeah.  Maybe the compulsion will take me some day.  For the
sake of several other pressing projects, hopefully not today.

--b.

Re: [PATCH] mountd: Fix is_subdirectory again.

[Steve Dickson]

On 02/05/13 03:05, NeilBrown wrote:
> 
> 
> Hi Steve,
>  I just noticed
> 
> commit ebe2826ca571a3959c3b5c8e29686c621f2775cf
> Author: Steve Dickson <steved@redhat.com>
> Date:   Sat Mar 23 10:30:17 2013 -0400
> 
>     mountd: regression in crossmounts
> 
> Sorry I missed the email you presumably sent me to let me know
> that I had caused a regression.
Yes I should have... That was my bad... 
> 
> Here is the proper fix.
Yes it does work..  thanks! 

The new is committed!

steved.
> 
> Thanks,
> NeilBrown
> 
> 
> Subject: [PATCH] mountd: Fix is_subdirectory again.
> 
> commit ebe2826ca571a (mountd: regression in crossmounts)
> said it was:
>     Reverting the logic of commit 8e2fb3fc until
>     a better solution can be found for the original
>     problem.
> 
> here is the "something better".
> 
> The problem was that is_subdirectory() would also succeed if the two
> directories were the same.  This is needed for path_matches() which
> needs to see if the child is same-or-descendant.
> 
> So this patch rearranges path_matches() to do the "are they the same"
> test itself and only bother with is_subdirectory() if it they are not
> the same.
> 
> So now is_subdirectory() can be strict, and so can be usable for
> subexport(), which needs a strong 'in subdirectory - not the same' test.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
> index 737927c..517aa62 100644
> --- a/utils/mountd/cache.c
> +++ b/utils/mountd/cache.c
> @@ -347,20 +347,26 @@ static char *next_mnt(void **v, char *p)
>  
>  static int is_subdirectory(char *child, char *parent)
>  {
> +	/* Check is child is strictly a subdirectory of
> +	 * parent or a more distant descendant.
> +	 */
>  	size_t l = strlen(parent);
>  
> -	if (strcmp(parent, "/") == 0)
> +	if (strcmp(parent, "/") == 0 && child[1] != 0)
>  		return 1;
>  
> -	return strcmp(child, parent) == 0
> -		|| (strncmp(child, parent, l) == 0 && child[l] == '/');
> +	return (strncmp(child, parent, l) == 0 && child[l] == '/');
>  }
>  
>  static int path_matches(nfs_export *exp, char *path)
>  {
> -	if (exp->m_export.e_flags & NFSEXP_CROSSMOUNT)
> -		return is_subdirectory(path, exp->m_export.e_path);
> -	return strcmp(path, exp->m_export.e_path) == 0;
> +	/* Does the path match the export?  I.e. is it an
> +	 * exact match, or does the export have CROSSMOUNT, and path
> +	 * is a descendant?
> +	 */
> +	return strcmp(path, exp->m_export.e_path) == 0
> +		|| ((exp->m_export.e_flags & NFSEXP_CROSSMOUNT)
> +		    && is_subdirectory(path, exp->m_export.e_path));
>  }
>  
>  static int
> @@ -369,15 +375,13 @@ export_matches(nfs_export *exp, char *dom, char *path, struct addrinfo *ai)
>  	return path_matches(exp, path) && client_matches(exp, dom, ai);
>  }
>  
> -/* True iff e1 is a child of e2 and e2 has crossmnt set: */
> +/* True iff e1 is a child of e2 (or descendant) and e2 has crossmnt set: */
>  static bool subexport(struct exportent *e1, struct exportent *e2)
>  {
>  	char *p1 = e1->e_path, *p2 = e2->e_path;
> -	size_t l2 = strlen(p2);
>  
>  	return e2->e_flags & NFSEXP_CROSSMOUNT
> -		&& strncmp(p1, p2, l2) == 0
> -		&& p1[l2] == '/';
> +		&& is_subdirectory(p1, p2);
>  }
>  
>  struct parsed_fsid {
>