| Message ID | 55488006.6050502@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 05, 2015 at 04:32:06PM +0800, Kinglong Mee wrote: > ----------------------------------------------------------------------------------- > >From d831154bb7e527f9003e16ac049526be5ed90228 Mon Sep 17 00:00:00 2001 > From: Kinglong Mee <kinglongmee@gmail.com> > 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. ACK to this patch. (Steved, did you get this?) --b. > > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> > --- > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
ping .... > ----------------------------------------------------------------------------------- >>From d831154bb7e527f9003e16ac049526be5ed90228 Mon Sep 17 00:00:00 2001 > From: Kinglong Mee <kinglongmee@gmail.com> > 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@gmail.com> > --- > 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) > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 27 Jun 2015 07:14:00 +0800 Kinglong Mee <kinglongmee@gmail.com> wrote: > ping .... > > > ----------------------------------------------------------------------------------- > >>From d831154bb7e527f9003e16ac049526be5ed90228 Mon Sep 17 00:00:00 2001 > > From: Kinglong Mee <kinglongmee@gmail.com> > > 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. > > How do you know that every file system that treats multiple different strings as the same, imposes the rule that the strings must be the same length? Given how complex unicode case rules can be, I certainly wouldn't be certain of that. If the only purpose of this patch is to avoid triggering a bug in a separate piece of code, then I am not in favour of it. Thanks, NeilBrown > > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> > > --- > > 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) > > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/27/2015 07:35, NeilBrown wrote: > On Sat, 27 Jun 2015 07:14:00 +0800 Kinglong Mee <kinglongmee@gmail.com> > wrote: > >> ping .... >> >>> ----------------------------------------------------------------------------------- >>> >From d831154bb7e527f9003e16ac049526be5ed90228 Mon Sep 17 00:00:00 2001 >>> From: Kinglong Mee <kinglongmee@gmail.com> >>> 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. >>> > > How do you know that every file system that treats multiple different > strings as the same, imposes the rule that the strings must be the same > length? Agree with you. With your suggestion, there is a bug exist. 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); # rpc.mountd get parent's length. if (strcmp(parent, "/") == 0 && child[1] != 0) return 1; return (same_path(child, parent, l) && child[l] == '/'); # will truncate child's name by parent's length, also checking child[l]. # I think the length should be calculated by the count of '/'. } > > Given how complex unicode case rules can be, I certainly wouldn't be > certain of that. > > If the only purpose of this patch is to avoid triggering a bug in a > separate piece of code, then I am not in favour of it. Yes, This patch just change two condition's position. Please ignore this patch, maybe a new patch for NeilBrown's suggestion will be post. thanks, Kinglong Mee > > Thanks, > NeilBrown > > >>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> >>> --- >>> 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) >>> > > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)