[PATCH RESEND 0/2] loop: fix regression from max_loop default value change

[PATCH RESEND 0/2] loop: fix regression from max_loop default value change

[Mauricio Faria de Oliveira]

Apparently, there's an unintended consequence of the improvement for max_loop=0
in commit 85c50197716c ("loop: Fix the max_loop commandline argument treatment
when it is set to 0") which might break programs that handle /dev/loop devices.

The (deprecated) autoloading path fails (ENXIO) if the requested minor number
is greater than or equal to the (new) default (CONFIG_BLK_DEV_LOOP_MIN_COUNT),
when [loop.]max_loop is not specified.  This behavior used to work previously.

Patch 1/2 just notes the loop driver's autoloading path is deprecated/legacy.
Patch 2/2 detects whether or not max_loop is set to restore default behavior
as before the regression (and keeps the improvement done by the commit above).

Tested on v6.5-rc2.

Thanks,
Mauricio

Mauricio Faria de Oliveira (2):
  loop: deprecate autoloading callback loop_probe()
  loop: do not enforce max_loop hard limit by (new) default

 drivers/block/loop.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

-- 
2.39.2

[PATCH RESEND 1/2] loop: deprecate autoloading callback loop_probe()

[Mauricio Faria de Oliveira]

The 'probe' callback in __register_blkdev() is only used
under the CONFIG_BLOCK_LEGACY_AUTOLOAD deprecation guard.

The loop_probe() function is only used for that callback,
so guard it too, accordingly.

See commit fbdee71bb5d8 ("block: deprecate autoloading based on dev_t").

Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
---
 drivers/block/loop.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 37511d2b2caf..7268ff71c92c 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2093,6 +2093,7 @@ static void loop_remove(struct loop_device *lo)
 	put_disk(lo->lo_disk);
 }
 
+#ifdef CONFIG_BLOCK_LEGACY_AUTOLOAD
 static void loop_probe(dev_t dev)
 {
 	int idx = MINOR(dev) >> part_shift;
@@ -2101,6 +2102,7 @@ static void loop_probe(dev_t dev)
 		return;
 	loop_add(idx);
 }
+#endif
 
 static int loop_control_remove(int idx)
 {
@@ -2235,8 +2237,11 @@ static int __init loop_init(void)
 	if (err < 0)
 		goto err_out;
 
-
+#ifdef CONFIG_BLOCK_LEGACY_AUTOLOAD
 	if (__register_blkdev(LOOP_MAJOR, "loop", loop_probe)) {
+#else
+	if (register_blkdev(LOOP_MAJOR, "loop")) {
+#endif
 		err = -EIO;
 		goto misc_out;
 	}
-- 
2.39.2

[PATCH RESEND 2/2] loop: do not enforce max_loop hard limit by (new) default

[Mauricio Faria de Oliveira]

Problem:

The max_loop parameter is used for 2 different purposes:
1) initial number of loop devices to pre-create on init
2) maximum number of loop devices to add on access/open()

Historically, its default value (zero) caused 1) to create
non-zero number of devices (CONFIG_BLK_DEV_LOOP_MIN_COUNT),
and _no_ hard limit on 2) to add devices with autoloading.

However, the default value changed in commit 85c50197716c
("loop: Fix the max_loop commandline argument treatment
when it is set to 0") to CONFIG_BLK_DEV_LOOP_MIN_COUNT,
so that max_loop=0 does not pre-create devices at all.

That does improve 1), but unfortunately _breaks_ 2), as
default behavior *changed* from no-limit to hard-limit.

Example:

For example, this userspace code broke for N >= CONFIG,
if the user relied on the default value 0 for max_loop:

    mknod("/dev/loopN")
    open("/dev/loopN")  // now fails with ENXIO

Though affected users may "fix" it with (loop.)max_loop=0,
this means to require a kernel parameter change on stable
kernel update (that commit Fixes: an old commit in stable).

Solution:

The original semantics for the default value in 2) can be
applied if the parameter is not set (ie, default behavior).

This still keeps the intended function in 1) and 2) if set,
and that commit's intended improvement in 1) if max_loop=0.

Before 85c50197716c:
  - default:     1) CONFIG devices   2) no limit
  - max_loop=0:  1) CONFIG devices   2) no limit
  - max_loop=X:  1) X devices        2) X limit

After 85c50197716c:
  - default:     1) CONFIG devices   2) CONFIG limit (*)
  - max_loop=0:  1) 0 devices (*)    2) no limit
  - max_loop=X:  1) X devices        2) X limit

This commit:
  - default:     1) CONFIG devices   2) no limit (*)
  - max_loop=0:  1) 0 devices        2) no limit
  - max_loop=X:  1) X devices        2) X limit

Future:

The issue/regression from that commit only affects code
under the CONFIG_BLOCK_LEGACY_AUTOLOAD deprecation guard,
thus the fix too is contained under it.

Once that deprecated functionality/code is removed, the
purpose 2) of max_loop (hard limit) is no longer in use,
so the module parameter description can be changed then.

Tests:

Linux 6.5-rc2
CONFIG_BLK_DEV_LOOP_MIN_COUNT=8
CONFIG_BLOCK_LEGACY_AUTOLOAD=y

- default (original)

	# ls -1 /dev/loop*
	/dev/loop-control
	/dev/loop0
	...
	/dev/loop7

	# ./test-loop
	open: /dev/loop8: No such device or address

- default (patched)

	# ls -1 /dev/loop*
	/dev/loop-control
	/dev/loop0
	...
	/dev/loop7

	# ./test-loop
	#

- max_loop=0 (original & patched):

	# ls -1 /dev/loop*
	/dev/loop-control

	# ./test-loop
	#

- max_loop=8 (original & patched):

	# ls -1 /dev/loop*
	/dev/loop-control
	/dev/loop0
	...
	/dev/loop7

	# ./test-loop
	open: /dev/loop8: No such device or address

- max_loop=0 (patched; CONFIG_BLOCK_LEGACY_AUTOLOAD is not set)

	# ls -1 /dev/loop*
	/dev/loop-control

	# ./test-loop
	open: /dev/loop8: No such device or address

Fixes: 85c50197716c ("loop: Fix the max_loop commandline argument treatment when it is set to 0")
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
---
 drivers/block/loop.c | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 7268ff71c92c..310e21889086 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1775,14 +1775,43 @@ static const struct block_device_operations lo_fops = {
 /*
  * If max_loop is specified, create that many devices upfront.
  * This also becomes a hard limit. If max_loop is not specified,
+ * the default isn't a hard limit (as before commit 85c50197716c
+ * changed the default value from 0 for max_loop=0 reasons), just
  * create CONFIG_BLK_DEV_LOOP_MIN_COUNT loop devices at module
  * init time. Loop devices can be requested on-demand with the
  * /dev/loop-control interface, or be instantiated by accessing
  * a 'dead' device node.
  */
 static int max_loop = CONFIG_BLK_DEV_LOOP_MIN_COUNT;
-module_param(max_loop, int, 0444);
+
+#ifdef CONFIG_BLOCK_LEGACY_AUTOLOAD
+static bool max_loop_specified;
+
+static int max_loop_param_set_int(const char *val,
+				  const struct kernel_param *kp)
+{
+	int ret;
+
+	ret = param_set_int(val, kp);
+	if (ret < 0)
+		return ret;
+
+	max_loop_specified = true;
+	return 0;
+}
+
+static const struct kernel_param_ops max_loop_param_ops = {
+	.set = max_loop_param_set_int,
+	.get = param_get_int,
+};
+
+module_param_cb(max_loop, &max_loop_param_ops, &max_loop, 0444);
 MODULE_PARM_DESC(max_loop, "Maximum number of loop devices");
+#else
+module_param(max_loop, int, 0444);
+MODULE_PARM_DESC(max_loop, "Initial number of loop devices");
+#endif
+
 module_param(max_part, int, 0444);
 MODULE_PARM_DESC(max_part, "Maximum number of partitions per loop device");
 
@@ -2098,7 +2127,7 @@ static void loop_probe(dev_t dev)
 {
 	int idx = MINOR(dev) >> part_shift;
 
-	if (max_loop && idx >= max_loop)
+	if (max_loop_specified && max_loop && idx >= max_loop)
 		return;
 	loop_add(idx);
 }
@@ -2286,6 +2315,9 @@ module_exit(loop_exit);
 static int __init max_loop_setup(char *str)
 {
 	max_loop = simple_strtol(str, NULL, 0);
+#ifdef CONFIG_BLOCK_LEGACY_AUTOLOAD
+	max_loop_specified = true;
+#endif
 	return 1;
 }
 
-- 
2.39.2

Re: [PATCH RESEND 1/2] loop: deprecate autoloading callback loop_probe()

[Christoph Hellwig]

On Mon, Jul 17, 2023 at 04:16:27PM -0300, Mauricio Faria de Oliveira wrote:
> The 'probe' callback in __register_blkdev() is only used
> under the CONFIG_BLOCK_LEGACY_AUTOLOAD deprecation guard.
> 
> The loop_probe() function is only used for that callback,
> so guard it too, accordingly.
> 
> See commit fbdee71bb5d8 ("block: deprecate autoloading based on dev_t").
> 
> Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
> ---
>  drivers/block/loop.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 37511d2b2caf..7268ff71c92c 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -2093,6 +2093,7 @@ static void loop_remove(struct loop_device *lo)
>  	put_disk(lo->lo_disk);
>  }
>  
> +#ifdef CONFIG_BLOCK_LEGACY_AUTOLOAD
>  static void loop_probe(dev_t dev)
>  {
>  	int idx = MINOR(dev) >> part_shift;
> @@ -2101,6 +2102,7 @@ static void loop_probe(dev_t dev)
>  		return;
>  	loop_add(idx);
>  }
> +#endif

Turn this into..

#else
#define loop_probe	NULL
#endif /* !CONFIG_BLOCK_LEGACY_AUTOLOAD */

and you can skip the pretty ugly second hunk.

Re: [PATCH RESEND 2/2] loop: do not enforce max_loop hard limit by (new) default

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH RESEND 1/2] loop: deprecate autoloading callback loop_probe()

[Mauricio Faria de Oliveira]

On Thu, Jul 20, 2023 at 5:30 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Jul 17, 2023 at 04:16:27PM -0300, Mauricio Faria de Oliveira wrote:
> > The 'probe' callback in __register_blkdev() is only used
> > under the CONFIG_BLOCK_LEGACY_AUTOLOAD deprecation guard.
> >
> > The loop_probe() function is only used for that callback,
> > so guard it too, accordingly.
> >
> > See commit fbdee71bb5d8 ("block: deprecate autoloading based on dev_t").
> >
> > Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
> > ---
> >  drivers/block/loop.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index 37511d2b2caf..7268ff71c92c 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -2093,6 +2093,7 @@ static void loop_remove(struct loop_device *lo)
> >       put_disk(lo->lo_disk);
> >  }
> >
> > +#ifdef CONFIG_BLOCK_LEGACY_AUTOLOAD
> >  static void loop_probe(dev_t dev)
> >  {
> >       int idx = MINOR(dev) >> part_shift;
> > @@ -2101,6 +2102,7 @@ static void loop_probe(dev_t dev)
> >               return;
> >       loop_add(idx);
> >  }
> > +#endif
>
> Turn this into..
>
> #else
> #define loop_probe      NULL
> #endif /* !CONFIG_BLOCK_LEGACY_AUTOLOAD */
>
> and you can skip the pretty ugly second hunk.
>

Thanks for reviewing and the suggestion; just sent v2.


--
Mauricio Faria de Oliveira