[v4,2/2] RDMA/srp: Fix error return code in srp_parse_options()

  In the previous while loop, "ret" may be assigned zero, , so the error
return code may be incorrectly set to 0 instead of -EINVAL. Alse
investigate each case separately as Andy suggessted.

Fixes: e711f968c49c ("IB/srp: replace custom implementation of hex2bin()")
Fixes: 2a174df0c602 ("IB/srp: Use kstrtoull() instead of simple_strtoull()")
Fixes: 19f313438c77 ("IB/srp: Add RDMA/CM support")
Signed-off-by: Wang Yufen <wangyufen@huawei.com>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 71 +++++++++++++++++++++++++++++--------
 1 file changed, 56 insertions(+), 15 deletions(-)
  

On 11/28/22 18:04, Wang Yufen wrote:
> In the previous while loop, "ret" may be assigned zero, , so the error

The word "iteration" is missing from the above sentence. Additionally, 
there is a double comma.

> return code may be incorrectly set to 0 instead of -EINVAL. Alse

Alse -> Also

>   		case SRP_OPT_QUEUE_SIZE:
> -			if (match_int(args, &token) || token < 1) {
> +			ret = match_int(args, &token);
> +			if (ret)
> +				goto out;
> +			if (token < 1) {
>   				pr_warn("bad queue_size parameter '%s'\n", p);
> +				ret = -EINVAL;
>   				goto out;
>   			}
>   			target->scsi_host->can_queue = token;

Why only to report "bad queue_size parameter" if ret == 0 && token < 1 
but not if ret < 0? This is a behavior change that has not been 
explained in the patch description.

> @@ -3490,25 +3496,34 @@ static int srp_parse_options(struct net *net, const char *buf,
>   			break;
>   
>   		case SRP_OPT_MAX_CMD_PER_LUN:
> -			if (match_int(args, &token) || token < 1) {
> +			ret = match_int(args, &token);
> +			if (ret)
> +				goto out;
> +			if (token < 1) {
>   				pr_warn("bad max cmd_per_lun parameter '%s'\n",
>   					p);
> +				ret = -EINVAL;
>   				goto out;
>   			}
>   			target->scsi_host->cmd_per_lun = token;
>   			break;

Same comment here and for many other changes below.

Thanks,

Bart.
  

I'm so sorry for the poor patch description. Is the following 
description OK?

In the previous iteration of the while loop, "ret" may have been 
assigned a value of 0, so the error return code -EINVAL may have been 
incorrectly set to 0.
Also, investigate each case separately as Andy suggessted. If the help 
function match_int() fails, the error code is returned, which is 
different from the warning information printed before. If the parsing 
result token is incorrect, "-EINVAL" is returned and the original 
warning information is printed.


Thanks.

在 2022/11/30 2:43, Bart Van Assche 写道:
> On 11/28/22 18:04, Wang Yufen wrote:
>> In the previous while loop, "ret" may be assigned zero, , so the error
> 
> The word "iteration" is missing from the above sentence. Additionally, 
> there is a double comma.
> 
>> return code may be incorrectly set to 0 instead of -EINVAL. Alse
> 
> Alse -> Also
> 
>>           case SRP_OPT_QUEUE_SIZE:
>> -            if (match_int(args, &token) || token < 1) {
>> +            ret = match_int(args, &token);
>> +            if (ret)
>> +                goto out;
>> +            if (token < 1) {
>>                   pr_warn("bad queue_size parameter '%s'\n", p);
>> +                ret = -EINVAL;
>>                   goto out;
>>               }
>>               target->scsi_host->can_queue = token;
> 
> Why only to report "bad queue_size parameter" if ret == 0 && token < 1 
> but not if ret < 0? This is a behavior change that has not been 
> explained in the patch description.
> 
>> @@ -3490,25 +3496,34 @@ static int srp_parse_options(struct net *net, 
>> const char *buf,
>>               break;
>>           case SRP_OPT_MAX_CMD_PER_LUN:
>> -            if (match_int(args, &token) || token < 1) {
>> +            ret = match_int(args, &token);
>> +            if (ret)
>> +                goto out;
>> +            if (token < 1) {
>>                   pr_warn("bad max cmd_per_lun parameter '%s'\n",
>>                       p);
>> +                ret = -EINVAL;
>>                   goto out;
>>               }
>>               target->scsi_host->cmd_per_lun = token;
>>               break;
> 
> Same comment here and for many other changes below.
> 
> Thanks,
> 
> Bart.
> 
>
  

On 11/29/22 19:31, wangyufen wrote:
> I'm so sorry for the poor patch description. Is the following 
> description OK?
> 
> In the previous iteration of the while loop, "ret" may have been 
> assigned a value of 0, so the error return code -EINVAL may have been 
> incorrectly set to 0.
> Also, investigate each case separately as Andy suggessted. If the help 
> function match_int() fails, the error code is returned, which is 
> different from the warning information printed before. If the parsing 
> result token is incorrect, "-EINVAL" is returned and the original 
> warning information is printed.

Please reply below instead of above. See also 
https://en.wikipedia.org/wiki/Posting_style.

Regarding your question: not logging an error message if user input is 
rejected is unfriendly to the user. I think it's better to keep the 
behavior of reporting an error if a match* function fails instead of 
reporting in the patch description that the behavior has changed.

Thanks,

Bart.
  

在 2022/12/1 2:00, Bart Van Assche 写道:
> On 11/29/22 19:31, wangyufen wrote:
>> I'm so sorry for the poor patch description. Is the following 
>> description OK?
>>
>> In the previous iteration of the while loop, "ret" may have been 
>> assigned a value of 0, so the error return code -EINVAL may have been 
>> incorrectly set to 0.
>> Also, investigate each case separately as Andy suggessted. If the help 
>> function match_int() fails, the error code is returned, which is 
>> different from the warning information printed before. If the parsing 
>> result token is incorrect, "-EINVAL" is returned and the original 
>> warning information is printed.
> 
> Please reply below instead of above. See also 
> https://en.wikipedia.org/wiki/Posting_style.
> 

Thanks, that's helpful.

> Regarding your question: not logging an error message if user input is 
> rejected is unfriendly to the user. I think it's better to keep the 
> behavior of reporting an error if a match* function fails instead of 
> reporting in the patch description that the behavior has changed.
> 

> Thanks,
> 
> Bart.
> 
>
  

在 2022/12/1 2:00, Bart Van Assche 写道:
> On 11/29/22 19:31, wangyufen wrote:
>> I'm so sorry for the poor patch description. Is the following 
>> description OK?
>>
>> In the previous iteration of the while loop, "ret" may have been 
>> assigned a value of 0, so the error return code -EINVAL may have been 
>> incorrectly set to 0.
>> Also, investigate each case separately as Andy suggessted. If the help 
>> function match_int() fails, the error code is returned, which is 
>> different from the warning information printed before. If the parsing 
>> result token is incorrect, "-EINVAL" is returned and the original 
>> warning information is printed.
> 
> Please reply below instead of above. See also 
> https://en.wikipedia.org/wiki/Posting_style.
> 
> Regarding your question: not logging an error message if user input is 
> rejected is unfriendly to the user. I think it's better to keep the 
> behavior of reporting an error if a match* function fails instead of 
> reporting in the patch description that the behavior has changed.
> 

So the following modification is better?

                 case SRP_OPT_CMD_SG_ENTRIES:
-                       if (match_int(args, &token) || token < 1 || 
token > 255) {
+                       ret = match_int(args, &token);
+                       if (ret) {
+                               pr_warn("bad max cmd_sg_entries 
parameter '%s'\n",
+                                       p);
+                               goto out;
+                       }
+                       if (token < 1 || token > 255) {
                                 pr_warn("bad max cmd_sg_entries 
parameter '%s'\n",
                                         p);
+                               ret = -EINVAL;
                                 goto out;
                         }
                         target->cmd_sg_cnt = token;
                         break;


Or the following is better?

                         if (match_int(args, &token) || token < 1 || 
token > 255) {
                                 pr_warn("bad max cmd_sg_entries 
parameter '%s'\n",
                                         p);
+                               ret = -EINVAL;
                                 goto out;
                         }
                         target->cmd_sg_cnt = token;
                         break;


> Thanks,
> 
> Bart.
> 
>
  

On Thu, Dec 01, 2022 at 09:49:51AM +0800, wangyufen wrote:
> 在 2022/12/1 2:00, Bart Van Assche 写道:
> > On 11/29/22 19:31, wangyufen wrote:

...

>                 case SRP_OPT_CMD_SG_ENTRIES:
> -                       if (match_int(args, &token) || token < 1 || token >
> 255) {
> +                       ret = match_int(args, &token);
> +                       if (ret) {
> +                               pr_warn("bad max cmd_sg_entries parameter
> '%s'\n",

It's misleading message here. The problem is that parser failed by some reason.
Otherwise this variant seems good one.

> +                                       p);
> +                               goto out;
> +                       }
> +                       if (token < 1 || token > 255) {
>                                 pr_warn("bad max cmd_sg_entries parameter
> '%s'\n",
>                                         p);
> +                               ret = -EINVAL;
>                                 goto out;
>                         }
>                         target->cmd_sg_cnt = token;
>                         break;

...

> Or the following is better?

Why do you want to shadow actual error code?