Re: bug in dm-loop? - was:Re: Re: device mapper integrated loops - and one more year !

["Bryn M. Reeves" <breeves@redhat.com>]

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

devzero@web.de wrote:
> 
> i was doing some testing of dm-loop with latest dmsetup on a 2.6.20-rc5 system.
> 
> i did some basic test, which seems to work ok.
> 
> i can now map a file to become /dev/mapper/loop0
> 
> dmlosetup loop0 test.img 
> 
> -> device-mapper: loop: Finalized extent map of 1232 bytes, 44 entries.
> 
> root@localhost ~ # ls -la /dev/mapper/loop0
> brw------- 1 root root 252, 0 21. Jan 17:25 /dev/mapper/loop0

Great! Am glad this much is working for you!

> 
> while fiddling around a little bit, i accidentally tried to map that file a second time - anyway, i would expect this is quite ok and should work.
> 
> dmlosetup loop1 test.img 
> 
> this bails out with the following error, leaving trace in dmesg:  
> 
> device-mapper: loop: file is already in use: /root/device-mapper/dmsetup/test.dat
> BUG: unable to handle kernel NULL pointer dereference at virtual address 00000000
>  printing eip:
> d097589c
> *pde = 00000000
> Oops: 0000 [#1]

The dm-loop target only allows a file to be mapped once. That said, it
should just return the error, not oops the kernel!

This is fixed in my CVS copy of this patch - I'm attaching a patch that
you can apply on top of the one that you already have that includes this
fix plus a couple of other minor fixes/enhancements.

I'll also get the version on kernel.org updated - thanks for catching this!

> after this, if i do 
> dmlosetup -d loop1
> dmlosetup -d loop0
> 
> i cannot "rmmod dm-loop" , now telling me "device is in use", but it isn`t ! 
> 
> so, this looks like a bug to me, leaving dm-loop in an inconsitent state.

Unfortunately, after an oops in a kernel module you're pretty much
forced to reboot - the kernel terminates the code path that caused the
fault, leaving a hanging reference count on the module.

> furthermore, one question about naming conventions :
> 
> dmlosetup testloop1 test.dat
> dmlosetup: Could not parse loop_device testloop1
> Usage:
> 
> losetup [-d|-a] [-e encryption] [-o offset] [-f|loop_device] [file]
> 
> Couldn't process command line.
> 
> 
> so, it`s mandatory that loop_device needs to begin with "loop...." !?
> 

It is at the moment :) The current command line parameters try to be as
faithful as possible to the nomrmal losetup, where devices are normally
named as (/dev/)loopN. We can't exactly match that without disrupting
users of the regular loop driver though, so this should probably be relaxed.

Thanks for testing!

Bryn.





-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iD8DBQFFtItQ6YSQoMYUY94RAijWAJ47OJL21+0W3yiMqbq51k3EHAw0WACfaefN
PjSa8djCMZuyZ/gGyUAikp0=
=3bv3
-----END PGP SIGNATURE-----

[-- Attachment #2: dm-loop.fix_busy_image.patch --]
[-- Type: text/x-patch, Size: 2834 bytes --]

--- drivers/md/dm-loop.c.rel	2007-01-22 09:45:04.000000000 +0000
+++ drivers/md/dm-loop.c	2007-01-22 09:45:10.000000000 +0000
@@ -22,7 +22,7 @@
 #include "dm-bio-record.h"
 
 #define DM_MSG_PREFIX "loop"
-#define LOOP_MAX_EXTENTS 1024
+#define LOOP_MAX_EXTENTS 2048
 
 #define	DMLOOP_READONLY	0x01
 #define	DMLOOP_SYNC	0x02
@@ -69,7 +69,7 @@ struct loop_c {
 };
 
 #ifdef CONFIG_DM_DEBUG
-static void dump_extent(struct extent *e)
+static void dump_extent(unsigned i, struct extent *e)
 {
 	const char types[] = { 'f', 'd' };
 
@@ -81,8 +81,8 @@ static void dump_extent(struct extent *e
 		return;
 	}
 
-	DMDEBUG("start: %8llu len: %4llu %4c.rstart: %8llu",
-				e->start, e->len, types[e->type],
+	DMDEBUG(" [%u] start: %8llu len: %4llu %4c.rstart: %8llu",
+				i, e->start, e->len, types[e->type],
 				(sector_t)e->data );
 }
 
@@ -97,7 +97,7 @@ static void dump_extent_map(struct exten
 			map->nr_extents, map->cur_extent);
 
 	for (i = 0; i < map->nr_extents; i++)
-		dump_extent(DMLOOP_EXTENT(i));
+		dump_extent(i, DMLOOP_EXTENT(i));
 }
 
 #else /* CONFIG_DM_DEBUG */
@@ -228,16 +228,25 @@ reprobe:
 		continue;
 	}
 
+	if (nr_extents == LOOP_MAX_EXTENTS && probe_block < last_block){
+		DMERR("Loopfile contains more than LOOP_MAX_EXTENTS (%d) fragments\n",
+					LOOP_MAX_EXTENTS);
+		goto out_free;
+	}
+
 	map->nr_extents = nr_extents;
 	map->cur_extent = 0;
 
 	DMDEBUG("created initial extent map, finalizing.");
 	map = finalize_map(map);
+	if (!map)
+		return -ENOMEM;
+
+	//dump_extent_map(map);
 	DMINFO("Finalized extent map of %u bytes, %d entries.",
 			(map->nr_extents * sizeof(struct extent)),
 			map->nr_extents);
 
-	dump_extent_map(map);
 	lc->blkbits = blkbits;
 	lc->map = map;
 
@@ -408,7 +417,7 @@ static struct file *loop_get_file(char *
 {
 	struct file *filp;
 	struct inode *inode;
-	int r;
+	int r = 0;
 
 	filp = filp_open(loop_path,
 			 ((*flags & DMLOOP_READONLY) ? O_RDONLY : O_RDWR) |
@@ -434,6 +443,7 @@ static struct file *loop_get_file(char *
 
 	if (IS_SWAPFILE(inode)) {
 		DMERR("file is already in use: %s", loop_path);
+		r = -EBUSY;
 		goto out;
 	}
 
@@ -461,7 +471,7 @@ static int loop_setup_size(struct loop_c
 	lc->size = i_size_read(inode);
 	lc->blkbits = inode->i_blkbits;
 
-	if (lc->offset & (1 << lc->blkbits - 1)) {
+	if (lc->offset & ((1 << lc->blkbits) - 1)) {
 		DMERR("Backing file offset of %lld bytes not a multiple of "
 			"filesystem blocksize (%d)", lc->offset,
 			1 << lc->blkbits);
@@ -565,8 +575,11 @@ static int loop_ctr(struct dm_target *ti
 		goto out_putf;
 
 	r = setup_loop_extents(lc);
-	if (r) {
-		ti->error = "Could not create extent map";
+	if (r == -EINVAL) {
+		ti->error = "Could not create extent map - invalid argument";
+		goto out_putf;
+	} else if (r == -ENOMEM) {
+		ti->error = "Could not allocate extent map";
 		goto out_putf;
 	}
 

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]