Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=gb2312
Mime-Version: 1.0 (Mac OS X Mail 11.3 \(3445.6.18\))
Subject: Re: [V9fs-developer] [PATCH v2 1/2] net/9p: detecting invalid options
 as much as possible
From:   "cgxu519@gmx.com" <cgxu519@gmx.com>
In-Reply-To: <20180502153258.39b65209a0a85063441c6d85@linux-foundation.org>
Date:   Thu, 3 May 2018 14:14:55 +0800
Cc:     "cgxu519@gmx.com" <cgxu519@gmx.com>, ericvh@gmail.com,
        rminnich@sandia.gov, lucho@ionkov.net,
        v9fs-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8BIT
Message-Id: <71B73328-C3E3-4472-986F-644A19209F65@gmx.com>
References: <1523947502-6103-1-git-send-email-cgxu519@gmx.com>
 <20180502153258.39b65209a0a85063441c6d85@linux-foundation.org>
To:     Andrew Morton <akpm@linux-foundation.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thanks for your review, I??ll fix the issue in v3.


> ?? 2018??5??3?գ?????6:32??Andrew Morton <akpm@linux-foundation.org> д????
> 
> On Tue, 17 Apr 2018 14:45:01 +0800 Chengguang Xu <cgxu519@gmx.com> wrote:
> 
>> Currently when detecting invalid options in option parsing,
>> some options(e.g. msize) just set errno and allow to continuously
>> validate other options so that it can detect invalid options
>> as much as possible and give proper error messages together.
>> 
>> This patch applies same rule to option 'trans' and 'version'
>> when detecting EINVAL.
>> 
>> ...
> 
> A few issues:
> 
>> --- a/net/9p/client.c
>> +++ b/net/9p/client.c
>> @@ -199,7 +199,7 @@ static int parse_opts(char *opts, struct p9_client *clnt)
>> 					s);
>> 				ret = -EINVAL;
>> 				kfree(s);
>> -				goto free_and_return;
>> +				continue;
>> 			}
>> 			kfree(s);
>> 			break;
> 
> Here we can just do
> 
> --- a/net/9p/client.c~net-9p-detecting-invalid-options-as-much-as-possible-fix
> +++ a/net/9p/client.c
> @@ -198,8 +198,6 @@ static int parse_opts(char *opts, struct
> 				pr_info("Could not find request transport: %s\n",
> 					s);
> 				ret = -EINVAL;
> -				kfree(s);
> -				continue;
> 			}
> 			kfree(s);
> 			break;
> 
>> @@ -217,7 +217,7 @@ static int parse_opts(char *opts, struct p9_client *clnt)
>> 			ret = get_protocol_version(s);
> 
> And here's the patch introduces a bug: if `ret' has value -EINVAL from
> an earlier parsing error, this code will overwrite that error code with
> zero.
> 
> So you'll need to introduce a new temporary variable here.  And I
> suggest that for future-safety, we permit all error returns from
> get_protocol_version(), not just -EINVAL.  So something like:
> 
> --- a/net/9p/client.c~net-9p-detecting-invalid-options-as-much-as-possible-fix
> +++ a/net/9p/client.c
> @@ -214,13 +214,12 @@ static int parse_opts(char *opts, struct
> 					 "problem allocating copy of version arg\n");
> 				goto free_and_return;
> 			}
> -			ret = get_protocol_version(s);
> -			if (ret == -EINVAL) {
> -				kfree(s);
> -				continue;
> -			}
> +			pv = get_protocol_version(s);
> +			if (pv >= 0)
> +				clnt->proto_version = pv;
> +			else
> +				ret = pv;
> 			kfree(s);
> -			clnt->proto_version = ret;
> 			break;
> 		default:
> 			continue;
> _
> 
> 
> Similar changes can be made to patch 2/2.
>