[patch] [bugfix] loop.c

[patch] [bugfix] loop.c

[Tomas M]

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

I posted this yesterday but it seems people didn't understand the real 
goal of my patch. So I will explain once more again:

This is a bugfix for loop.c block driver, as it currently allocates more 
memory then it needs, without any further use.

If 'max_loop=255' parameter is given, the loop.c driver allocates this 
amount of memory:

   kmalloc(max_loop * sizeof(struct loop_device))

But in this case, (max_loop * sizeof) is greater than 65536, and thus 
kmalloc must allocate the next bigger size (which is 128KB of RAM).

Unfortunately the loop.c driver doesn't allow users to use bigger 
max_loop then 255 (for reasons which are completely obsolete) so the 
rest of memory is *unused*.

This patch doesn't fix unused memory in the case of max_loop=255, but 
rather it allows user to specify bigger max_loop (up to 455 on 386), so 
  the user can fully use all the memory, which would be allocated anyway.

Thank you for your consideration

Tomas M
slax.org

[-- Attachment #2: loop.c.diff --]
[-- Type: text/x-patch, Size: 1048 bytes --]

--- linux/drivers/block/loop.c_old	2007-03-23 14:27:04.000000000 +0000
+++ linux/drivers/block/loop.c	2007-03-23 14:31:18.098458759 +0000
@@ -1358,7 +1358,7 @@
  * And now the modules code and kernel interface.
  */
 module_param(max_loop, int, 0);
-MODULE_PARM_DESC(max_loop, "Maximum number of loop devices (1-256)");
+MODULE_PARM_DESC(max_loop, "Maximum number of loop devices (455/355 on x86/x86_64)");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);
 
@@ -1402,9 +1402,9 @@
 {
 	int	i;
 
-	if (max_loop < 1 || max_loop > 256) {
-		printk(KERN_WARNING "loop: invalid max_loop (must be between"
-				    " 1 and 256), using default (8)\n");
+	if (max_loop < 1) {
+		printk(KERN_WARNING "loop: invalid max_loop (must be at least 1"
+				    ", using default (8)\n");
 		max_loop = 8;
 	}
 
@@ -1465,7 +1465,7 @@
 	kfree(loop_dev);
 out_mem1:
 	unregister_blkdev(LOOP_MAJOR, "loop");
-	printk(KERN_ERR "loop: ran out of memory\n");
+	printk(KERN_ERR "loop: ran out of memory for max_loop=%d\n", max_loop);
 	return -ENOMEM;
 }
 

Re: [patch] [bugfix] loop.c

[Jiri Kosina]

On Fri, 23 Mar 2007, Tomas M wrote:

> This is a bugfix for loop.c block driver, as it currently allocates more 
> memory then it needs, without any further use.
> If 'max_loop=255' parameter is given, the loop.c driver allocates this 
> amount of memory:
>   kmalloc(max_loop * sizeof(struct loop_device))
> But in this case, (max_loop * sizeof) is greater than 65536, and thus kmalloc
> must allocate the next bigger size (which is 128KB of RAM).

Which is very unlikely to work after some time of uptime, as the memory 
will be too fragmented to successfully kmalloc(128k). I guess that the 
code should be rather rewritten not to allocate such large contignuous 
chunks of memory through kmalloc().

-- 
Jiri Kosina

Re: [patch] [bugfix] loop.c

[Eric Dumazet]

On Fri, 23 Mar 2007 15:04:54 +0100
Tomas M <tomas@slax.org> wrote:

> I posted this yesterday but it seems people didn't understand the real 
> goal of my patch. So I will explain once more again:
> 
> This is a bugfix for loop.c block driver, as it currently allocates more 
> memory then it needs, without any further use.

Well... changing the Changelog wont help I'm afraid.

I cooked the following patch (untested), feel free to test it.

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6b5b642..3f4b68c 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -65,6 +65,7 @@ #include <linux/init.h>
 #include <linux/smp_lock.h>
 #include <linux/swap.h>
 #include <linux/slab.h>
+#include <linux/vmalloc.h>
 #include <linux/loop.h>
 #include <linux/compat.h>
 #include <linux/suspend.h>
@@ -78,8 +79,8 @@ #include <linux/kthread.h>
 #include <asm/uaccess.h>
 
 static int max_loop = 8;
-static struct loop_device *loop_dev;
-static struct gendisk **disks;
+static struct loop_device **loop_dev;
+static int loop_dev_vmalloced;
 
 /*
  * Transfer functions
@@ -183,7 +184,7 @@ figure_loop_size(struct loop_device *lo)
 	if (unlikely((loff_t)x != size))
 		return -EFBIG;
 
-	set_capacity(disks[lo->lo_number], x);
+	set_capacity(lo->lo_disk, x);
 	return 0;					
 }
 
@@ -812,7 +813,7 @@ static int loop_set_fd(struct loop_devic
 	lo->lo_queue->queuedata = lo;
 	lo->lo_queue->unplug_fn = loop_unplug;
 
-	set_capacity(disks[lo->lo_number], size);
+	set_capacity(lo->lo_disk, size);
 	bd_set_size(bdev, size << 9);
 
 	set_blocksize(bdev, lo_blocksize);
@@ -832,7 +833,7 @@ out_clr:
 	lo->lo_device = NULL;
 	lo->lo_backing_file = NULL;
 	lo->lo_flags = 0;
-	set_capacity(disks[lo->lo_number], 0);
+	set_capacity(lo->lo_disk, 0);
 	invalidate_bdev(bdev, 0);
 	bd_set_size(bdev, 0);
 	mapping_set_gfp_mask(mapping, lo->old_gfp_mask);
@@ -918,7 +919,7 @@ static int loop_clr_fd(struct loop_devic
 	memset(lo->lo_crypt_name, 0, LO_NAME_SIZE);
 	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
 	invalidate_bdev(bdev, 0);
-	set_capacity(disks[lo->lo_number], 0);
+	set_capacity(lo->lo_disk, 0);
 	bd_set_size(bdev, 0);
 	mapping_set_gfp_mask(filp->f_mapping, gfp);
 	lo->lo_state = Lo_unbound;
@@ -1358,7 +1359,7 @@ #endif
  * And now the modules code and kernel interface.
  */
 module_param(max_loop, int, 0);
-MODULE_PARM_DESC(max_loop, "Maximum number of loop devices (1-256)");
+MODULE_PARM_DESC(max_loop, "Maximum number of loop devices (1-16384)");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);
 
@@ -1377,13 +1378,15 @@ int loop_unregister_transfer(int number)
 	unsigned int n = number;
 	struct loop_device *lo;
 	struct loop_func_table *xfer;
+	int i;
 
 	if (n == 0 || n >= MAX_LO_CRYPT || (xfer = xfer_funcs[n]) == NULL)
 		return -EINVAL;
 
 	xfer_funcs[n] = NULL;
 
-	for (lo = &loop_dev[0]; lo < &loop_dev[max_loop]; lo++) {
+	for (i = 0; i < max_loop; i++) {
+		lo = loop_dev[i];
 		mutex_lock(&lo->lo_ctl_mutex);
 
 		if (lo->lo_encryption == xfer)
@@ -1400,70 +1403,74 @@ EXPORT_SYMBOL(loop_unregister_transfer);
 
 static int __init loop_init(void)
 {
-	int	i;
+	struct gendisk *disk;
+	struct loop_device *lo;
+	int	i, nba = 0, nbl = 0;
 
-	if (max_loop < 1 || max_loop > 256) {
-		printk(KERN_WARNING "loop: invalid max_loop (must be between"
-				    " 1 and 256), using default (8)\n");
+	if (max_loop < 1) {
+		printk(KERN_WARNING "loop: invalid max_loop (must be > 1)"
+				    ", using default (8)\n");
 		max_loop = 8;
 	}
 
 	if (register_blkdev(LOOP_MAJOR, "loop"))
 		return -EIO;
 
-	loop_dev = kmalloc(max_loop * sizeof(struct loop_device), GFP_KERNEL);
-	if (!loop_dev)
-		goto out_mem1;
-	memset(loop_dev, 0, max_loop * sizeof(struct loop_device));
-
-	disks = kmalloc(max_loop * sizeof(struct gendisk *), GFP_KERNEL);
-	if (!disks)
-		goto out_mem2;
-
-	for (i = 0; i < max_loop; i++) {
-		disks[i] = alloc_disk(1);
-		if (!disks[i])
-			goto out_mem3;
+	loop_dev = kmalloc(max_loop * sizeof(struct loop_device *), GFP_KERNEL);
+	if (!loop_dev) {
+		loop_dev = vmalloc(max_loop * sizeof(struct loop_device *));
+		if (!loop_dev)
+			goto out_mem;
+		loop_dev_vmalloced = 1;
 	}
 
-	for (i = 0; i < max_loop; i++) {
-		struct loop_device *lo = &loop_dev[i];
-		struct gendisk *disk = disks[i];
+	while (nbl < max_loop) {
+		lo = kzalloc(sizeof(struct loop_device), GFP_KERNEL);
+		if (!lo)
+			goto out_mem;
+		disk = alloc_disk(1);
+		lo->lo_disk = disk;
+		loop_dev[nbl++] = lo;
+		if (!disk)
+			goto out_mem;
+	}
 
-		memset(lo, 0, sizeof(*lo));
+	for (; nba < max_loop; nba++) {
+		lo = loop_dev[nba];
 		lo->lo_queue = blk_alloc_queue(GFP_KERNEL);
 		if (!lo->lo_queue)
-			goto out_mem4;
+			goto out_mem;
+		disk = lo->lo_disk;
 		mutex_init(&lo->lo_ctl_mutex);
-		lo->lo_number = i;
+		lo->lo_number = nba;
 		lo->lo_thread = NULL;
 		init_waitqueue_head(&lo->lo_event);
 		spin_lock_init(&lo->lo_lock);
 		disk->major = LOOP_MAJOR;
-		disk->first_minor = i;
+		disk->first_minor = nba;
 		disk->fops = &lo_fops;
-		sprintf(disk->disk_name, "loop%d", i);
+		sprintf(disk->disk_name, "loop%d", nba);
 		disk->private_data = lo;
 		disk->queue = lo->lo_queue;
 	}
 
 	/* We cannot fail after we call this, so another loop!*/
 	for (i = 0; i < max_loop; i++)
-		add_disk(disks[i]);
+		add_disk(loop_dev[i]->lo_disk);
 	printk(KERN_INFO "loop: loaded (max %d devices)\n", max_loop);
 	return 0;
 
-out_mem4:
-	while (i--)
-		blk_cleanup_queue(loop_dev[i].lo_queue);
-	i = max_loop;
-out_mem3:
-	while (i--)
-		put_disk(disks[i]);
-	kfree(disks);
-out_mem2:
-	kfree(loop_dev);
-out_mem1:
+out_mem:
+	while (nba--)
+		blk_cleanup_queue(loop_dev[nba]->lo_queue);
+	while (nbl--) {
+		put_disk(loop_dev[nbl]->lo_disk);
+		kfree(loop_dev[nbl]);
+		}
+	if (loop_dev_vmalloced)
+		vfree(loop_dev);
+	else
+		kfree(loop_dev);
 	unregister_blkdev(LOOP_MAJOR, "loop");
 	printk(KERN_ERR "loop: ran out of memory\n");
 	return -ENOMEM;
@@ -1472,17 +1479,22 @@ out_mem1:
 static void loop_exit(void)
 {
 	int i;
+	struct loop_device *lo;
 
 	for (i = 0; i < max_loop; i++) {
-		del_gendisk(disks[i]);
-		blk_cleanup_queue(loop_dev[i].lo_queue);
-		put_disk(disks[i]);
+		lo = loop_dev[i];
+		del_gendisk(lo->lo_disk);
+		blk_cleanup_queue(lo->lo_queue);
+		put_disk(lo->lo_disk);
+		kfree(lo);
 	}
 	if (unregister_blkdev(LOOP_MAJOR, "loop"))
 		printk(KERN_WARNING "loop: cannot unregister blkdev\n");
 
-	kfree(disks);
-	kfree(loop_dev);
+	if (loop_dev_vmalloced)
+		vfree(loop_dev);
+	else
+		kfree(loop_dev);
 }
 
 module_init(loop_init);
diff --git a/include/linux/loop.h b/include/linux/loop.h
index 191a595..fef7c5e 100644
--- a/include/linux/loop.h
+++ b/include/linux/loop.h
@@ -29,6 +29,7 @@ enum {
 struct loop_func_table;
 
 struct loop_device {
+	struct gendisk *lo_disk;
 	int		lo_number;
 	int		lo_refcnt;
 	loff_t		lo_offset;

Re: [patch] [bugfix] loop.c

[Jiri Kosina]

On Fri, 23 Mar 2007, Eric Dumazet wrote:

> -	if (max_loop < 1 || max_loop > 256) {
> -		printk(KERN_WARNING "loop: invalid max_loop (must be between"
> -				    " 1 and 256), using default (8)\n");
> +	if (max_loop < 1) {
> +		printk(KERN_WARNING "loop: invalid max_loop (must be > 1)"
> +				    ", using default (8)\n");
>  		max_loop = 8;
>  	}
[...]
> +	loop_dev = kmalloc(max_loop * sizeof(struct loop_device *), GFP_KERNEL);
> +	if (!loop_dev) {
> +		loop_dev = vmalloc(max_loop * sizeof(struct loop_device *));
> +		if (!loop_dev)
> +			goto out_mem;
> +		loop_dev_vmalloced = 1;
>  	}

Why did you remove the upper bound check for max_loop value? Now you 
effectively allow to max_loop * sizeof(struct loop_device *) to overflow, 
when passed value of max_loop which is large enough. Or am I just blind?

The "while (nbl < max_loop)" which immediately follows is then going to 
corrupt memory, right?

-- 
Jiri Kosina

Re: [patch] [bugfix] loop.c

[William Lee Irwin III]

On Fri, 23 Mar 2007 15:04:54 +0100 Tomas M <tomas@slax.org> wrote:
>> I posted this yesterday but it seems people didn't understand the real 
>> goal of my patch. So I will explain once more again:
>> This is a bugfix for loop.c block driver, as it currently allocates more 
>> memory then it needs, without any further use.

On Fri, Mar 23, 2007 at 03:19:56PM +0100, Eric Dumazet wrote:
> Well... changing the Changelog wont help I'm afraid.
> I cooked the following patch (untested), feel free to test it.

The array of preallocated garbage is to be killed by instantiating the
driver-private state at ->open() or at the time of file attachment or
whatever (I myself am not entirely sure of the right way to go about it).
I think someone's working on writing a patch of that form. I'm not sure
what they say is to be done about the gendisk array but I think there
are other ways to find the things by major and minor numbers.

IOW teaching loop.c to allocate more is not the way to go; one should
rather teach it to avoid doing all those allocations up-front.


-- wli

Re: [patch] [bugfix] loop.c

[Al Viro]

On Fri, Mar 23, 2007 at 03:19:56PM +0100, Eric Dumazet wrote:
> On Fri, 23 Mar 2007 15:04:54 +0100
> Tomas M <tomas@slax.org> wrote:
> 
> > I posted this yesterday but it seems people didn't understand the real 
> > goal of my patch. So I will explain once more again:
> > 
> > This is a bugfix for loop.c block driver, as it currently allocates more 
> > memory then it needs, without any further use.
> 
> Well... changing the Changelog wont help I'm afraid.
> 
> I cooked the following patch (untested), feel free to test it.

Please, get the cleanup into saner shape.  This is too ugly.

Re: [patch] [bugfix] loop.c

[Christoph Hellwig]

On Fri, Mar 23, 2007 at 03:04:54PM +0100, Tomas M wrote:
> I posted this yesterday but it seems people didn't understand the real 
> goal of my patch. So I will explain once more again:
> 
> This is a bugfix for loop.c block driver, as it currently allocates more 
> memory then it needs, without any further use.
> 
> If 'max_loop=255' parameter is given, the loop.c driver allocates this 
> amount of memory:
> 
>   kmalloc(max_loop * sizeof(struct loop_device))
> 
> But in this case, (max_loop * sizeof) is greater than 65536, and thus 
> kmalloc must allocate the next bigger size (which is 128KB of RAM).
> 
> Unfortunately the loop.c driver doesn't allow users to use bigger 
> max_loop then 255 (for reasons which are completely obsolete) so the 
> rest of memory is *unused*.
> 
> This patch doesn't fix unused memory in the case of max_loop=255, but 
> rather it allows user to specify bigger max_loop (up to 455 on 386), so 
>  the user can fully use all the memory, which would be allocated anyway.
> 
> Thank you for your consideration

The right thing to start with is to split the allocation up, and allocate
each loop device by itself, like in the untested patch below:

After that you're not wasting memory for any off number of loop devices
and can create as many as you have device numbers available:


Index: linux-2.6/drivers/block/loop.c
===================================================================
--- linux-2.6.orig/drivers/block/loop.c	2007-03-23 15:21:57.000000000 +0100
+++ linux-2.6/drivers/block/loop.c	2007-03-23 15:39:44.000000000 +0100
@@ -78,8 +78,7 @@
 #include <asm/uaccess.h>
 
 static int max_loop = 8;
-static struct loop_device *loop_dev;
-static struct gendisk **disks;
+static LIST_HEAD(loop_devices);
 
 /*
  * Transfer functions
@@ -183,7 +182,7 @@ figure_loop_size(struct loop_device *lo)
 	if (unlikely((loff_t)x != size))
 		return -EFBIG;
 
-	set_capacity(disks[lo->lo_number], x);
+	set_capacity(lo->lo_disk, x);
 	return 0;					
 }
 
@@ -812,7 +811,7 @@ static int loop_set_fd(struct loop_devic
 	lo->lo_queue->queuedata = lo;
 	lo->lo_queue->unplug_fn = loop_unplug;
 
-	set_capacity(disks[lo->lo_number], size);
+	set_capacity(lo->lo_disk, size);
 	bd_set_size(bdev, size << 9);
 
 	set_blocksize(bdev, lo_blocksize);
@@ -832,7 +831,7 @@ out_clr:
 	lo->lo_device = NULL;
 	lo->lo_backing_file = NULL;
 	lo->lo_flags = 0;
-	set_capacity(disks[lo->lo_number], 0);
+	set_capacity(lo->lo_disk, 0);
 	invalidate_bdev(bdev, 0);
 	bd_set_size(bdev, 0);
 	mapping_set_gfp_mask(mapping, lo->old_gfp_mask);
@@ -918,7 +917,7 @@ static int loop_clr_fd(struct loop_devic
 	memset(lo->lo_crypt_name, 0, LO_NAME_SIZE);
 	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
 	invalidate_bdev(bdev, 0);
-	set_capacity(disks[lo->lo_number], 0);
+	set_capacity(lo->lo_disk, 0);
 	bd_set_size(bdev, 0);
 	mapping_set_gfp_mask(filp->f_mapping, gfp);
 	lo->lo_state = Lo_unbound;
@@ -1383,7 +1382,7 @@ int loop_unregister_transfer(int number)
 
 	xfer_funcs[n] = NULL;
 
-	for (lo = &loop_dev[0]; lo < &loop_dev[max_loop]; lo++) {
+	list_for_each_entry(lo, &loop_devices, lo_list) {
 		mutex_lock(&lo->lo_ctl_mutex);
 
 		if (lo->lo_encryption == xfer)
@@ -1398,9 +1397,56 @@ int loop_unregister_transfer(int number)
 EXPORT_SYMBOL(loop_register_transfer);
 EXPORT_SYMBOL(loop_unregister_transfer);
 
+static int __init loop_init_one(int i)
+{
+	struct loop_device *lo;
+
+	lo = kzalloc(sizeof(*lo), GFP_KERNEL);
+	if (!lo)
+		goto out;
+
+	lo->lo_queue = blk_alloc_queue(GFP_KERNEL);
+	if (!lo->lo_queue)
+		goto out_free_dev;
+	lo->lo_disk = alloc_disk(1);
+	if (!lo->lo_disk)
+		goto out_free_queue;
+	mutex_init(&lo->lo_ctl_mutex);
+	lo->lo_number = i;
+	lo->lo_thread = NULL;
+	init_waitqueue_head(&lo->lo_event);
+	spin_lock_init(&lo->lo_lock);
+	list_add(&lo->lo_list, &loop_devices);
+	lo->lo_disk->major = LOOP_MAJOR;
+	lo->lo_disk->first_minor = i;
+	lo->lo_disk->fops = &lo_fops;
+	sprintf(lo->lo_disk->disk_name, "loop%d", i);
+	lo->lo_disk->private_data = lo;
+	lo->lo_disk->queue = lo->lo_queue;
+	add_disk(lo->lo_disk);
+	return 0;
+
+ out_free_queue:
+	blk_cleanup_queue(lo->lo_queue);
+ out_free_dev:
+	kfree(lo);
+ out:
+	return -ENOMEM;
+}
+
+static void loop_del_one(struct loop_device *lo)
+{
+	del_gendisk(lo->lo_disk);
+	blk_cleanup_queue(lo->lo_queue);
+	put_disk(lo->lo_disk);
+	list_del(&lo->lo_list);
+	kfree(lo);
+}
+
 static int __init loop_init(void)
 {
-	int	i;
+	struct loop_device *lo, *next;
+	int err, i;
 
 	if (max_loop < 1 || max_loop > 256) {
 		printk(KERN_WARNING "loop: invalid max_loop (must be between"
@@ -1411,78 +1457,32 @@ static int __init loop_init(void)
 	if (register_blkdev(LOOP_MAJOR, "loop"))
 		return -EIO;
 
-	loop_dev = kmalloc(max_loop * sizeof(struct loop_device), GFP_KERNEL);
-	if (!loop_dev)
-		goto out_mem1;
-	memset(loop_dev, 0, max_loop * sizeof(struct loop_device));
-
-	disks = kmalloc(max_loop * sizeof(struct gendisk *), GFP_KERNEL);
-	if (!disks)
-		goto out_mem2;
-
 	for (i = 0; i < max_loop; i++) {
-		disks[i] = alloc_disk(1);
-		if (!disks[i])
-			goto out_mem3;
+		err = loop_init_one(i);
+		if (err)
+			goto out_free_devs;
 	}
 
-	for (i = 0; i < max_loop; i++) {
-		struct loop_device *lo = &loop_dev[i];
-		struct gendisk *disk = disks[i];
-
-		memset(lo, 0, sizeof(*lo));
-		lo->lo_queue = blk_alloc_queue(GFP_KERNEL);
-		if (!lo->lo_queue)
-			goto out_mem4;
-		mutex_init(&lo->lo_ctl_mutex);
-		lo->lo_number = i;
-		lo->lo_thread = NULL;
-		init_waitqueue_head(&lo->lo_event);
-		spin_lock_init(&lo->lo_lock);
-		disk->major = LOOP_MAJOR;
-		disk->first_minor = i;
-		disk->fops = &lo_fops;
-		sprintf(disk->disk_name, "loop%d", i);
-		disk->private_data = lo;
-		disk->queue = lo->lo_queue;
-	}
-
-	/* We cannot fail after we call this, so another loop!*/
-	for (i = 0; i < max_loop; i++)
-		add_disk(disks[i]);
 	printk(KERN_INFO "loop: loaded (max %d devices)\n", max_loop);
 	return 0;
 
-out_mem4:
-	while (i--)
-		blk_cleanup_queue(loop_dev[i].lo_queue);
-	i = max_loop;
-out_mem3:
-	while (i--)
-		put_disk(disks[i]);
-	kfree(disks);
-out_mem2:
-	kfree(loop_dev);
-out_mem1:
+ out_free_devs:
+	list_for_each_entry_safe(lo, next, &loop_devices, lo_list)
+		loop_del_one(lo);
 	unregister_blkdev(LOOP_MAJOR, "loop");
 	printk(KERN_ERR "loop: ran out of memory\n");
-	return -ENOMEM;
+	return err;
 }
 
 static void loop_exit(void)
 {
-	int i;
+	struct loop_device *lo, *next;
+
+	list_for_each_entry_safe(lo, next, &loop_devices, lo_list)
+		loop_del_one(lo);
 
-	for (i = 0; i < max_loop; i++) {
-		del_gendisk(disks[i]);
-		blk_cleanup_queue(loop_dev[i].lo_queue);
-		put_disk(disks[i]);
-	}
 	if (unregister_blkdev(LOOP_MAJOR, "loop"))
 		printk(KERN_WARNING "loop: cannot unregister blkdev\n");
-
-	kfree(disks);
-	kfree(loop_dev);
 }
 
 module_init(loop_init);
Index: linux-2.6/include/linux/loop.h
===================================================================
--- linux-2.6.orig/include/linux/loop.h	2007-03-23 15:23:32.000000000 +0100
+++ linux-2.6/include/linux/loop.h	2007-03-23 15:30:40.000000000 +0100
@@ -64,6 +64,8 @@ struct loop_device {
 	wait_queue_head_t	lo_event;
 
 	request_queue_t		*lo_queue;
+	struct gendisk		*lo_disk;
+	struct list_head	lo_list;
 };
 
 #endif /* __KERNEL__ */

Re: [patch] [bugfix] loop.c

[Eric Dumazet]

On Fri, 23 Mar 2007 14:36:05 +0000
Al Viro <viro@ftp.linux.org.uk> wrote:

> On Fri, Mar 23, 2007 at 03:19:56PM +0100, Eric Dumazet wrote:
> > I cooked the following patch (untested), feel free to test it.
> 
> Please, get the cleanup into saner shape.  This is too ugly.

out_mem:
        while (nba--)
                blk_cleanup_queue(loop_dev[nba]->lo_queue);
        while (nbl--) {
                put_disk(loop_dev[nbl]->lo_disk);
                kfree(loop_dev[nbl]);
        }
        if (loop_dev_vmalloced)
                vfree(loop_dev);
        else
                kfree(loop_dev);
        unregister_blkdev(LOOP_MAJOR, "loop");


What is ugly in this code ?

Re: [patch] [bugfix] loop.c

[Al Viro]

On Fri, Mar 23, 2007 at 02:41:09PM +0000, Christoph Hellwig wrote:
> The right thing to start with is to split the allocation up, and allocate
> each loop device by itself, like in the untested patch below:
> 
> After that you're not wasting memory for any off number of loop devices
> and can create as many as you have device numbers available:

NAK.  You _can't_ bail out in init after add_disk().  It might have been
opened already.

Re: [patch] [bugfix] loop.c

[Eric Dumazet]

On Fri, 23 Mar 2007 15:25:23 +0100 (CET)
Jiri Kosina <jikos@jikos.cz> wrote:

> On Fri, 23 Mar 2007, Eric Dumazet wrote:
> 
> > -	if (max_loop < 1 || max_loop > 256) {
> > -		printk(KERN_WARNING "loop: invalid max_loop (must be between"
> > -				    " 1 and 256), using default (8)\n");
> > +	if (max_loop < 1) {
> > +		printk(KERN_WARNING "loop: invalid max_loop (must be > 1)"
> > +				    ", using default (8)\n");
> >  		max_loop = 8;
> >  	}
> [...]
> > +	loop_dev = kmalloc(max_loop * sizeof(struct loop_device *), GFP_KERNEL);
> > +	if (!loop_dev) {
> > +		loop_dev = vmalloc(max_loop * sizeof(struct loop_device *));
> > +		if (!loop_dev)
> > +			goto out_mem;
> > +		loop_dev_vmalloced = 1;
> >  	}
> 
> Why did you remove the upper bound check for max_loop value? Now you 
> effectively allow to max_loop * sizeof(struct loop_device *) to overflow, 
> when passed value of max_loop which is large enough. Or am I just blind?

Yes, I forgot to change this, but the new limit was 16384 in my mind

MODULE_PARM_DESC(max_loop, "Maximum number of loop devices (1-16384)");

Re: [patch] [bugfix] loop.c

[Al Viro]

On Fri, Mar 23, 2007 at 03:48:09PM +0100, Eric Dumazet wrote:
> On Fri, 23 Mar 2007 14:36:05 +0000
> Al Viro <viro@ftp.linux.org.uk> wrote:
> 
> > On Fri, Mar 23, 2007 at 03:19:56PM +0100, Eric Dumazet wrote:
> > > I cooked the following patch (untested), feel free to test it.
> > 
> > Please, get the cleanup into saner shape.  This is too ugly.
> 
> out_mem:
>         while (nba--)
>                 blk_cleanup_queue(loop_dev[nba]->lo_queue);
>         while (nbl--) {
>                 put_disk(loop_dev[nbl]->lo_disk);
>                 kfree(loop_dev[nbl]);
>         }
>         if (loop_dev_vmalloced)
>                 vfree(loop_dev);
>         else
>                 kfree(loop_dev);
>         unregister_blkdev(LOOP_MAJOR, "loop");
> 
> 
> What is ugly in this code ?

Your counters.  Two loops instead of one (no reason to allocate queues in
a a separate loop).  vmalloc conditional on kmalloc failure.

Re: [patch] [bugfix] loop.c

[Michael Tokarev]

Eric Dumazet wrote:
[]
> 
> MODULE_PARM_DESC(max_loop, "Maximum number of loop devices (1-16384)");

Speaking of which, I wonder...  Here, and in many other places.

If some variable is marked as MODULE_PARAM (or whatever it is called
nowadays), used in module init routine, AND subsequently used for various
bound checks and loops...

Consider this:

  MODULE_PARAM(n);
  foo_init() {
    mem = kmalloc(n * sizeof(void*));
    ..
  }
  foo_func() {
    for (i = 0; i < n; ++i)
      do_something_with_mem(mem[i])
  }

and so on.  Together with:

  # modprobe foo n=10
  # echo 20 > /sys/module/foo/parameters/n

After that, we have 10 entries in mem[], and
n is equal to 20, so the for-loop above will be
up to i=19.  Which will reference unallocated
memory....

Amd I dreaming?

Thanks.

/mjt

Re: [patch] [bugfix] loop.c

[Jiri Kosina]

On Fri, 23 Mar 2007, Michael Tokarev wrote:

> Speaking of which, I wonder...  Here, and in many other places.
> If some variable is marked as MODULE_PARAM (or whatever it is called 
> nowadays), used in module init routine, AND subsequently used for 
> various bound checks and loops...
[...]
> and so on.  Together with:
>   # modprobe foo n=10
>   # echo 20 > /sys/module/foo/parameters/n
> After that, we have 10 entries in mem[], and n is equal to 20, so the 
> for-loop above will be up to i=19.  Which will reference unallocated 
> memory.... Amd I dreaming?

This can be solved either by using module_param_call() in every module, 
and properly handling the "asynchronous" changes of module parameter, or 
by just disallowing the parameter from being tuned through sysfs 
completely (by setting the 'permissions' parameter of module_param() to 
zero ... which is what many drivers do anyway, I'd guess).

-- 
Jiri Kosina