| Message ID | 55483EB7.5060104@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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] == '/'); > } >
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] == '/'); }