[PATCH] loopback device can't act as its backing store

[PATCH] loopback device can't act as its backing store

[Franz Pletz]

The patch below fixes a bug in loop which apparently causes the kernel to call
the initialization routine of a loopback device recursively while trying to set
the backing store to the loopback device it's being mapped to.

Ludwig Schmidt <ludoschmidt@web.de> found this misbehaviour by accident. After
having been informed by him, I analyzed the problem and wrote this patch.

You can verify this bug for instance by issuing the following command
     # mount -o loop /dev/loop0 /mnt
with /dev/loop0 being the first free loop device. You should sync before. ;-)

However, if you try setting the backing file of the loopback device using the
losetup utility, you won't experience any crash. For example:
     # losetup /dev/loop0 /dev/loop0

But as a matter of fact, the device will be busy until the next reboot. Forced
unloading of loop will succeed. But after reloading, the kernel will lock up on
any attempt accessing a loop device.
Consequently this bug needs to be resolved in any case, although it seems that
there may also be a bug in the mount utility. By looking at the source code of
mount, which ironically shares the same codebase as losetup within the
util-linux package, I couldn't find anything suspicious.

This bug is fully reproduceable on at least all recent 2.6 series kernels.
The patch below applies cleanly on 2.6.10-rc2.

Comments would be graciously appreciated as this being my first serious kernel
patch. ;-)


Signed-off-by: Franz Pletz <franz_pletz@t-online.de>

  linux/drivers/block/loop.c |    7 +++++++
   1 files changed, 7 insertions(+)

--- linux-2.6.10-rc2/drivers/block/loop.c	2004-11-25 19:56:32.000000000 +0100
+++ linux/drivers/block/loop.c	2004-12-02 23:39:43.516913144 +0100
@@ -596,6 +596,9 @@
  	old_file = lo->lo_backing_file;

  	error = -EINVAL;
+	/* new backing store mustn't be the loop device it's being mapped to */
+	if(inode->i_rdev == bdev->bd_dev)
+		goto out_putf;

  	if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
  		goto out_putf;
@@ -652,6 +655,10 @@
  		lo_flags |= LO_FLAGS_READ_ONLY;

  	error = -EINVAL;
+	/* new backing store mustn't be the loop device it's being mapped to */
+	if(inode->i_rdev == bdev->bd_dev)
+		goto out_putf;
+
  	if (S_ISREG(inode->i_mode) || S_ISBLK(inode->i_mode)) {
  		struct address_space_operations *aops = mapping->a_ops;
  		/*

Re: [PATCH] loopback device can't act as its backing store

[Phil Oester]

On Fri, Dec 03, 2004 at 08:51:03PM +0100, Franz Pletz wrote:
> Comments would be graciously appreciated as this being my first serious 
> kernel
> patch. ;-)

Your mailer mangled the patch.

Phil

Re: [PATCH] loopback device can't act as its backing store

[Franz Pletz]

On Fri, 3 Dec 2004 13:42:38 -0800, Phil Oester <kernel@linuxace.com> wrote:
> Your mailer mangled the patch.

Thanks for your feedback.
I apologize for any inconvenience at applying or evaluating the patch.

I think I got it now. Let's give it another try.


Signed-off-by: Franz Pletz <franz_pletz@t-online.de>

 linux/drivers/block/loop.c |    7 +++++++
  1 files changed, 7 insertions(+)

--- linux-2.6.10-rc2/drivers/block/loop.c	2004-11-25 19:56:32.000000000 +0100
+++ linux/drivers/block/loop.c	2004-12-02 23:39:43.516913144 +0100
@@ -596,6 +596,9 @@
 	old_file = lo->lo_backing_file;
 
 	error = -EINVAL;
+	/* new backing store mustn't be the loop device it's being mapped to */
+	if(inode->i_rdev == bdev->bd_dev)
+		goto out_putf;
 
 	if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
 		goto out_putf;
@@ -652,6 +655,10 @@
 		lo_flags |= LO_FLAGS_READ_ONLY;
 
 	error = -EINVAL;
+	/* new backing store mustn't be the loop device it's being mapped to */
+	if(inode->i_rdev == bdev->bd_dev)
+		goto out_putf;
+
 	if (S_ISREG(inode->i_mode) || S_ISBLK(inode->i_mode)) {
 		struct address_space_operations *aops = mapping->a_ops;
 		/*

Re: [PATCH] loopback device can't act as its backing store

[Andrew Morton]

franz_pletz@t-online.de (Franz Pletz) wrote:
>
> The patch below fixes a bug in loop which apparently causes the kernel to call
> the initialization routine of a loopback device recursively while trying to set
> the backing store to the loopback device it's being mapped to.

Your patch addresses direct loop0-on-loop0 recursion, but does it fix the
more complex loop-stacks which Chris Spiegel identified?

I don't think there's any actual infinite recursion in Chris's example - in
his case we simply stacked loop deveces too deep.  But a fix for Chris's
scenario will also fix the one which you identify, I think.  Andries posted
such a patch but I have not yet got around to looking at it.




Begin forwarded message:

Date: Fri, 12 Nov 2004 02:49:34 -0800
From: Chris Spiegel <lkml@happyjack.org>
To: linux-kernel@vger.kernel.org
Subject: Oops with loop devices on 2.6.9


Hi,
  While playing around with loop mounts on kernel 2.6.9 I managed to get
a kernel panic.  After messing around with it I can reproduce the
problem reliably.  The sequence I came up with to cause the problem:

mount -o loop /dev/loop/0 /mnt
mount -o loop /dev/loop/1 /mnt
mount -o loop /dev/loop/2 /mnt
mount /dev/loop/0 /mnt -t ext2

I know the above is silly and contrived.

An example oops is as follows (I copied this down on paper and then
back, so hopefully I made no transcription errors):

Unable to handle kernel paging request at virtual address 98858a6f
 printing eip:
c011345a
*pde = 00000000
Oops: 0000 [#1]
SMP
Modules linked in:
CPU     0
EIP     0060:[<c011345a>]    Not tainted VLI
EFLAGS  00010083   (2.6.9)
EIP is at do_page_fault+0x99/0x599
eax: c9100000   ebx: 65642f3c   ecx: 0000007b   edx: f7d4858b
esi: 00000000   edi: c01133c1   ebp: 988589ff   esp: c9100108
ds: 007b   es: 007b   ss: 0068
Unable to handle kernel NULL pointer dereference at virtual address 00000070
 printing eip:
c011345a
*pde = 00000000

I ran this through ksymoops, but it just spit it back at me with the
following tacked on:

Warning (Oops_read): Code line not seen, dumping what data is available


>>eax; c9100000 <pg0+8b9f000/3fa9d400>
>>edx; f7d4858b <pg0+377e758b/3fa9d400>
>>edi; c01133c1 <do_page_fault+0/599>
>>esp; c9100108 <pg0+8b9f108/3fa9d400>


1 warning issued.  Results may not be reliable.


So I'm not sure if that's useful.  I could also get my system to lock up
if I did the above, but without the loop1 and 2 devices.  One time it
just froze, no messages.  Another time I got:
double fault, gdt at c1408260 [255 bytes]

I'm attaching my kernel config if that's of any help.  If you'd like me
to reply to the list, please CC me so I can set the In-Reply-To header
properly.

Chris

Re: [PATCH] loopback device can't act as its backing store

[Andrew Morton]

Andrew Morton <akpm@osdl.org> wrote:
>
> Andries posted such a patch



Begin forwarded message:

Date: Sun, 14 Nov 2004 23:31:21 +0100 (MET)
From: <Andries.Brouwer@cwi.nl>
To: akpm@osdl.org, lkml@happyjack.org, torvalds@osdl.org
Cc: linux-kernel@vger.kernel.org
Subject: [PATCH?] prevent loop infinite recursion


After "losetup /dev/loop0 /dev/loop0" I see a hard crash upon
access of /dev/loop0. And of course the same happens after
"losetup /dev/loop0 /dev/loop1; "losetup /dev/loop1 /dev/loop0"

Chris Spiegel reports a slightly different crash doing similar things.

The easiest fix is saying "don't do that then".

The patch below adds a (somewhat ugly) test for recursion.
Maybe someone can think of a nicer version.

Andries

diff -uprN -X /linux/dontdiff a/drivers/block/loop.c b/drivers/block/loop.c
--- a/drivers/block/loop.c	2004-08-26 22:05:15.000000000 +0200
+++ b/drivers/block/loop.c	2004-11-14 22:43:31.000000000 +0100
@@ -622,10 +622,17 @@ static int loop_change_fd(struct loop_de
 	return error;
 }
 
+static inline int is_loop_device(struct file *file)
+{
+	struct inode *i = file->f_mapping->host;
+
+	return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
+}
+
 static int loop_set_fd(struct loop_device *lo, struct file *lo_file,
 		       struct block_device *bdev, unsigned int arg)
 {
-	struct file	*file;
+	struct file	*file, *f;
 	struct inode	*inode;
 	struct address_space *mapping;
 	unsigned lo_blocksize;
@@ -636,15 +643,28 @@ static int loop_set_fd(struct loop_devic
 	/* This is safe, since we have a reference from open(). */
 	__module_get(THIS_MODULE);
 
-	error = -EBUSY;
-	if (lo->lo_state != Lo_unbound)
-		goto out;
-
 	error = -EBADF;
 	file = fget(arg);
 	if (!file)
 		goto out;
 
+	error = -EBUSY;
+	if (lo->lo_state != Lo_unbound)
+		goto out_putf;
+
+	/* Avoid recursion */
+	f = file;
+	while (is_loop_device(f)) {
+		struct loop_device *l;
+
+		if (f->f_mapping->host == lo_file->f_mapping->host)
+			goto out_putf;
+		l = f->f_mapping->host->i_bdev->bd_disk->private_data;
+		if (l->lo_state == Lo_unbound)
+			break;
+		f = l->lo_backing_file;
+	}
+
 	mapping = file->f_mapping;
 	inode = mapping->host;
 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] loopback device can't act as its backing store

[Bodo Eggert]

Andrew Morton wrote:

> +        /* Avoid recursion */
> +        f = file;
> +        while (is_loop_device(f)) {
> +                struct loop_device *l;
> +
> +                if (f->f_mapping->host == lo_file->f_mapping->host)
> +                        goto out_putf;
> +                l = f->f_mapping->host->i_bdev->bd_disk->private_data;
> +                if (l->lo_state == Lo_unbound)
> +                        break;
> +                f = l->lo_backing_file;
> +        }
> 

This seems wrong to me. AFAI can see, this does not address

1) a->b->c->b->... (Maybe it' is catched later after some recursions)
2) the max. recursion problem you mentioned. (Maybe it's catched by
   not having enough loop devices and catching (1), but this may change)


I think the real fix is something like: (guess from this patch, untested)

f = file;
i = MAX_LOOP_CHAIN;
while (is_loop_device(f)) {
        struct loop_device *l;

/* if MAX_LOOP_CHAIN is high, leaving in the old check might be a nice
   shortcut for the common case. Is it?
 */

        if(!--i) goto out_putf;
/* off by one? I don't think so, but check it 'cause it's late. */

        l = f->f_mapping->host->i_bdev->bd_disk->private_data;
        if (l->lo_state == Lo_unbound)
                break;
/* shouldn't we generate an error instead?
   I guess the error will be generated while recursing, but we just
   followed the chain and know the result.
 */
        f = l->lo_backing_file;
}

Re: [PATCH] loopback device can't act as its backing store

[Franz Pletz]

Andrew Morton <akpm@osdl.org> wrote:
> Your patch addresses direct loop0-on-loop0 recursion, but does it fix the
> more complex loop-stacks which Chris Spiegel identified?

No, it doesn't. I was unfortunately much too focused on the loop0-loop0 problem to realize those issues.

> I don't think there's any actual infinite recursion in Chris's example - in
> his case we simply stacked loop deveces too deep.  But a fix for Chris's
> scenario will also fix the one which you identify, I think.  Andries posted
> such a patch but I have not yet got around to looking at it.

Andries' patch addresses all issues Chris and I encountered although raising some minor problems.

In the while-loop only the inodes are being compared. But you can have more than one device node pointing to this specific device. Consequently, we need to compare the dev_t structures like in my initial patch.

Moreover, if the loopback device l is unbound, the while-loop shouldn't be terminated as this (unbound!) device file would therefore be set as the new backing file, which makes no sense in my opinion and which additionally could lead into problems while accessing the device. I would propose a goto out_putf to return -EINVAL instead.

Now, Chris' and my issues with mount and losetup are resolved properly. I attached the patch of Andries including the proposed changes.

--- linux-orig/drivers/block/loop.c	2004-11-25 19:56:32.000000000 +0100
+++ linux/drivers/block/loop.c	2004-12-04 00:58:09.897336912 +0100
@@ -622,10 +622,17 @@
 	return error;
 }
 
+static inline int is_loop_device(struct file *file)
+{
+	struct inode *i = file->f_mapping->host;
+
+	return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
+}
+
 static int loop_set_fd(struct loop_device *lo, struct file *lo_file,
 		       struct block_device *bdev, unsigned int arg)
 {
-	struct file	*file;
+	struct file	*file, *f;
 	struct inode	*inode;
 	struct address_space *mapping;
 	unsigned lo_blocksize;
@@ -636,15 +643,31 @@
 	/* This is safe, since we have a reference from open(). */
 	__module_get(THIS_MODULE);
 
-	error = -EBUSY;
-	if (lo->lo_state != Lo_unbound)
-		goto out;
-
 	error = -EBADF;
 	file = fget(arg);
 	if (!file)
 		goto out;
 
+	error = -EBUSY;
+	if (lo->lo_state != Lo_unbound)
+		goto out_putf;
+
+	/* Avoid recursion */
+	f = file;
+	while (is_loop_device(f)) {
+		struct loop_device *l;
+
+		if (f->f_mapping->host->i_rdev == lo_file->f_mapping->host->i_rdev)
+			goto out_putf;
+		
+		l = f->f_mapping->host->i_bdev->bd_disk->private_data;
+		if (l->lo_state == Lo_unbound) {
+			error = -EINVAL;
+			goto out_putf;
+		}
+		f = l->lo_backing_file;
+	}
+
 	mapping = file->f_mapping;
 	inode = mapping->host;