Re: [PATCH v2 2/3] loop: replace simple_strtol() with kstrtoint()

[Tao Cui]

Hi Bart,

在 2026/8/31 18:03, Tao Cui 写道:
> From: Tao Cui <cuitao@xxxxxxxxxx>
> 
> simple_strtol() is deprecated and swallows errors.  Use kstrtoint()
> in the max_loop setup code; an invalid option keeps the default and
> now says so.
> 
> Only mark max_loop_specified when the option was parsed successfully.
> With simple_strtol() a garbage string yields max_loop = 0, and
> loop_probe()'s "max_loop_specified && max_loop" check short-circuits
> on 0, so legacy autoloading is not capped.  Keeping max_loop at its
> default while still setting max_loop_specified would instead turn the
> default into a hard upper bound for dynamic device creation.  This
> mirrors what max_loop_param_set_int() already does for the module
> parameter.
> 

Thanks for the review!

After sending v1 I noticed a regression in this patch, so I'll follow
up with a v2 of just this one (1/3 and 3/3 are unchanged and keep your
Reviewed-by).

With simple_strtol() a garbage max_loop= string yields max_loop = 0,
and loop_probe()'s "max_loop_specified && max_loop" check
short-circuits on 0, so legacy autoloading was not capped.  In v1 an
invalid string keeps max_loop at its default value while
max_loop_specified is still set to true, which turns the default into
a hard upper bound for dynamic device creation.  The v2 only marks
max_loop_specified when kstrtoint() succeeds, mirroring
max_loop_param_set_int():

 	if (kstrtoint(str, 0, &max_loop))
+	{
 		pr_warn("loop: invalid max_loop, keeping default\n");
+		return 1;
+	}
 #ifdef CONFIG_BLOCK_LEGACY_AUTOLOAD
 		max_loop_specified = true;
 #endif

This was pointed out by an AI-assisted review of v1, so I've left your
Reviewed-by off this one for now.  Could you take another look when
you have a chance?

Thanks,
Tao

> Signed-off-by: Tao Cui <cuitao@xxxxxxxxxx>
> 
> ---
> Changes in v2:
> - move max_loop_specified = true under the success path, pointed out
>   by an AI-assisted review of v1.
> ---
>  drivers/block/loop.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 68a9cc7aeb13..126f42580632 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -2341,7 +2341,10 @@ module_exit(loop_exit);
>  #ifndef MODULE
>  static int __init max_loop_setup(char *str)
>  {
> -	max_loop = simple_strtol(str, NULL, 0);
> +	if (kstrtoint(str, 0, &max_loop)) {
> +		pr_warn("loop: invalid max_loop, keeping default\n");
> +		return 1;
> +	}
>  #ifdef CONFIG_BLOCK_LEGACY_AUTOLOAD
>  	max_loop_specified = true;
>  #endif