Re: [PATCH RFC] NFSD: fix cannot umounting mount points under pseudo root

From: NeilBrown <neilb@suse.de>
To: Kinglong Mee <kinglongmee@gmail.com>
Cc: "J. Bruce Fields" <bfields@fieldses.org>,
	"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Steve Dickson <SteveD@redhat.com>
Subject: Re: [PATCH RFC] NFSD: fix cannot umounting mount points under pseudo root
Date: Tue, 5 May 2015 14:19:57 +1000	[thread overview]
Message-ID: <20150505141957.2aef920e@notabene.brown> (raw)
In-Reply-To: <55483EB7.5060104@gmail.com>

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

On Tue, 05 May 2015 11:53:27 +0800 Kinglong Mee <kinglongmee@gmail.com> wrote:

> Cc Steve, Viro,
> 
> On 5/1/2015 5:36 AM, J. Bruce Fields wrote:
> > On Thu, Apr 30, 2015 at 07:52:25AM +1000, NeilBrown wrote:
> >> On Wed, 29 Apr 2015 15:19:34 -0400 "J. Bruce Fields" <bfields@fieldses.org>
> >> wrote:
> >>> Maybe drop the locking from nfsd_buffered_readdir and *just* take the
> >>> i_mutex around lookup_one_len(), if that's the only place we need it?
> 
> As description in other thread, before the upcall to rpc.mountd,
> nfsd have call lookup_one_len() for the file, but why rpc.mountd
> also blocked in lookup ?
> 
> There is a bug in rpc.mountd when checking sub-directory, 
> it sets bad patch length for child.
> 
> If parent if "/nfs/xfs" and child is "/nfs/test", the child name
> will be truncated to "/nfs/tes" for strlen(parent), "/nfs/test"
> have exist in kernel's cache for the lookup_one_len(), but
> "/nfs/tes" is a bad path, which needs lookup_slow(), so blocked.

Testing for "/nfs/tes" certain seems like a wrong thing to do.

> 
> 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 && child[1] != 0)
>                 return 1;
> 
>         return (same_path(child, parent, l) && child[l] == '/');

I guess this should be:

          child[l] == '/' && same_path(child, parent, l)

That way there would be no risk of truncating anything.

Can you please test if that one-line change removes the problem?

> }
> 
> The following path makes a correct path, not a truncated path.
> Have be tested, everything is OK.
> 
> thanks,
> Kinglong Mee
> 
> -----------------------------------------------------------------------------------
> >From 70b9d1d93a24db8a7837998cb7eb0ff4e98480a6 Mon Sep 17 00:00:00 2001
> From: Kinglong Mee <kinglongmee@gmail.com>
> Date: Tue, 5 May 2015 11:47:20 +0800
> Subject: [PATCH] mountd: Case-insensitive path length must equal to parent
> 
> Commit 6091c0a4c4 (mountd: add support for case-insensitive file names)
> introdues a bug cause mutex race when looking bad path.

I think we should be clear that the mutex race is already present.
I think you are right that there is a bug here which is making it easy to
trigger, but it isn't exactly "causing" the bug.

Thanks,
NeilBrown

> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  utils/mountd/cache.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
> index 7d250f9..9d9a1bb 100644
> --- a/utils/mountd/cache.c
> +++ b/utils/mountd/cache.c
> @@ -468,16 +468,36 @@ fallback:
>  	return 1;
>  }
>  
> +static int subdir_len(char *name, int count_slashes)
> +{
> +	char *ptr = NULL;
> +	int i;
> +
> +	ptr = name;
> +	for (i = 0; i < count_slashes + 1; i++) {
> +		ptr = strchr(ptr, '/');
> +		if (NULL == ptr)
> +			return strlen(name);
> +		ptr++;
> +	}
> +
> +	return ptr - name;
> +}
> +
>  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);
> +	size_t l = subdir_len(child, count_slashes(parent));
>  
>  	if (strcmp(parent, "/") == 0 && child[1] != 0)
>  		return 1;
>  
> +	/* Case-insensitive path length must equal to parent */
> +	if (l != strlen(parent))
> +		return 0;
> +
>  	return (same_path(child, parent, l) && child[l] == '/');
>  }
>  

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]