On 5/5/2015 12:19 PM, NeilBrown wrote:
> 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?
Yes, that's OK.
I have think about the "/nfs/nfs" and "/nfs/123" of the first patch,
yes, that's redundant.
>
>> }
>>
>> 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 for the comments.
thanks,
Kinglong Mee
-----------------------------------------------------------------------------------
>From d831154bb7e527f9003e16ac049526be5ed90228 Mon Sep 17 00:00:00 2001
From: Kinglong Mee <kinglongmee@xxxxxxxxx>
Date: Tue, 5 May 2015 16:24:16 +0800
Subject: [PATCH] mountd: Case-insensitive path length must equals to parent
Commit 6091c0a4c4 (mountd: add support for case-insensitive file names)
introduces looking up bad path which is easy to trigger a present mutex race.
Signed-off-by: Kinglong Mee <kinglongmee@xxxxxxxxx>
---
utils/mountd/cache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 7d250f9..155695a 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -478,7 +478,7 @@ static int is_subdirectory(char *child, char *parent)
if (strcmp(parent, "/") == 0 && child[1] != 0)
return 1;
- return (same_path(child, parent, l) && child[l] == '/');
+ return (child[l] == '/' && same_path(child, parent, l));
}
static int path_matches(nfs_export *exp, char *path)
--
2.4.0
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
