| Message ID | 20130502170511.646e2717@notabene.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 02, 2013 at 05:05:11PM +1000, NeilBrown wrote: > > > Hi Steve, > I just noticed > > commit ebe2826ca571a3959c3b5c8e29686c621f2775cf > Author: Steve Dickson <steved@redhat.com> > Date: Sat Mar 23 10:30:17 2013 -0400 > > mountd: regression in crossmounts > > Sorry I missed the email you presumably sent me to let me know > that I had caused a regression. Yup. The strategy around here is to bury people in email and trust they have industrial-strength filtering. And I could have added Neil on the cc: too and forgot, oops: http://marc.info/?l=linux-nfs&m=136404930104976&w=2 > Here is the proper fix. I'm dense. I still had to scratch my head a while: So: in a case like the one steved gives: /home *(rw,fsid=0,crossmnt) /home/fs1 *(rw,crossmnt) /home/fs1/fs2/fs3 *(rw,nohide) where nfsd_fh is asked to look for an export with fsid=0, it gets a match on the export of /home. But that loop there keeps going in case it finds some better match. In particular it considers any mountpoints under /home as candidates, since /home has "crossmnt". And they all match too. So it considers in sequence the possibilities: (found=/export of /home, found_path=/home) (found=/export of /home, found_path=/home/fs1) (found=/export of /home, found_path=/home/fs1/fs2/fs3) The subexport() test--designed to favor crossmnt exports which are "closer" to the target filesystem--passes in each case, so we end up taking the last. So we pass down the longest path to the kernel, it does an export upcall for that path, the hilarity ensues. Anyway: Acked-by: J. Bruce Fields <bfields@redhat.com> But this all still gives me a mild headache: - it only works if we iterate through the submounts in the correct order? - should fsid= be inherited by crossmnt anyway? - nfsd_fh() is too long, and the logic (with those extra loop control variables declared static inside the CROSSMOUNT case) could be more straightforward. --b. > > Thanks, > NeilBrown > > > Subject: [PATCH] mountd: Fix is_subdirectory again. > > commit ebe2826ca571a (mountd: regression in crossmounts) > said it was: > Reverting the logic of commit 8e2fb3fc until > a better solution can be found for the original > problem. > > here is the "something better". > > The problem was that is_subdirectory() would also succeed if the two > directories were the same. This is needed for path_matches() which > needs to see if the child is same-or-descendant. > > So this patch rearranges path_matches() to do the "are they the same" > test itself and only bother with is_subdirectory() if it they are not > the same. > > So now is_subdirectory() can be strict, and so can be usable for > subexport(), which needs a strong 'in subdirectory - not the same' test. > > Signed-off-by: NeilBrown <neilb@suse.de> > > diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c > index 737927c..517aa62 100644 > --- a/utils/mountd/cache.c > +++ b/utils/mountd/cache.c > @@ -347,20 +347,26 @@ static char *next_mnt(void **v, char *p) > > 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) > + if (strcmp(parent, "/") == 0 && child[1] != 0) > return 1; > > - return strcmp(child, parent) == 0 > - || (strncmp(child, parent, l) == 0 && child[l] == '/'); > + return (strncmp(child, parent, l) == 0 && child[l] == '/'); > } > > static int path_matches(nfs_export *exp, char *path) > { > - if (exp->m_export.e_flags & NFSEXP_CROSSMOUNT) > - return is_subdirectory(path, exp->m_export.e_path); > - return strcmp(path, exp->m_export.e_path) == 0; > + /* Does the path match the export? I.e. is it an > + * exact match, or does the export have CROSSMOUNT, and path > + * is a descendant? > + */ > + return strcmp(path, exp->m_export.e_path) == 0 > + || ((exp->m_export.e_flags & NFSEXP_CROSSMOUNT) > + && is_subdirectory(path, exp->m_export.e_path)); > } > > static int > @@ -369,15 +375,13 @@ export_matches(nfs_export *exp, char *dom, char *path, struct addrinfo *ai) > return path_matches(exp, path) && client_matches(exp, dom, ai); > } > > -/* True iff e1 is a child of e2 and e2 has crossmnt set: */ > +/* True iff e1 is a child of e2 (or descendant) and e2 has crossmnt set: */ > static bool subexport(struct exportent *e1, struct exportent *e2) > { > char *p1 = e1->e_path, *p2 = e2->e_path; > - size_t l2 = strlen(p2); > > return e2->e_flags & NFSEXP_CROSSMOUNT > - && strncmp(p1, p2, l2) == 0 > - && p1[l2] == '/'; > + && is_subdirectory(p1, p2); > } > > struct parsed_fsid { -- 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 Thu, 2 May 2013 11:16:34 -0400 "J. Bruce Fields" <bfields@fieldses.org> wrote: > On Thu, May 02, 2013 at 05:05:11PM +1000, NeilBrown wrote: > > > > > > Hi Steve, > > I just noticed > > > > commit ebe2826ca571a3959c3b5c8e29686c621f2775cf > > Author: Steve Dickson <steved@redhat.com> > > Date: Sat Mar 23 10:30:17 2013 -0400 > > > > mountd: regression in crossmounts > > > > Sorry I missed the email you presumably sent me to let me know > > that I had caused a regression. > > Yup. The strategy around here is to bury people in email and trust they > have industrial-strength filtering. > > And I could have added Neil on the cc: too and forgot, oops: > > http://marc.info/?l=linux-nfs&m=136404930104976&w=2 > > > Here is the proper fix. > > I'm dense. I still had to scratch my head a while: > > So: in a case like the one steved gives: > > /home *(rw,fsid=0,crossmnt) > /home/fs1 *(rw,crossmnt) > /home/fs1/fs2/fs3 *(rw,nohide) > > where nfsd_fh is asked to look for an export with fsid=0, it gets a > match on the export of /home. But that loop there keeps going in case > it finds some better match. In particular it considers any mountpoints > under /home as candidates, since /home has "crossmnt". And they all > match too. So it considers in sequence the possibilities: > > (found=/export of /home, found_path=/home) > (found=/export of /home, found_path=/home/fs1) > (found=/export of /home, found_path=/home/fs1/fs2/fs3) > > The subexport() test--designed to favor crossmnt exports which are > "closer" to the target filesystem--passes in each case, so we end up > taking the last. > > So we pass down the longest path to the kernel, it does an export upcall > for that path, the hilarity ensues. > > Anyway: > > Acked-by: J. Bruce Fields <bfields@redhat.com> Thanks. > > But this all still gives me a mild headache: > > - it only works if we iterate through the submounts in the > correct order? Does it? nfsd_fh() is trying to find the export for a given file handle. For every export it tries that filesystem and (if CROSSMOUNT) ever filesystem mounted below there. It only considers filesystem for which the fsid matches. Of those, it chooses the exp which is 'lowest' in the hierarchy. I don't think the result of that can be dependant on order. > - should fsid= be inherited by crossmnt anyway? No... If you have an export with fsid=N,crossmnt and a filehandle arrives for fsid=N, then when we first look at that export match_fsid() will report a match and 'found' will be set. nfsd_fh() will loop around and try again for the next entry in /etc/mtab which is under that same filesystem. It will get a match_fsid() as well but as subexport() will report "no" (as it is the same export), no change will be made to found. It also will not report that "X and Y have same file handle for Z, using first" as the e_paths will match. So it works as is. Maybe the code could be made clearer though. > - nfsd_fh() is too long, and the logic (with those extra loop > control variables declared static inside the CROSSMOUNT case) > could be more straightforward. You have a history of taking my too-long functions and breaking them up into manageable bits (kernel commit 3211af1119174fbe8b). Maybe you could try again :-) NeilBrown
On Mon, May 06, 2013 at 02:44:13PM +1000, NeilBrown wrote: > On Thu, 2 May 2013 11:16:34 -0400 "J. Bruce Fields" <bfields@fieldses.org> > wrote: > > > On Thu, May 02, 2013 at 05:05:11PM +1000, NeilBrown wrote: > > > > > > > > > Hi Steve, > > > I just noticed > > > > > > commit ebe2826ca571a3959c3b5c8e29686c621f2775cf > > > Author: Steve Dickson <steved@redhat.com> > > > Date: Sat Mar 23 10:30:17 2013 -0400 > > > > > > mountd: regression in crossmounts > > > > > > Sorry I missed the email you presumably sent me to let me know > > > that I had caused a regression. > > > > Yup. The strategy around here is to bury people in email and trust they > > have industrial-strength filtering. > > > > And I could have added Neil on the cc: too and forgot, oops: > > > > http://marc.info/?l=linux-nfs&m=136404930104976&w=2 > > > > > Here is the proper fix. > > > > I'm dense. I still had to scratch my head a while: > > > > So: in a case like the one steved gives: > > > > /home *(rw,fsid=0,crossmnt) > > /home/fs1 *(rw,crossmnt) > > /home/fs1/fs2/fs3 *(rw,nohide) > > > > where nfsd_fh is asked to look for an export with fsid=0, it gets a > > match on the export of /home. But that loop there keeps going in case > > it finds some better match. In particular it considers any mountpoints > > under /home as candidates, since /home has "crossmnt". And they all > > match too. So it considers in sequence the possibilities: > > > > (found=/export of /home, found_path=/home) > > (found=/export of /home, found_path=/home/fs1) > > (found=/export of /home, found_path=/home/fs1/fs2/fs3) > > > > The subexport() test--designed to favor crossmnt exports which are > > "closer" to the target filesystem--passes in each case, so we end up > > taking the last. > > > > So we pass down the longest path to the kernel, it does an export upcall > > for that path, the hilarity ensues. > > > > Anyway: > > > > Acked-by: J. Bruce Fields <bfields@redhat.com> > > Thanks. > > > > > But this all still gives me a mild headache: > > > > - it only works if we iterate through the submounts in the > > correct order? > > Does it? nfsd_fh() is trying to find the export for a given file handle. > For every export it tries that filesystem and (if CROSSMOUNT) ever > filesystem mounted below there. > It only considers filesystem for which the fsid matches. > Of those, it chooses the exp which is 'lowest' in the hierarchy. > > I don't think the result of that can be dependant on order. Well, my only point was that we depend on first trying the main export and then the filesystems mounted below--as that's probably how it would work under any reasonable organization of the code, I suppose that's not so fragile. > > - should fsid= be inherited by crossmnt anyway? > > No... > If you have an export with fsid=N,crossmnt and a filehandle arrives for > fsid=N, then when we first look at that export match_fsid() will report a > match and 'found' will be set. > nfsd_fh() will loop around and try again for the next entry in /etc/mtab > which is under that same filesystem. It will get a match_fsid() as well but > as subexport() will report "no" (as it is the same export), no change will be > made to found. It also will not report that "X and Y have same file handle > for Z, using first" as the e_paths will match. Oh, I see--you're right. > So it works as is. Maybe the code could be made clearer though. > > > > - nfsd_fh() is too long, and the logic (with those extra loop > > control variables declared static inside the CROSSMOUNT case) > > could be more straightforward. > > You have a history of taking my too-long functions and breaking them up into > manageable bits (kernel commit 3211af1119174fbe8b). Maybe you could try > again :-) Yeah, yeah, yeah. Maybe the compulsion will take me some day. For the sake of several other pressing projects, hopefully not today. --b. -- 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 02/05/13 03:05, NeilBrown wrote: > > > Hi Steve, > I just noticed > > commit ebe2826ca571a3959c3b5c8e29686c621f2775cf > Author: Steve Dickson <steved@redhat.com> > Date: Sat Mar 23 10:30:17 2013 -0400 > > mountd: regression in crossmounts > > Sorry I missed the email you presumably sent me to let me know > that I had caused a regression. Yes I should have... That was my bad... > > Here is the proper fix. Yes it does work.. thanks! The new is committed! steved. > > Thanks, > NeilBrown > > > Subject: [PATCH] mountd: Fix is_subdirectory again. > > commit ebe2826ca571a (mountd: regression in crossmounts) > said it was: > Reverting the logic of commit 8e2fb3fc until > a better solution can be found for the original > problem. > > here is the "something better". > > The problem was that is_subdirectory() would also succeed if the two > directories were the same. This is needed for path_matches() which > needs to see if the child is same-or-descendant. > > So this patch rearranges path_matches() to do the "are they the same" > test itself and only bother with is_subdirectory() if it they are not > the same. > > So now is_subdirectory() can be strict, and so can be usable for > subexport(), which needs a strong 'in subdirectory - not the same' test. > > Signed-off-by: NeilBrown <neilb@suse.de> > > diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c > index 737927c..517aa62 100644 > --- a/utils/mountd/cache.c > +++ b/utils/mountd/cache.c > @@ -347,20 +347,26 @@ static char *next_mnt(void **v, char *p) > > 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) > + if (strcmp(parent, "/") == 0 && child[1] != 0) > return 1; > > - return strcmp(child, parent) == 0 > - || (strncmp(child, parent, l) == 0 && child[l] == '/'); > + return (strncmp(child, parent, l) == 0 && child[l] == '/'); > } > > static int path_matches(nfs_export *exp, char *path) > { > - if (exp->m_export.e_flags & NFSEXP_CROSSMOUNT) > - return is_subdirectory(path, exp->m_export.e_path); > - return strcmp(path, exp->m_export.e_path) == 0; > + /* Does the path match the export? I.e. is it an > + * exact match, or does the export have CROSSMOUNT, and path > + * is a descendant? > + */ > + return strcmp(path, exp->m_export.e_path) == 0 > + || ((exp->m_export.e_flags & NFSEXP_CROSSMOUNT) > + && is_subdirectory(path, exp->m_export.e_path)); > } > > static int > @@ -369,15 +375,13 @@ export_matches(nfs_export *exp, char *dom, char *path, struct addrinfo *ai) > return path_matches(exp, path) && client_matches(exp, dom, ai); > } > > -/* True iff e1 is a child of e2 and e2 has crossmnt set: */ > +/* True iff e1 is a child of e2 (or descendant) and e2 has crossmnt set: */ > static bool subexport(struct exportent *e1, struct exportent *e2) > { > char *p1 = e1->e_path, *p2 = e2->e_path; > - size_t l2 = strlen(p2); > > return e2->e_flags & NFSEXP_CROSSMOUNT > - && strncmp(p1, p2, l2) == 0 > - && p1[l2] == '/'; > + && is_subdirectory(p1, p2); > } > > struct parsed_fsid { > -- 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 737927c..517aa62 100644 --- a/utils/mountd/cache.c +++ b/utils/mountd/cache.c @@ -347,20 +347,26 @@ static char *next_mnt(void **v, char *p) 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) + if (strcmp(parent, "/") == 0 && child[1] != 0) return 1; - return strcmp(child, parent) == 0 - || (strncmp(child, parent, l) == 0 && child[l] == '/'); + return (strncmp(child, parent, l) == 0 && child[l] == '/'); } static int path_matches(nfs_export *exp, char *path) { - if (exp->m_export.e_flags & NFSEXP_CROSSMOUNT) - return is_subdirectory(path, exp->m_export.e_path); - return strcmp(path, exp->m_export.e_path) == 0; + /* Does the path match the export? I.e. is it an + * exact match, or does the export have CROSSMOUNT, and path + * is a descendant? + */ + return strcmp(path, exp->m_export.e_path) == 0 + || ((exp->m_export.e_flags & NFSEXP_CROSSMOUNT) + && is_subdirectory(path, exp->m_export.e_path)); } static int @@ -369,15 +375,13 @@ export_matches(nfs_export *exp, char *dom, char *path, struct addrinfo *ai) return path_matches(exp, path) && client_matches(exp, dom, ai); } -/* True iff e1 is a child of e2 and e2 has crossmnt set: */ +/* True iff e1 is a child of e2 (or descendant) and e2 has crossmnt set: */ static bool subexport(struct exportent *e1, struct exportent *e2) { char *p1 = e1->e_path, *p2 = e2->e_path; - size_t l2 = strlen(p2); return e2->e_flags & NFSEXP_CROSSMOUNT - && strncmp(p1, p2, l2) == 0 - && p1[l2] == '/'; + && is_subdirectory(p1, p2); } struct parsed_fsid {
Hi Steve, I just noticed commit ebe2826ca571a3959c3b5c8e29686c621f2775cf Author: Steve Dickson <steved@redhat.com> Date: Sat Mar 23 10:30:17 2013 -0400 mountd: regression in crossmounts Sorry I missed the email you presumably sent me to let me know that I had caused a regression. Here is the proper fix. Thanks, NeilBrown Subject: [PATCH] mountd: Fix is_subdirectory again. commit ebe2826ca571a (mountd: regression in crossmounts) said it was: Reverting the logic of commit 8e2fb3fc until a better solution can be found for the original problem. here is the "something better". The problem was that is_subdirectory() would also succeed if the two directories were the same. This is needed for path_matches() which needs to see if the child is same-or-descendant. So this patch rearranges path_matches() to do the "are they the same" test itself and only bother with is_subdirectory() if it they are not the same. So now is_subdirectory() can be strict, and so can be usable for subexport(), which needs a strong 'in subdirectory - not the same' test. Signed-off-by: NeilBrown <neilb@suse.de>