[PATCH] NFSD: only support readonly export for !fsync and readonly filesystem

[PATCH] NFSD: only support readonly export for !fsync and readonly filesystem

[Kinglong Mee]

Commit fae5096ad217
"nfsd: assume writeable exportabled filesystems have f_sync"
have remove the checking of f_sync.

Christoph Hellwig suggests,
"Warn and refuse the writable export."

I think just covert to a readonly export for !fsync filesystem,
also, for a readonly filesystem is reasonable.

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/export.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index 43e109c..3ec3b6b 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -358,6 +358,18 @@ static int check_export(struct inode *inode, int *flags, unsigned char *uuid)
 	if (*flags & NFSEXP_V4ROOT)
 		*flags |= NFSEXP_READONLY;
 
+	/*
+	 * Convert to a readonly export for that,
+	 * 1. not supported fsync filesystem,
+	 * 2. readonly filesystem.
+	 */
+	if ((!inode->i_fop->fsync || IS_RDONLY(inode))
+	    && !(*flags & NFSEXP_READONLY)) {
+		dprintk("exp_export: Only support readonly export "
+			"for fsync unsupported or readonly filesystem.\n");
+		*flags |= NFSEXP_READONLY;
+	}
+
 	/* There are two requirements on a filesystem to be exportable.
 	 * 1:  We must be able to identify the filesystem from a number.
 	 *       either a device number (so FS_REQUIRES_DEV needed)
-- 
2.9.3

Re: [PATCH] NFSD: only support readonly export for !fsync and readonly filesystem

[J. Bruce Fields]

On Sat, Dec 31, 2016 at 09:18:08PM +0800, Kinglong Mee wrote:
> Commit fae5096ad217
> "nfsd: assume writeable exportabled filesystems have f_sync"
> have remove the checking of f_sync.
> 
> Christoph Hellwig suggests,
> "Warn and refuse the writable export."
> 
> I think just covert to a readonly export for !fsync filesystem,
> also, for a readonly filesystem is reasonable.

Hmmm.  It's not something we've done before.  Off hand, I can't see why
it would cause a problem, but I'm not convinced yet.

Could you add to the changelog a description of the use case you gave
Christoph in your defense of this idea?

Also:

> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfsd/export.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index 43e109c..3ec3b6b 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -358,6 +358,18 @@ static int check_export(struct inode *inode, int *flags, unsigned char *uuid)
>  	if (*flags & NFSEXP_V4ROOT)
>  		*flags |= NFSEXP_READONLY;
>  
> +	/*
> +	 * Convert to a readonly export for that,
> +	 * 1. not supported fsync filesystem,
> +	 * 2. readonly filesystem.
> +	 */
> +	if ((!inode->i_fop->fsync || IS_RDONLY(inode))
> +	    && !(*flags & NFSEXP_READONLY)) {
> +		dprintk("exp_export: Only support readonly export "
> +			"for fsync unsupported or readonly filesystem.\n");

Something like this might be more helpful:

	"Filesystem %s: exporting read-only\n", IS_RDONLY(inode) ?
			"is read-only" : "has no fsync method"

Also if we passed the dentry to check_export, could we do something
like:

	"%s %s: exporting read-only\n", d_path(dentry,...), IS_RDONLY...

here and in the other warnings?

--b.

> +		*flags |= NFSEXP_READONLY;
> +	}
> +
>  	/* There are two requirements on a filesystem to be exportable.
>  	 * 1:  We must be able to identify the filesystem from a number.
>  	 *       either a device number (so FS_REQUIRES_DEV needed)
> -- 
> 2.9.3

Re: [PATCH] NFSD: only support readonly export for !fsync and readonly filesystem

[Kinglong Mee]

On Thu, Jan 5, 2017 at 1:29 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Sat, Dec 31, 2016 at 09:18:08PM +0800, Kinglong Mee wrote:
>> Commit fae5096ad217
>> "nfsd: assume writeable exportabled filesystems have f_sync"
>> have remove the checking of f_sync.
>>
>> Christoph Hellwig suggests,
>> "Warn and refuse the writable export."
>>
>> I think just covert to a readonly export for !fsync filesystem,
>> also, for a readonly filesystem is reasonable.
>
> Hmmm.  It's not something we've done before.  Off hand, I can't see why
> it would cause a problem, but I'm not convinced yet.
>
> Could you add to the changelog a description of the use case you gave
> Christoph in your defense of this idea?

Okay, I will give more description about the patch include that.

>
> Also:
>
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>> ---
>>  fs/nfsd/export.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
>> index 43e109c..3ec3b6b 100644
>> --- a/fs/nfsd/export.c
>> +++ b/fs/nfsd/export.c
>> @@ -358,6 +358,18 @@ static int check_export(struct inode *inode, int *flags, unsigned char *uuid)
>>       if (*flags & NFSEXP_V4ROOT)
>>               *flags |= NFSEXP_READONLY;
>>
>> +     /*
>> +      * Convert to a readonly export for that,
>> +      * 1. not supported fsync filesystem,
>> +      * 2. readonly filesystem.
>> +      */
>> +     if ((!inode->i_fop->fsync || IS_RDONLY(inode))
>> +         && !(*flags & NFSEXP_READONLY)) {
>> +             dprintk("exp_export: Only support readonly export "
>> +                     "for fsync unsupported or readonly filesystem.\n");
>
> Something like this might be more helpful:
>
>         "Filesystem %s: exporting read-only\n", IS_RDONLY(inode) ?
>                         "is read-only" : "has no fsync method"
>
> Also if we passed the dentry to check_export, could we do something
> like:
>
>         "%s %s: exporting read-only\n", d_path(dentry,...), IS_RDONLY...
>
> here and in the other warnings?

A kstrdup from svc_export_parse() 's string path parsing is simplify,
also, I will show in the next patch.

thanks,
Kinglong Mee

>
> --b.
>
>> +             *flags |= NFSEXP_READONLY;
>> +     }
>> +
>>       /* There are two requirements on a filesystem to be exportable.
>>        * 1:  We must be able to identify the filesystem from a number.
>>        *       either a device number (so FS_REQUIRES_DEV needed)
>> --
>> 2.9.3

[PATCH v2] NFSD: Only support readonly export for !fsync and readonly filesystem

[Kinglong Mee]

Commit fae5096ad217
"nfsd: assume writeable exportabled filesystems have f_sync"
have remove the checking of f_sync.

Christoph Hellwig suggests, "Warn and refuse the writable export."
I think just covert to a readonly export for !fsync filesystem and
readonly filesystem is reasonable.

For example,
A test directory may be mounted many underlay filesystem (contain
 ISO9660, f2fs, etc) frequently. If refuse the writeable export,
for ISO9660 the exports entry must be *(ro,...), but for f2fs
may be *(rw,...). I don't think someone wants change it every time.

Also, I let the message as a dprintk for, an export entry cache will
be dropped without any use in some times, and created in the next use.
If let the message as a warning, there will be many messages that
users maybe think that's a noise or bug.

v2, update the notice message advice from Bruce
    just pass svc_exoprt to check_export instead many parameters
    a path string for exporting entry is simply than d_path

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/export.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index 43e109c..5b9527b 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -339,8 +339,10 @@ static struct svc_export *svc_export_update(struct svc_export *new,
 					    struct svc_export *old);
 static struct svc_export *svc_export_lookup(struct svc_export *);
 
-static int check_export(struct inode *inode, int *flags, unsigned char *uuid)
+static int check_export(struct svc_export *exp, char *path_str)
 {
+	struct inode *inode = d_inode(exp->ex_path.dentry);
+	int *flags = &exp->ex_flags;
 
 	/*
 	 * We currently export only dirs, regular files, and (for v4
@@ -358,6 +360,18 @@ static int check_export(struct inode *inode, int *flags, unsigned char *uuid)
 	if (*flags & NFSEXP_V4ROOT)
 		*flags |= NFSEXP_READONLY;
 
+	/*
+	 * Convert to a readonly export for that,
+	 * 1. not supported fsync filesystem,
+	 * 2. readonly filesystem.
+	 */
+	if ((!inode->i_fop->fsync || IS_RDONLY(inode))
+	    && !(*flags & NFSEXP_READONLY)) {
+		dprintk("%s %s: exporting read-only.\n", path_str,
+			IS_RDONLY(inode) ? "is read-only" : "has no fsync method");
+		*flags |= NFSEXP_READONLY;
+	}
+
 	/* There are two requirements on a filesystem to be exportable.
 	 * 1:  We must be able to identify the filesystem from a number.
 	 *       either a device number (so FS_REQUIRES_DEV needed)
@@ -367,7 +381,7 @@ static int check_export(struct inode *inode, int *flags, unsigned char *uuid)
 	 */
 	if (!(inode->i_sb->s_type->fs_flags & FS_REQUIRES_DEV) &&
 	    !(*flags & NFSEXP_FSID) &&
-	    uuid == NULL) {
+	    exp->ex_uuid == NULL) {
 		dprintk("exp_export: export of non-dev fs without fsid\n");
 		return -EINVAL;
 	}
@@ -509,7 +523,7 @@ uuid_parse(char **mesg, char *buf, unsigned char **puuid)
 static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
 {
 	/* client path expiry [flags anonuid anongid fsid] */
-	char *buf;
+	char *buf, *path_str = NULL;
 	int len;
 	int err;
 	struct auth_domain *dom = NULL;
@@ -544,6 +558,7 @@ static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
 	if (err)
 		goto out1;
 
+	path_str = kstrdup(buf, GFP_KERNEL);
 	exp.ex_client = dom;
 	exp.cd = cd;
 	exp.ex_devid_map = NULL;
@@ -599,8 +614,7 @@ static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
 				goto out4;
 		}
 
-		err = check_export(d_inode(exp.ex_path.dentry), &exp.ex_flags,
-				   exp.ex_uuid);
+		err = check_export(&exp, path_str);
 		if (err)
 			goto out4;
 		/*
@@ -645,6 +659,7 @@ static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
 out1:
 	auth_domain_put(dom);
 out:
+	kfree(path_str);
 	kfree(buf);
 	return err;
 }
-- 
2.9.3

Re: [PATCH] NFSD: only support readonly export for !fsync and readonly filesystem

[Christoph Hellwig]

On Sat, Dec 31, 2016 at 09:18:08PM +0800, Kinglong Mee wrote:
> Commit fae5096ad217
> "nfsd: assume writeable exportabled filesystems have f_sync"
> have remove the checking of f_sync.
> 
> Christoph Hellwig suggests,
> "Warn and refuse the writable export."
> 
> I think just covert to a readonly export for !fsync filesystem,
> also, for a readonly filesystem is reasonable.

I don't like degrading the export.  We should require an explicit
ro option in this case.

Re: [PATCH] NFSD: only support readonly export for !fsync and readonly filesystem

[Kinglong Mee]

On 1/8/2017 18:07, Christoph Hellwig wrote:
> On Sat, Dec 31, 2016 at 09:18:08PM +0800, Kinglong Mee wrote:
>> Commit fae5096ad217
>> "nfsd: assume writeable exportabled filesystems have f_sync"
>> have remove the checking of f_sync.
>>
>> Christoph Hellwig suggests,
>> "Warn and refuse the writable export."
>>
>> I think just covert to a readonly export for !fsync filesystem,
>> also, for a readonly filesystem is reasonable.
> 
> I don't like degrading the export.  We should require an explicit
> ro option in this case.

With this patch, we can see the ro option in the proc file.

# mount |grep xfs
/dev/sdc on /nfs type xfs (ro,relatime,seclabel,attr2,inode64,noquota)

# cat /etc/exports
/nfs/	*(rw,no_subtree_check,no_root_squash,insecure,fsid=0)

# cat /proc/fs/nfsd/exports 
# Version 1.1
# Path Client(Flags) # IPs
/nfs	*(ro,insecure,no_root_squash,sync,wdelay,no_subtree_check,fsid=0,uuid=a4a352bc:252a47cb:b3953193:040e050d,sec=1,rw,insecure,no_root_squash)

thanks,
Kinglong Mee

Re: [PATCH] NFSD: only support readonly export for !fsync and readonly filesystem

[J. Bruce Fields]

On Sun, Jan 08, 2017 at 02:07:15AM -0800, Christoph Hellwig wrote:
> On Sat, Dec 31, 2016 at 09:18:08PM +0800, Kinglong Mee wrote:
> > Commit fae5096ad217
> > "nfsd: assume writeable exportabled filesystems have f_sync"
> > have remove the checking of f_sync.
> > 
> > Christoph Hellwig suggests,
> > "Warn and refuse the writable export."
> > 
> > I think just covert to a readonly export for !fsync filesystem,
> > also, for a readonly filesystem is reasonable.
> 
> I don't like degrading the export.

Anything there other than an intuition?

> We should require an explicit ro option in this case.

Well, I can't tell if Kinglong's case is something people are actively
complaining about or more hypotethetical, and in any case it doesn't
seem like a big deal, so I'm ignoring this for now, I guess....

--b.