[PATCH] drivers/block/loop: Remove deprecated function, range check for max_loop

[PATCH] drivers/block/loop: Remove deprecated function, range check for max_loop

[Florian Knauf]

This patch removes the deprecated simple_strtol function from the option
parsing logic in the loopback device driver. It also introduces a range
check for the max_loop parameter to ensure that negative and out-of-range
values (that cannot be represented by int max_loop) are ignored.

Signed-off-by: Florian Knauf <florian.knauf@stud.uni-hannover.de>
Signed-off-by: Christian Ewert <christian.ewert@stud.uni-hannover.de>
---
 drivers/block/loop.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 102d79575895..acdd028ed486 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2289,7 +2289,17 @@ module_exit(loop_exit);
 #ifndef MODULE
 static int __init max_loop_setup(char *str)
 {
-	max_loop = simple_strtol(str, NULL, 0);
+	long max_loop_long = 0;
+
+	/*
+	 * Range check for max_loop: negative values and values not
+	 * representable by int are ignored.
+	 */
+	if (kstrtol(str, 0, &max_loop_long) == 0 &&
+			max_loop_long >= 0 &&
+			max_loop_long <= INT_MAX)
+		max_loop = (int) max_loop_long;
+
 	return 1;
 }
 
-- 
2.17.1

Re: [PATCH] drivers/block/loop: Remove deprecated function, range check for max_loop

[Chaitanya Kulkarni]

Thanks for your patch.

On 06/25/2019 04:47 AM, Florian Knauf wrote:
> This patch removes the deprecated simple_strtol function from the option
> parsing logic in the loopback device driver. It also introduces a range
> check for the max_loop parameter to ensure that negative and out-of-range
> values (that cannot be represented by int max_loop) are ignored.
>
> Signed-off-by: Florian Knauf <florian.knauf@stud.uni-hannover.de>
> Signed-off-by: Christian Ewert <christian.ewert@stud.uni-hannover.de>
> ---
>   drivers/block/loop.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 102d79575895..acdd028ed486 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -2289,7 +2289,17 @@ module_exit(loop_exit);
>   #ifndef MODULE
>   static int __init max_loop_setup(char *str)
>   {
> -	max_loop = simple_strtol(str, NULL, 0);
> +	long max_loop_long = 0;
> +
> +	/*
> +	 * Range check for max_loop: negative values and values not
> +	 * representable by int are ignored.
> +	 */
> +	if (kstrtol(str, 0, &max_loop_long) == 0 &&
Is there any specific reason to use kstrtol()?

Since max_loop global variable is defined as an int, can we use kstrtoint()?
> +			max_loop_long >= 0 &&
> +			max_loop_long <= INT_MAX)
> +		max_loop = (int) max_loop_long;
> +
>   	return 1;
>   }
>
>

[PATCH v2] drivers/block/loop: Replace deprecated function in option parsing code

[Florian Knauf]

This patch removes the deprecated simple_strtol function from the option
parsing logic in the loopback device driver. Instead kstrtoint is used to
parse int max_loop, to ensure that input values it cannot represent are
ignored.

Signed-off-by: Florian Knauf <florian.knauf@stud.uni-hannover.de>
Signed-off-by: Christian Ewert <christian.ewert@stud.uni-hannover.de>
---
Thank you for your feedback.

There's no specific reason to use kstrtol, other than the fact that we
weren't yet aware that kstrtoint exists. (We're new at this, I'm afraid.)

We've amended the patch to make use of kstrtoint, which is of course much
more straightforward.

drivers/block/loop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 102d79575895..adfaf4ad37d1 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2289,7 +2289,7 @@ module_exit(loop_exit);
 #ifndef MODULE
 static int __init max_loop_setup(char *str)
 {
-	max_loop = simple_strtol(str, NULL, 0);
+	kstrtoint(str, 0, &max_loop);
 	return 1;
 }
 
-- 
2.17.1

Re: [PATCH v2] drivers/block/loop: Replace deprecated function in option parsing code

[Chaitanya Kulkarni]

I believe you have tested this patch with loop testcases present in the 
:- https://github.com/osandov/blktests/tree/master/tests/loop.

With that, looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>.

On 06/25/2019 10:55 AM, Florian Knauf wrote:
> This patch removes the deprecated simple_strtol function from the option
> parsing logic in the loopback device driver. Instead kstrtoint is used to
> parse int max_loop, to ensure that input values it cannot represent are
> ignored.
>
> Signed-off-by: Florian Knauf <florian.knauf@stud.uni-hannover.de>
> Signed-off-by: Christian Ewert <christian.ewert@stud.uni-hannover.de>
> ---
> Thank you for your feedback.
>
> There's no specific reason to use kstrtol, other than the fact that we
> weren't yet aware that kstrtoint exists. (We're new at this, I'm afraid.)
>
> We've amended the patch to make use of kstrtoint, which is of course much
> more straightforward.
>
> drivers/block/loop.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 102d79575895..adfaf4ad37d1 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -2289,7 +2289,7 @@ module_exit(loop_exit);
>   #ifndef MODULE
>   static int __init max_loop_setup(char *str)
>   {
> -	max_loop = simple_strtol(str, NULL, 0);
> +	kstrtoint(str, 0, &max_loop);
>   	return 1;
>   }
>
>

Re: [PATCH v2] drivers/block/loop: Replace deprecated function in option parsing code

[Florian Knauf]

[-- Attachment #1: Type: text/plain, Size: 2164 bytes --]

I have now, on the latest staging master (test log attached, everything 
green), and also learned a lesson about looking more thoroughly for 
automated test cases. That's a mea culpa, I suppose. :P

Before this I'd only found the Linux Test Project, which (if I'm not 
mistaken) contains tests that use loopback devices but no tests that 
specifically test the loopback driver itself. Given the small scope of 
the change, we then considered it sufficient to test manually that the 
loop device still worked and that the max_loop parameter was handled 
correctly. Of course, the blktests way is better.

Thanks for taking the time to answer and review.

Am 25.06.19 um 21:24 schrieb Chaitanya Kulkarni:
> I believe you have tested this patch with loop testcases present in the
> :- https://github.com/osandov/blktests/tree/master/tests/loop.
> 
> With that, looks good.
> 
> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>.
> 
> On 06/25/2019 10:55 AM, Florian Knauf wrote:
>> This patch removes the deprecated simple_strtol function from the option
>> parsing logic in the loopback device driver. Instead kstrtoint is used to
>> parse int max_loop, to ensure that input values it cannot represent are
>> ignored.
>>
>> Signed-off-by: Florian Knauf <florian.knauf@stud.uni-hannover.de>
>> Signed-off-by: Christian Ewert <christian.ewert@stud.uni-hannover.de>
>> ---
>> Thank you for your feedback.
>>
>> There's no specific reason to use kstrtol, other than the fact that we
>> weren't yet aware that kstrtoint exists. (We're new at this, I'm afraid.)
>>
>> We've amended the patch to make use of kstrtoint, which is of course much
>> more straightforward.
>>
>> drivers/block/loop.c | 2 +-
>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
>> index 102d79575895..adfaf4ad37d1 100644
>> --- a/drivers/block/loop.c
>> +++ b/drivers/block/loop.c
>> @@ -2289,7 +2289,7 @@ module_exit(loop_exit);
>>    #ifndef MODULE
>>    static int __init max_loop_setup(char *str)
>>    {
>> -	max_loop = simple_strtol(str, NULL, 0);
>> +	kstrtoint(str, 0, &max_loop);
>>    	return 1;
>>    }
>>
>>
> 

[-- Attachment #2: check.log --]
[-- Type: text/x-log, Size: 1370 bytes --]

loop/001 (scan loop device partitions)                      
    runtime  0,401s  ...
loop/001 (scan loop device partitions)                       [passed]
    runtime  0,401s  ...  0,269s
loop/002 (try various loop device block sizes)              
    runtime  0,142s  ...
loop/002 (try various loop device block sizes)               [passed]
    runtime  0,142s  ...  0,148s
loop/003 (time opening and closing an unbound loop device)  
    runtime  0,047s  ...
loop/003 (time opening and closing an unbound loop device)   [passed]
    runtime  0,047s  ...  0,052s
loop/004 (combine loop direct I/O mode and a custom block size)
    runtime  0,382s  ...
loop/004 (combine loop direct I/O mode and a custom block size) [passed]
    runtime  0,382s  ...  0,383s
loop/005 (call LOOP_GET_STATUS{,64} with a NULL arg)        
    runtime  0,024s  ...
loop/005 (call LOOP_GET_STATUS{,64} with a NULL arg)         [passed]
    runtime  0,024s  ...  0,025s
loop/006 (change loop backing file while creating/removing another loop device)
    runtime  31,071s  ...
loop/006 (change loop backing file while creating/removing another loop device) [passed]
    runtime  31,071s  ...  31,050s
loop/007 (update loop device capacity with filesystem)      
    runtime  0,417s  ...
loop/007 (update loop device capacity with filesystem)       [passed]
    runtime  0,417s  ...  0,351s

Re: [PATCH v2] drivers/block/loop: Replace deprecated function in option parsing code

[Chaitanya Kulkarni]

You are welcome.
> On Jun 29, 2019, at 12:46 PM, Florian Knauf <florian.knauf@stud.uni-hannover.de> wrote:
> 
> I have now, on the latest staging master (test log attached, everything green), and also learned a lesson about looking more thoroughly for automated test cases. That's a mea culpa, I suppose. :P
> 
> Before this I'd only found the Linux Test Project, which (if I'm not mistaken) contains tests that use loopback devices but no tests that specifically test the loopback driver itself. Given the small scope of the change, we then considered it sufficient to test manually that the loop device still worked and that the max_loop parameter was handled correctly. Of course, the blktests way is better.
> 
> Thanks for taking the time to answer and review.
> 
>> Am 25.06.19 um 21:24 schrieb Chaitanya Kulkarni:
>> I believe you have tested this patch with loop testcases present in the
>> :- https://github.com/osandov/blktests/tree/master/tests/loop.
>> With that, looks good.
>> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>.
>>> On 06/25/2019 10:55 AM, Florian Knauf wrote:
>>> This patch removes the deprecated simple_strtol function from the option
>>> parsing logic in the loopback device driver. Instead kstrtoint is used to
>>> parse int max_loop, to ensure that input values it cannot represent are
>>> ignored.
>>> 
>>> Signed-off-by: Florian Knauf <florian.knauf@stud.uni-hannover.de>
>>> Signed-off-by: Christian Ewert <christian.ewert@stud.uni-hannover.de>
>>> ---
>>> Thank you for your feedback.
>>> 
>>> There's no specific reason to use kstrtol, other than the fact that we
>>> weren't yet aware that kstrtoint exists. (We're new at this, I'm afraid.)
>>> 
>>> We've amended the patch to make use of kstrtoint, which is of course much
>>> more straightforward.
>>> 
>>> drivers/block/loop.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
>>> index 102d79575895..adfaf4ad37d1 100644
>>> --- a/drivers/block/loop.c
>>> +++ b/drivers/block/loop.c
>>> @@ -2289,7 +2289,7 @@ module_exit(loop_exit);
>>>   #ifndef MODULE
>>>   static int __init max_loop_setup(char *str)
>>>   {
>>> -    max_loop = simple_strtol(str, NULL, 0);
>>> +    kstrtoint(str, 0, &max_loop);
>>>       return 1;
>>>   }
>>> 
>>> 
> <check.log>