[PATCH 1/2] net/9p: detecting invalid options as much as possible

[PATCH 1/2] net/9p: detecting invalid options as much as possible

[Chengguang Xu]

Currently when detecting invalid options in option parsing,
some options(e.g. msize) just set errno and allow to continusly
validate other options so that it can detect invalid options
as much as possible and give proper error messages together.

This patch apply this policy to all options and the case of memory
allocation error in option parsing.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
 net/9p/client.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/net/9p/client.c b/net/9p/client.c
index 21e6df1..066f136 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -186,10 +186,11 @@ static int parse_opts(char *opts, struct p9_client *clnt)
 		case Opt_trans:
 			s = match_strdup(&args[0]);
 			if (!s) {
-				ret = -ENOMEM;
+				if (!ret)
+					ret = -ENOMEM;
 				p9_debug(P9_DEBUG_ERROR,
 					 "problem allocating copy of trans arg\n");
-				goto free_and_return;
+				continue;
 			}
 
 			v9fs_put_trans(clnt->trans_mod);
@@ -199,7 +200,7 @@ static int parse_opts(char *opts, struct p9_client *clnt)
 					s);
 				ret = -EINVAL;
 				kfree(s);
-				goto free_and_return;
+				continue;
 			}
 			kfree(s);
 			break;
@@ -209,25 +210,28 @@ static int parse_opts(char *opts, struct p9_client *clnt)
 		case Opt_version:
 			s = match_strdup(&args[0]);
 			if (!s) {
-				ret = -ENOMEM;
+				if (!ret)
+					ret = -ENOMEM;
 				p9_debug(P9_DEBUG_ERROR,
 					 "problem allocating copy of version arg\n");
-				goto free_and_return;
+				continue;
 			}
 			ret = get_protocol_version(s);
 			if (ret == -EINVAL) {
 				kfree(s);
-				goto free_and_return;
+				continue;
 			}
 			kfree(s);
 			clnt->proto_version = ret;
 			break;
 		default:
+			p9_debug(P9_DEBUG_ERROR,
+				 "unrecognized option \"%s\" or missing value\n",
+					p);
 			continue;
 		}
 	}
 
-free_and_return:
 	v9fs_put_trans(clnt->trans_mod);
 	kfree(tmp_options);
 	return ret;
-- 
1.8.3.1

[PATCH 2/2] fs/9p: detecting invalid options as much as possible

[Chengguang Xu]

Currently when detecting invalid options in option parsing,
some options(e.g. debug) just set errno and allow to continusly
validate other options so that it can detect invalid options
as much as possible and give proper error messages together.

This patch apply this policy to all options and the case of memory
allocation error in option parsing.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
 fs/9p/v9fs.c | 49 ++++++++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c
index e622f0f..29ba937 100644
--- a/fs/9p/v9fs.c
+++ b/fs/9p/v9fs.c
@@ -192,10 +192,9 @@ static int v9fs_parse_options(struct v9fs_session_info *v9ses, char *opts)
 		return 0;
 
 	tmp_options = kstrdup(opts, GFP_KERNEL);
-	if (!tmp_options) {
-		ret = -ENOMEM;
-		goto fail_option_alloc;
-	}
+	if (!tmp_options)
+		return -ENOMEM;
+
 	options = tmp_options;
 
 	while ((p = strsep(&options, ",")) != NULL) {
@@ -263,18 +262,16 @@ static int v9fs_parse_options(struct v9fs_session_info *v9ses, char *opts)
 		case Opt_uname:
 			kfree(v9ses->uname);
 			v9ses->uname = match_strdup(&args[0]);
-			if (!v9ses->uname) {
-				ret = -ENOMEM;
-				goto free_and_return;
-			}
+			if (!v9ses->uname)
+				if (!ret)
+					ret = -ENOMEM;
 			break;
 		case Opt_remotename:
 			kfree(v9ses->aname);
 			v9ses->aname = match_strdup(&args[0]);
-			if (!v9ses->aname) {
-				ret = -ENOMEM;
-				goto free_and_return;
-			}
+			if (!v9ses->aname)
+				if (!ret)
+					ret = -ENOMEM;
 			break;
 		case Opt_nodevmap:
 			v9ses->nodev = 1;
@@ -292,24 +289,24 @@ static int v9fs_parse_options(struct v9fs_session_info *v9ses, char *opts)
 #ifdef CONFIG_9P_FSCACHE
 			kfree(v9ses->cachetag);
 			v9ses->cachetag = match_strdup(&args[0]);
-			if (!v9ses->cachetag) {
-				ret = -ENOMEM;
-				goto free_and_return;
-			}
+			if (!v9ses->cachetag)
+				if (!ret)
+					ret = -ENOMEM;
 #endif
 			break;
 		case Opt_cache:
 			s = match_strdup(&args[0]);
 			if (!s) {
-				ret = -ENOMEM;
+				if (!ret)
+					ret = -ENOMEM;
 				p9_debug(P9_DEBUG_ERROR,
 					 "problem allocating copy of cache arg\n");
-				goto free_and_return;
+				continue;
 			}
 			ret = get_cache_mode(s);
 			if (ret == -EINVAL) {
 				kfree(s);
-				goto free_and_return;
+				continue;
 			}
 
 			v9ses->cache = ret;
@@ -319,10 +316,11 @@ static int v9fs_parse_options(struct v9fs_session_info *v9ses, char *opts)
 		case Opt_access:
 			s = match_strdup(&args[0]);
 			if (!s) {
-				ret = -ENOMEM;
+				if (!ret)
+					ret = -ENOMEM;
 				p9_debug(P9_DEBUG_ERROR,
 					 "problem allocating copy of access arg\n");
-				goto free_and_return;
+				continue;
 			}
 
 			v9ses->flags &= ~V9FS_ACCESS_MASK;
@@ -341,14 +339,14 @@ static int v9fs_parse_options(struct v9fs_session_info *v9ses, char *opts)
 					pr_info("Unknown access argument %s\n",
 						s);
 					kfree(s);
-					goto free_and_return;
+					continue;
 				}
 				v9ses->uid = make_kuid(current_user_ns(), uid);
 				if (!uid_valid(v9ses->uid)) {
 					ret = -EINVAL;
 					pr_info("Uknown uid %s\n", s);
 					kfree(s);
-					goto free_and_return;
+					continue;
 				}
 			}
 
@@ -365,13 +363,14 @@ static int v9fs_parse_options(struct v9fs_session_info *v9ses, char *opts)
 			break;
 
 		default:
+			p9_debug(P9_DEBUG_ERROR,
+				"unrecognized mount option \"%s\" or missing value\n",
+					p);
 			continue;
 		}
 	}
 
-free_and_return:
 	kfree(tmp_options);
-fail_option_alloc:
 	return ret;
 }
 
-- 
1.8.3.1

Re: [V9fs-developer] [PATCH 2/2] fs/9p: detecting invalid options as much as possible

[Dominique Martinet]

Chengguang Xu wrote on Mon, Apr 16, 2018:
>  		default:
> +			p9_debug(P9_DEBUG_ERROR,
> +				"unrecognized mount option \"%s\" or missing value\n",
> +					p);
>  			continue;

The problem with that is that the same options are passed to the vfs,
net and transport init later on - none of which know the full subset of
valid options to tell what option has been recognized or not.

Unless we do some backward-incompatible breakage of passing all the
net/transport options in its own option (e.g. net=foo:bar:moo) there
unfortunately is no nice way of fixing this now.


(I don't mind the rest of the patches, although I'd say if we hit ENOMEM
there is likely trouble going on so I'm not so sure about hiding it if
there also is an EINVAL, but I don't really care)
-- 
Dominique Martinet | Asmadeus

Re: [V9fs-developer] [PATCH 2/2] fs/9p: detecting invalid options as much as possible

[cgxu519]

Hi Dominique,

Thanks for your quick reply and comment.


在 2018年4月16日，下午3:56，Dominique Martinet <asmadeus@codewreck.org> 写道：
> 
> Chengguang Xu wrote on Mon, Apr 16, 2018:
>> 		default:
>> +			p9_debug(P9_DEBUG_ERROR,
>> +				"unrecognized mount option \"%s\" or missing value\n",
>> +					p);
>> 			continue;
> 
> The problem with that is that the same options are passed to the vfs,
> net and transport init later on - none of which know the full subset of
> valid options to tell what option has been recognized or not.
> 
> Unless we do some backward-incompatible breakage of passing all the
> net/transport options in its own option (e.g. net=foo:bar:moo) there
> unfortunately is no nice way of fixing this now.

Yes, I agree with you.


> 
> 
> (I don't mind the rest of the patches, although I'd say if we hit ENOMEM
> there is likely trouble going on so I'm not so sure about hiding it if
> there also is an EINVAL, but I don't really care)

The initial motivation of hiding ENOMEM here is when reproducing the error，
the error code may change by system condition(more accurately memory condition),
after this patch the error code will be persistent. However, as you pointed out,
when we hit ENOMEM there would be a serious trouble, so in real life maybe we
cannot benefit from it. What do you think? 

Thanks,
Chengguang.