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

[Franz Pletz]

Andrew Morton <akpm@xxxxxxxx> 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;
 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/