* [PATCH] exportfs: Make sure pass all valid export flags to nfsd @ 2016-12-31 13:05 Kinglong Mee 2017-01-04 16:56 ` Steve Dickson 2017-01-06 21:05 ` J. Bruce Fields 0 siblings, 2 replies; 8+ messages in thread From: Kinglong Mee @ 2016-12-31 13:05 UTC (permalink / raw) To: Steve Dickson, linux-nfs; +Cc: kinglongmee, Christoph Hellwig test_export pass a export flags only marks NFSEXP_FSID, nfsd may want other flags for export checking. This patch make sure exportfs pass all other flags to nfsd. Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> --- utils/exportfs/exportfs.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c index 15a1583..bacf106 100644 --- a/utils/exportfs/exportfs.c +++ b/utils/exportfs/exportfs.c @@ -473,8 +473,10 @@ static int can_test(void) return 1; } -static int test_export(char *path, int with_fsid) +static int test_export(nfs_export *exp, int with_fsid) { + char *path = exp->m_export.e_path; + int flags = exp->m_export.e_flags | (with_fsid ? NFSEXP_FSID : 0); /* beside max path, buf size should take protocol str into account */ char buf[NFS_MAXPATHLEN+1+64] = { 0 }; char *bp = buf; @@ -487,7 +489,7 @@ static int test_export(char *path, int with_fsid) qword_add(&bp, &len, path); if (len < 1) return 0; - snprintf(bp, len, " 3 %d 65534 65534 0\n", with_fsid ? NFSEXP_FSID : 0); + snprintf(bp, len, " 3 %d 65534 65534 0\n", flags); fd = open("/proc/net/rpc/nfsd.export/channel", O_WRONLY); if (fd < 0) return 0; @@ -529,12 +531,12 @@ validate_export(nfs_export *exp) if ((exp->m_export.e_flags & NFSEXP_FSID) || exp->m_export.e_uuid || fs_has_fsid) { - if ( !test_export(path, 1)) { + if ( !test_export(exp, 1)) { xlog(L_ERROR, "%s does not support NFS export", path); return; } - } else if ( ! test_export(path, 0)) { - if (test_export(path, 1)) + } else if ( !test_export(exp, 0)) { + if (test_export(exp, 1)) xlog(L_ERROR, "%s requires fsid= for NFS export", path); else xlog(L_ERROR, "%s does not support NFS export", path); -- 2.9.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] exportfs: Make sure pass all valid export flags to nfsd 2016-12-31 13:05 [PATCH] exportfs: Make sure pass all valid export flags to nfsd Kinglong Mee @ 2017-01-04 16:56 ` Steve Dickson 2017-01-06 21:05 ` J. Bruce Fields 1 sibling, 0 replies; 8+ messages in thread From: Steve Dickson @ 2017-01-04 16:56 UTC (permalink / raw) To: Kinglong Mee, linux-nfs; +Cc: Christoph Hellwig On 12/31/2016 08:05 AM, Kinglong Mee wrote: > test_export pass a export flags only marks NFSEXP_FSID, > nfsd may want other flags for export checking. > This patch make sure exportfs pass all other flags to nfsd. > > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> Committed! steved. > --- > utils/exportfs/exportfs.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c > index 15a1583..bacf106 100644 > --- a/utils/exportfs/exportfs.c > +++ b/utils/exportfs/exportfs.c > @@ -473,8 +473,10 @@ static int can_test(void) > return 1; > } > > -static int test_export(char *path, int with_fsid) > +static int test_export(nfs_export *exp, int with_fsid) > { > + char *path = exp->m_export.e_path; > + int flags = exp->m_export.e_flags | (with_fsid ? NFSEXP_FSID : 0); > /* beside max path, buf size should take protocol str into account */ > char buf[NFS_MAXPATHLEN+1+64] = { 0 }; > char *bp = buf; > @@ -487,7 +489,7 @@ static int test_export(char *path, int with_fsid) > qword_add(&bp, &len, path); > if (len < 1) > return 0; > - snprintf(bp, len, " 3 %d 65534 65534 0\n", with_fsid ? NFSEXP_FSID : 0); > + snprintf(bp, len, " 3 %d 65534 65534 0\n", flags); > fd = open("/proc/net/rpc/nfsd.export/channel", O_WRONLY); > if (fd < 0) > return 0; > @@ -529,12 +531,12 @@ validate_export(nfs_export *exp) > > if ((exp->m_export.e_flags & NFSEXP_FSID) || exp->m_export.e_uuid || > fs_has_fsid) { > - if ( !test_export(path, 1)) { > + if ( !test_export(exp, 1)) { > xlog(L_ERROR, "%s does not support NFS export", path); > return; > } > - } else if ( ! test_export(path, 0)) { > - if (test_export(path, 1)) > + } else if ( !test_export(exp, 0)) { > + if (test_export(exp, 1)) > xlog(L_ERROR, "%s requires fsid= for NFS export", path); > else > xlog(L_ERROR, "%s does not support NFS export", path); > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] exportfs: Make sure pass all valid export flags to nfsd 2016-12-31 13:05 [PATCH] exportfs: Make sure pass all valid export flags to nfsd Kinglong Mee 2017-01-04 16:56 ` Steve Dickson @ 2017-01-06 21:05 ` J. Bruce Fields 2017-01-07 11:57 ` Kinglong Mee 1 sibling, 1 reply; 8+ messages in thread From: J. Bruce Fields @ 2017-01-06 21:05 UTC (permalink / raw) To: Kinglong Mee; +Cc: Steve Dickson, linux-nfs, Christoph Hellwig On Sat, Dec 31, 2016 at 09:05:11PM +0800, Kinglong Mee wrote: > test_export pass a export flags only marks NFSEXP_FSID, > nfsd may want other flags for export checking. Why? What problem does this fix? --b. > This patch make sure exportfs pass all other flags to nfsd. > > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> > --- > utils/exportfs/exportfs.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c > index 15a1583..bacf106 100644 > --- a/utils/exportfs/exportfs.c > +++ b/utils/exportfs/exportfs.c > @@ -473,8 +473,10 @@ static int can_test(void) > return 1; > } > > -static int test_export(char *path, int with_fsid) > +static int test_export(nfs_export *exp, int with_fsid) > { > + char *path = exp->m_export.e_path; > + int flags = exp->m_export.e_flags | (with_fsid ? NFSEXP_FSID : 0); > /* beside max path, buf size should take protocol str into account */ > char buf[NFS_MAXPATHLEN+1+64] = { 0 }; > char *bp = buf; > @@ -487,7 +489,7 @@ static int test_export(char *path, int with_fsid) > qword_add(&bp, &len, path); > if (len < 1) > return 0; > - snprintf(bp, len, " 3 %d 65534 65534 0\n", with_fsid ? NFSEXP_FSID : 0); > + snprintf(bp, len, " 3 %d 65534 65534 0\n", flags); > fd = open("/proc/net/rpc/nfsd.export/channel", O_WRONLY); > if (fd < 0) > return 0; > @@ -529,12 +531,12 @@ validate_export(nfs_export *exp) > > if ((exp->m_export.e_flags & NFSEXP_FSID) || exp->m_export.e_uuid || > fs_has_fsid) { > - if ( !test_export(path, 1)) { > + if ( !test_export(exp, 1)) { > xlog(L_ERROR, "%s does not support NFS export", path); > return; > } > - } else if ( ! test_export(path, 0)) { > - if (test_export(path, 1)) > + } else if ( !test_export(exp, 0)) { > + if (test_export(exp, 1)) > xlog(L_ERROR, "%s requires fsid= for NFS export", path); > else > xlog(L_ERROR, "%s does not support NFS export", path); > -- > 2.9.3 > > -- > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] exportfs: Make sure pass all valid export flags to nfsd 2017-01-06 21:05 ` J. Bruce Fields @ 2017-01-07 11:57 ` Kinglong Mee 2017-01-11 14:16 ` J. Bruce Fields 0 siblings, 1 reply; 8+ messages in thread From: Kinglong Mee @ 2017-01-07 11:57 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Steve Dickson, linux-nfs, Christoph Hellwig, kinglongmee On 1/7/2017 05:05, J. Bruce Fields wrote: > On Sat, Dec 31, 2016 at 09:05:11PM +0800, Kinglong Mee wrote: >> test_export pass a export flags only marks NFSEXP_FSID, >> nfsd may want other flags for export checking. > > Why? What problem does this fix? When testing the patch "NFSD: Only support readonly export for !fsync and readonly filesystem", I found exportfs don't pass all valid export flags to nfsd. So, make this patch. thanks, Kinglong Mee > > --b. > >> This patch make sure exportfs pass all other flags to nfsd. >> >> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> >> --- >> utils/exportfs/exportfs.c | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c >> index 15a1583..bacf106 100644 >> --- a/utils/exportfs/exportfs.c >> +++ b/utils/exportfs/exportfs.c >> @@ -473,8 +473,10 @@ static int can_test(void) >> return 1; >> } >> >> -static int test_export(char *path, int with_fsid) >> +static int test_export(nfs_export *exp, int with_fsid) >> { >> + char *path = exp->m_export.e_path; >> + int flags = exp->m_export.e_flags | (with_fsid ? NFSEXP_FSID : 0); >> /* beside max path, buf size should take protocol str into account */ >> char buf[NFS_MAXPATHLEN+1+64] = { 0 }; >> char *bp = buf; >> @@ -487,7 +489,7 @@ static int test_export(char *path, int with_fsid) >> qword_add(&bp, &len, path); >> if (len < 1) >> return 0; >> - snprintf(bp, len, " 3 %d 65534 65534 0\n", with_fsid ? NFSEXP_FSID : 0); >> + snprintf(bp, len, " 3 %d 65534 65534 0\n", flags); >> fd = open("/proc/net/rpc/nfsd.export/channel", O_WRONLY); >> if (fd < 0) >> return 0; >> @@ -529,12 +531,12 @@ validate_export(nfs_export *exp) >> >> if ((exp->m_export.e_flags & NFSEXP_FSID) || exp->m_export.e_uuid || >> fs_has_fsid) { >> - if ( !test_export(path, 1)) { >> + if ( !test_export(exp, 1)) { >> xlog(L_ERROR, "%s does not support NFS export", path); >> return; >> } >> - } else if ( ! test_export(path, 0)) { >> - if (test_export(path, 1)) >> + } else if ( !test_export(exp, 0)) { >> + if (test_export(exp, 1)) >> xlog(L_ERROR, "%s requires fsid= for NFS export", path); >> else >> xlog(L_ERROR, "%s does not support NFS export", path); >> -- >> 2.9.3 >> >> -- >> 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] exportfs: Make sure pass all valid export flags to nfsd 2017-01-07 11:57 ` Kinglong Mee @ 2017-01-11 14:16 ` J. Bruce Fields 2017-01-15 7:43 ` Kinglong Mee 0 siblings, 1 reply; 8+ messages in thread From: J. Bruce Fields @ 2017-01-11 14:16 UTC (permalink / raw) To: Kinglong Mee; +Cc: Steve Dickson, linux-nfs, Christoph Hellwig On Sat, Jan 07, 2017 at 07:57:28PM +0800, Kinglong Mee wrote: > On 1/7/2017 05:05, J. Bruce Fields wrote: > > On Sat, Dec 31, 2016 at 09:05:11PM +0800, Kinglong Mee wrote: > >> test_export pass a export flags only marks NFSEXP_FSID, > >> nfsd may want other flags for export checking. > > > > Why? What problem does this fix? > > When testing the patch "NFSD: Only support readonly export for > !fsync and readonly filesystem", I found exportfs don't pass > all valid export flags to nfsd. So, make this patch. This function is meant to test whether a filesystem supports nfs export or not. It doesn't need the full set of export flags. Off the top of my head I can't see reason this would cause problems, but I'm not convinced it's safe, either. (New nfs-utils against old kernels might be a case to check, e.g. to see how unsupported flags are handled.) So until we have a reason we *need* this, just to be safe, I'd prefer to revert this patch. --b. > > thanks, > Kinglong Mee > > > > > --b. > > > >> This patch make sure exportfs pass all other flags to nfsd. > >> > >> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> > >> --- > >> utils/exportfs/exportfs.c | 12 +++++++----- > >> 1 file changed, 7 insertions(+), 5 deletions(-) > >> > >> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c > >> index 15a1583..bacf106 100644 > >> --- a/utils/exportfs/exportfs.c > >> +++ b/utils/exportfs/exportfs.c > >> @@ -473,8 +473,10 @@ static int can_test(void) > >> return 1; > >> } > >> > >> -static int test_export(char *path, int with_fsid) > >> +static int test_export(nfs_export *exp, int with_fsid) > >> { > >> + char *path = exp->m_export.e_path; > >> + int flags = exp->m_export.e_flags | (with_fsid ? NFSEXP_FSID : 0); > >> /* beside max path, buf size should take protocol str into account */ > >> char buf[NFS_MAXPATHLEN+1+64] = { 0 }; > >> char *bp = buf; > >> @@ -487,7 +489,7 @@ static int test_export(char *path, int with_fsid) > >> qword_add(&bp, &len, path); > >> if (len < 1) > >> return 0; > >> - snprintf(bp, len, " 3 %d 65534 65534 0\n", with_fsid ? NFSEXP_FSID : 0); > >> + snprintf(bp, len, " 3 %d 65534 65534 0\n", flags); > >> fd = open("/proc/net/rpc/nfsd.export/channel", O_WRONLY); > >> if (fd < 0) > >> return 0; > >> @@ -529,12 +531,12 @@ validate_export(nfs_export *exp) > >> > >> if ((exp->m_export.e_flags & NFSEXP_FSID) || exp->m_export.e_uuid || > >> fs_has_fsid) { > >> - if ( !test_export(path, 1)) { > >> + if ( !test_export(exp, 1)) { > >> xlog(L_ERROR, "%s does not support NFS export", path); > >> return; > >> } > >> - } else if ( ! test_export(path, 0)) { > >> - if (test_export(path, 1)) > >> + } else if ( !test_export(exp, 0)) { > >> + if (test_export(exp, 1)) > >> xlog(L_ERROR, "%s requires fsid= for NFS export", path); > >> else > >> xlog(L_ERROR, "%s does not support NFS export", path); > >> -- > >> 2.9.3 > >> > >> -- > >> 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 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] exportfs: Make sure pass all valid export flags to nfsd 2017-01-11 14:16 ` J. Bruce Fields @ 2017-01-15 7:43 ` Kinglong Mee 2017-01-16 16:54 ` J. Bruce Fields 0 siblings, 1 reply; 8+ messages in thread From: Kinglong Mee @ 2017-01-15 7:43 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Steve Dickson, linux-nfs, Christoph Hellwig, Kinglong Mee On 1/11/2017 22:16, J. Bruce Fields wrote: > On Sat, Jan 07, 2017 at 07:57:28PM +0800, Kinglong Mee wrote: >> On 1/7/2017 05:05, J. Bruce Fields wrote: >>> On Sat, Dec 31, 2016 at 09:05:11PM +0800, Kinglong Mee wrote: >>>> test_export pass a export flags only marks NFSEXP_FSID, >>>> nfsd may want other flags for export checking. >>> >>> Why? What problem does this fix? >> >> When testing the patch "NFSD: Only support readonly export for >> !fsync and readonly filesystem", I found exportfs don't pass >> all valid export flags to nfsd. So, make this patch. > > This function is meant to test whether a filesystem supports nfs export > or not. It doesn't need the full set of export flags. Off the top of > my head I can't see reason this would cause problems, but I'm not > convinced it's safe, either. (New nfs-utils against old kernels might > be a case to check, e.g. to see how unsupported flags are handled.) There are two cases that passing the flags to nfsd, 1, exportfs checks the export entry, only NFSEXP_FSID without this patch, 2, mountd pass the validate export entry, all validate export flags. When the new nfs-utils against old kernels, user specifies an unsupported flags(Not NFSEXP_FSID), exportfs doesn't warning that unsupported flags (without test), mountd doesn't get the export entry for the flags. If exportfs warning the unsupported flags, user can modify it and do the right export. Do I have an exact understanding? thanks, Kinglong Mee > > So until we have a reason we *need* this, just to be safe, I'd prefer to > revert this patch. > > --b. > >> >> thanks, >> Kinglong Mee >> >>> >>> --b. >>> >>>> This patch make sure exportfs pass all other flags to nfsd. >>>> >>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> >>>> --- >>>> utils/exportfs/exportfs.c | 12 +++++++----- >>>> 1 file changed, 7 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c >>>> index 15a1583..bacf106 100644 >>>> --- a/utils/exportfs/exportfs.c >>>> +++ b/utils/exportfs/exportfs.c >>>> @@ -473,8 +473,10 @@ static int can_test(void) >>>> return 1; >>>> } >>>> >>>> -static int test_export(char *path, int with_fsid) >>>> +static int test_export(nfs_export *exp, int with_fsid) >>>> { >>>> + char *path = exp->m_export.e_path; >>>> + int flags = exp->m_export.e_flags | (with_fsid ? NFSEXP_FSID : 0); >>>> /* beside max path, buf size should take protocol str into account */ >>>> char buf[NFS_MAXPATHLEN+1+64] = { 0 }; >>>> char *bp = buf; >>>> @@ -487,7 +489,7 @@ static int test_export(char *path, int with_fsid) >>>> qword_add(&bp, &len, path); >>>> if (len < 1) >>>> return 0; >>>> - snprintf(bp, len, " 3 %d 65534 65534 0\n", with_fsid ? NFSEXP_FSID : 0); >>>> + snprintf(bp, len, " 3 %d 65534 65534 0\n", flags); >>>> fd = open("/proc/net/rpc/nfsd.export/channel", O_WRONLY); >>>> if (fd < 0) >>>> return 0; >>>> @@ -529,12 +531,12 @@ validate_export(nfs_export *exp) >>>> >>>> if ((exp->m_export.e_flags & NFSEXP_FSID) || exp->m_export.e_uuid || >>>> fs_has_fsid) { >>>> - if ( !test_export(path, 1)) { >>>> + if ( !test_export(exp, 1)) { >>>> xlog(L_ERROR, "%s does not support NFS export", path); >>>> return; >>>> } >>>> - } else if ( ! test_export(path, 0)) { >>>> - if (test_export(path, 1)) >>>> + } else if ( !test_export(exp, 0)) { >>>> + if (test_export(exp, 1)) >>>> xlog(L_ERROR, "%s requires fsid= for NFS export", path); >>>> else >>>> xlog(L_ERROR, "%s does not support NFS export", path); >>>> -- >>>> 2.9.3 >>>> >>>> -- >>>> 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 >>> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] exportfs: Make sure pass all valid export flags to nfsd 2017-01-15 7:43 ` Kinglong Mee @ 2017-01-16 16:54 ` J. Bruce Fields 2017-01-18 10:16 ` Kinglong Mee 0 siblings, 1 reply; 8+ messages in thread From: J. Bruce Fields @ 2017-01-16 16:54 UTC (permalink / raw) To: Kinglong Mee; +Cc: Steve Dickson, linux-nfs, Christoph Hellwig On Sun, Jan 15, 2017 at 03:43:21PM +0800, Kinglong Mee wrote: > On 1/11/2017 22:16, J. Bruce Fields wrote: > > On Sat, Jan 07, 2017 at 07:57:28PM +0800, Kinglong Mee wrote: > >> On 1/7/2017 05:05, J. Bruce Fields wrote: > >>> On Sat, Dec 31, 2016 at 09:05:11PM +0800, Kinglong Mee wrote: > >>>> test_export pass a export flags only marks NFSEXP_FSID, > >>>> nfsd may want other flags for export checking. > >>> > >>> Why? What problem does this fix? > >> > >> When testing the patch "NFSD: Only support readonly export for > >> !fsync and readonly filesystem", I found exportfs don't pass > >> all valid export flags to nfsd. So, make this patch. > > > > This function is meant to test whether a filesystem supports nfs export > > or not. It doesn't need the full set of export flags. Off the top of > > my head I can't see reason this would cause problems, but I'm not > > convinced it's safe, either. (New nfs-utils against old kernels might > > be a case to check, e.g. to see how unsupported flags are handled.) > > There are two cases that passing the flags to nfsd, > 1, exportfs checks the export entry, only NFSEXP_FSID without this patch, > 2, mountd pass the validate export entry, all validate export flags. > > When the new nfs-utils against old kernels, > user specifies an unsupported flags(Not NFSEXP_FSID), > exportfs doesn't warning that unsupported flags (without test), > mountd doesn't get the export entry for the flags. > > If exportfs warning the unsupported flags, user can modify it and > do the right export. > Do I have an exact understanding? OK, so when somebody specifies unsupported flags, the failures is silent and client mounts just fail. It would be better to warn when exportfs runs. I agree that that would be helpful. Have you tested those cases? (new nfs-utils, old kernel). It would be worth looking at validate_export() to see how it could improve error messages in this case. Currently it will say only: /example/export does not support NFS export or /example/export requires fsid= for export With your patch (forgive me if I misremember), I believe those are still the only error messages, so it will be confusing if, for example, you get /example/export does not support NFS export when the real problem is that an export flag is unsupported. (But maybe you had kernel messages to help there, I don't remember.) --b. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] exportfs: Make sure pass all valid export flags to nfsd 2017-01-16 16:54 ` J. Bruce Fields @ 2017-01-18 10:16 ` Kinglong Mee 0 siblings, 0 replies; 8+ messages in thread From: Kinglong Mee @ 2017-01-18 10:16 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Steve Dickson, linux-nfs, Christoph Hellwig, Kinglong Mee On 1/17/2017 00:54, J. Bruce Fields wrote: > On Sun, Jan 15, 2017 at 03:43:21PM +0800, Kinglong Mee wrote: >> On 1/11/2017 22:16, J. Bruce Fields wrote: >>> On Sat, Jan 07, 2017 at 07:57:28PM +0800, Kinglong Mee wrote: >>>> On 1/7/2017 05:05, J. Bruce Fields wrote: >>>>> On Sat, Dec 31, 2016 at 09:05:11PM +0800, Kinglong Mee wrote: >>>>>> test_export pass a export flags only marks NFSEXP_FSID, >>>>>> nfsd may want other flags for export checking. >>>>> >>>>> Why? What problem does this fix? >>>> >>>> When testing the patch "NFSD: Only support readonly export for >>>> !fsync and readonly filesystem", I found exportfs don't pass >>>> all valid export flags to nfsd. So, make this patch. >>> >>> This function is meant to test whether a filesystem supports nfs export >>> or not. It doesn't need the full set of export flags. Off the top of >>> my head I can't see reason this would cause problems, but I'm not >>> convinced it's safe, either. (New nfs-utils against old kernels might >>> be a case to check, e.g. to see how unsupported flags are handled.) >> >> There are two cases that passing the flags to nfsd, >> 1, exportfs checks the export entry, only NFSEXP_FSID without this patch, >> 2, mountd pass the validate export entry, all validate export flags. >> >> When the new nfs-utils against old kernels, >> user specifies an unsupported flags(Not NFSEXP_FSID), >> exportfs doesn't warning that unsupported flags (without test), >> mountd doesn't get the export entry for the flags. >> >> If exportfs warning the unsupported flags, user can modify it and >> do the right export. >> Do I have an exact understanding? > > OK, so when somebody specifies unsupported flags, the failures is silent > and client mounts just fail. It would be better to warn when exportfs > runs. > > I agree that that would be helpful. Have you tested those cases? (new > nfs-utils, old kernel). I test the latest nfs-utils with centos7 (3.10.0-514.2.2.el7.x86_64) which not support "security_label" flag. nfsd runs without any errors. # cat /etc/exports /nfs *(rw,insecure,no_root_squash,no_subtree_check,fsid=0,pnfs,security_label) # cat /var/lib/nfs/etab /nfs *(rw,sync,wdelay,hide,nocrossmnt,insecure,no_root_squash, no_all_squash,no_subtree_check,secure_locks,acl,security_label,pnfs,fsid=0, anonuid=65534,anongid=65534,sec=sys,insecure,no_root_squash,no_all_squash) # cat /proc/fs/nfsd/exports # Version 1.1 # Path Client(Flags) # IPs /nfs *(rw,insecure,no_root_squash,sync,wdelay,no_subtree_check,pnfs,fsid=0, uuid=767ef2f3:a8b34db9:b372b254:8cae731a,sec=1) I add trace log for export flags in svc_export as, svc_export_parse: exflags 0x224a2 "a" contains NFSEXP_SECURITY_LABEL fh_verify: exflags 0x224a2 "a" contains NFSEXP_SECURITY_LABEL if I remove "security_label" from /etc/exports, svc_export_parse: exflags 0x22422 fh_verify: exflags 0x22422 nfsd has the right export flags (include unsupported flags), but doesn't parse the unsupported flags, so that, runs without any errors. > > It would be worth looking at validate_export() to see how it could > improve error messages in this case. Currently it will say only: > > /example/export does not support NFS export > > or > > /example/export requires fsid= for export > > With your patch (forgive me if I misremember), I believe those are still > the only error messages, so it will be confusing if, for example, you > get > > /example/export does not support NFS export > > when the real problem is that an export flag is unsupported. (But maybe > you had kernel messages to help there, I don't remember.) Yes, those error messages are shortage. I will check them and try to make them better. thanks, Kinglong Mee ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-01-18 10:16 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-12-31 13:05 [PATCH] exportfs: Make sure pass all valid export flags to nfsd Kinglong Mee 2017-01-04 16:56 ` Steve Dickson 2017-01-06 21:05 ` J. Bruce Fields 2017-01-07 11:57 ` Kinglong Mee 2017-01-11 14:16 ` J. Bruce Fields 2017-01-15 7:43 ` Kinglong Mee 2017-01-16 16:54 ` J. Bruce Fields 2017-01-18 10:16 ` Kinglong Mee
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).