On Tue, 05 May 2015 11:53:27 +0800 Kinglong Mee <kinglongmee@xxxxxxxxx> 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@xxxxxxxxxxxx>
> >> 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@xxxxxxxxx>
> 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@xxxxxxxxx>
> ---
> 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:
pgpKDV2QAQBBd.pgp
Description: OpenPGP digital signature
