All of lore.kernel.org
 help / color / mirror / Atom feed
* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.