[PATCH] dm: support ioctls on mapped devices: fix with fake file

[PATCH] dm: support ioctls on mapped devices: fix with fake file

[Alasdair G Kergon]

[This applies after all the current dm patches.]


From: Milan Broz <mbroz@redhat.com>

The new ioctl code passes the wrong file pointer to the underlying device.
No file pointer is available so make a temporary fake one.

Signed-off-by: Milan Broz <mbroz@redhat.com>
Signed-off-by: Alasdair G Kergon <agk@redhat.com>

Index: linux-2.6.17/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.17.orig/drivers/md/dm-mpath.c	2006-06-23 19:17:27.000000000 +0100
+++ linux-2.6.17/drivers/md/dm-mpath.c	2006-06-23 19:17:40.000000000 +0100
@@ -1272,15 +1272,22 @@ static int multipath_ioctl(struct dm_tar
 	struct multipath *m = (struct multipath *) ti->private;
 	struct block_device *bdev = NULL;
 	unsigned long flags;
+	struct file fake_file = {};
+	struct dentry fake_dentry = {};
 	int r = 0;
 
+	fake_file.f_dentry = &fake_dentry;
+
 	spin_lock_irqsave(&m->lock, flags);
 
 	if (!m->current_pgpath)
 		__choose_pgpath(m);
 
-	if (m->current_pgpath)
+	if (m->current_pgpath) {
 		bdev = m->current_pgpath->path.dev->bdev;
+		fake_dentry.d_inode = bdev->bd_inode;
+		fake_file.f_mode = m->current_pgpath->path.dev->mode;
+	}
 
 	if (m->queue_io)
 		r = -EAGAIN;
@@ -1289,8 +1296,8 @@ static int multipath_ioctl(struct dm_tar
 
 	spin_unlock_irqrestore(&m->lock, flags);
 
-	return r ? : blkdev_driver_ioctl(bdev->bd_inode, filp, bdev->bd_disk,
-		     cmd, arg);
+	return r ? : blkdev_driver_ioctl(bdev->bd_inode, &fake_file,
+					 bdev->bd_disk, cmd, arg);
 }
 
 /*-----------------------------------------------------------------
Index: linux-2.6.17/drivers/md/dm-linear.c
===================================================================
--- linux-2.6.17.orig/drivers/md/dm-linear.c	2006-06-23 19:17:27.000000000 +0100
+++ linux-2.6.17/drivers/md/dm-linear.c	2006-06-23 19:17:40.000000000 +0100
@@ -104,8 +104,14 @@ static int linear_ioctl(struct dm_target
 {
 	struct linear_c *lc = (struct linear_c *) ti->private;
 	struct block_device *bdev = lc->dev->bdev;
+	struct file fake_file = {};
+	struct dentry fake_dentry = {};
 
-	return blkdev_driver_ioctl(bdev->bd_inode, filp, bdev->bd_disk, cmd, arg);
+	fake_file.f_mode = lc->dev->mode;
+	fake_file.f_dentry = &fake_dentry;
+	fake_dentry.d_inode = bdev->bd_inode;
+
+	return blkdev_driver_ioctl(bdev->bd_inode, &fake_file, bdev->bd_disk, cmd, arg);
 }
 
 static struct target_type linear_target = {

Re: [PATCH] dm: support ioctls on mapped devices: fix with fake file

[Christoph Hellwig]

On Fri, Jun 23, 2006 at 07:38:29PM +0100, Alasdair G Kergon wrote:
> [This applies after all the current dm patches.]

NACK.  You should use ioctl_by_bdev and fix it to fake up the file pointer
instead.  That keeps faking the file pointer in a single place and solves
the problem of NULL file pointers for other cases like the s390 partitioning
code, or various odd filesystems figuring out partition tables by themselves.

For bonus points use a common helper for both blkdev_get and ioctl_by_bdev
that creates the fake struct file.

Re: [PATCH] dm: support ioctls on mapped devices: fix with fake file

[Alasdair G Kergon]

On Fri, Jun 23, 2006 at 07:45:20PM +0100, Christoph Hellwig wrote:
> NACK.  You should use ioctl_by_bdev and fix it to fake up the file pointer
> instead.  That keeps faking the file pointer in a single place and solves
> the problem of NULL file pointers for other cases like the s390 partitioning
> code, or various odd filesystems figuring out partition tables by themselves.
> For bonus points use a common helper for both blkdev_get and ioctl_by_bdev
> that creates the fake struct file.

Fixing the existing callers that pass a NULL file struct is a separate matter
that can be handled by a later patch, as can consolidating all the fake users.

I don't follow the reasoning behind backing out and using ioctl_by_bdev() here.
Isn't the primary reason for its existence to add a set_fs(KERNEL_DS) to the
mix?  I don't understand why device-mapper should interfere (surely
incorrectly?) at that level.

Alasdair
-- 
agk@redhat.com