Skip to contentSkip to conversation

Support compat_ioctl for block devices

PATCH In review Andi Kleen 2 participants 5 messages Jan 18, 2005

Subsystems (2)
VFS Block layer
lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

PATCH 1/1 Support compat_ioctl for block devices

Support passing down of compat_ioctl on block devices. This is needed for the compat_ioctl conversion of block drivers.

Signed-off-by Andi Kleen <[email protected]>
drivers/block/ioctl.c renamed from drivers/block/ioctl.c-o +17 -0
diff -u linux-2.6.11-rc1-bk4/drivers/block/ioctl.c-o linux-2.6.11-rc1-bk4/drivers/block/ioctl.c
@@ -3,6 +3,7 @@
 #include <linux/blkpg.h>
 #include <linux/backing-dev.h>
 #include <linux/buffer_head.h>
+#include <linux/smp_lock.h>
 #include <asm/uaccess.h>
 
 static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user *arg)

@@ -220,3 +221,19 @@
 	}
 	return -ENOTTY;
 }
+
+/* Most of the generic ioctls are handled in the normal fallback path.
+   This assumes the blkdev's low level compat_ioctl always returns
+   ENOIOCTLCMD for unknown ioctls. */
+long compat_blkdev_ioctl(struct file *file, unsigned cmd, unsigned long arg)
+{
+	struct block_device *bdev = file->f_dentry->d_inode->i_bdev;
+	struct gendisk *disk = bdev->bd_disk;
+	int ret = -ENOIOCTLCMD;
+	if (disk->fops->compat_ioctl) {
+		lock_kernel();
+		ret = disk->fops->compat_ioctl(file, cmd, arg);
+		unlock_kernel();
+	}
+	return ret;
+}
fs/block_dev.c renamed from fs/block_dev.c-o +3 -0
diff -u linux-2.6.11-rc1-bk4/fs/block_dev.c-o linux-2.6.11-rc1-bk4/fs/block_dev.c
@@ -804,6 +804,9 @@
 	.mmap		= generic_file_mmap,
 	.fsync		= block_fsync,
 	.ioctl		= block_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= compat_blkdev_ioctl,
+#endif
 	.readv		= generic_file_readv,
 	.writev		= generic_file_write_nolock,
 	.sendfile	= generic_file_sendfile,
include/linux/fs.h renamed from include/linux/fs.h-o +2 -0
diff -u linux-2.6.11-rc1-bk4/include/linux/fs.h-o linux-2.6.11-rc1-bk4/include/linux/fs.h
@@ -879,6 +879,7 @@
 	int (*open) (struct inode *, struct file *);
 	int (*release) (struct inode *, struct file *);
 	int (*ioctl) (struct inode *, struct file *, unsigned, unsigned long);
+	long (*compat_ioctl) (struct file *, unsigned, unsigned long);
 	int (*media_changed) (struct gendisk *);
 	int (*revalidate_disk) (struct gendisk *);
 	struct module *owner;

@@ -1291,6 +1292,7 @@
 extern struct file_operations def_fifo_fops;
 extern int ioctl_by_bdev(struct block_device *, unsigned, unsigned long);
 extern int blkdev_ioctl(struct inode *, struct file *, unsigned, unsigned long);
+extern long compat_blkdev_ioctl(struct file *, unsigned, unsigned long);
 extern int blkdev_get(struct block_device *, mode_t, unsigned);
 extern int blkdev_put(struct block_device *);
 extern int bd_claim(struct block_device *, void *);
#1 lore raw

drivers/block/ioctl.c line 239

Quoted 12 lines
drivers/block/ioctl.c renamed from drivers/block/ioctl.c-o +12 -0
+long compat_blkdev_ioctl(struct file *file, unsigned cmd, unsigned long arg)
+{
+	struct block_device *bdev = file->f_dentry->d_inode->i_bdev;
+	struct gendisk *disk = bdev->bd_disk;
+	int ret = -ENOIOCTLCMD;
+	if (disk->fops->compat_ioctl) {
+		lock_kernel();
+		ret = disk->fops->compat_ioctl(file, cmd, arg);
+		unlock_kernel();
+	}
+	return ret;
+}

- please don't introduce a new API with the BKL held.
- prototype isn't nice. just passing the gendisk for block_device should be enough.

also this wants documentation in Documentation/filesystems/Locking

author
#2 lore raw
Quoted 6 lines · Christoph Hellwig
Quoted 6 lines
drivers/block/ioctl.c renamed from drivers/block/ioctl.c-o +6 -0
+long compat_blkdev_ioctl(struct file *file, unsigned cmd, unsigned long arg)
+{
+	struct block_device *bdev = file->f_dentry->d_inode->i_bdev;
+	struct gendisk *disk = bdev->bd_disk;
+	int ret = -ENOIOCTLCMD;
+	if (disk->fops->compat_ioctl) {
Quoted 8 lines

drivers/block/ioctl.c line 239

Quoted 6 lines
drivers/block/ioctl.c renamed from drivers/block/ioctl.c-o +6 -0
+		lock_kernel();
+		ret = disk->fops->compat_ioctl(file, cmd, arg);
+		unlock_kernel();
+	}
+	return ret;
+}

- please don't introduce a new API with the BKL held.

Nope, I'm not going to audit zillions of low level functions for this.

Quoted 2 lines

- prototype isn't nice. just passing the gendisk for block_device should be enough.

No, it isn't, the compat handler needs cmd and arg, and file is useful when you pass it to an existing ioctl handler.

-Andi

#3 lore raw
Quoted 3 lines · Andi Kleen
Quoted 1 line

- please don't introduce a new API with the BKL held.

Nope, I'm not going to audit zillions of low level functions for this.

So just stick a lock_kernel() unlock_kernel() into the handler, it's not like there's more than a handfull of them.

Quoted 5 lines
Quoted 2 lines

- prototype isn't nice. just passing the gendisk for block_device should be enough.

No, it isn't, the compat handler needs cmd and arg, and file is useful when you pass it to an existing ioctl handler.

cmd/arg is needed, file shouldn't. If you care for the underlying handler add a version that doesn't take the file * either.

author
#4 lore raw
Quoted 7 lines · Christoph Hellwig
Quoted 3 lines · Andi Kleen
Quoted 1 line

- please don't introduce a new API with the BKL held.

Nope, I'm not going to audit zillions of low level functions for this.

So just stick a lock_kernel() unlock_kernel() into the handler, it's not like there's more than a handfull of them.

Hmm, possible, although it tends to be quite ugly (requiring either gotos or wrappers). But ok.

Quoted 9 lines
Quoted 6 lines
Quoted 3 lines

- prototype isn't nice. just passing the gendisk for block_device should be enough.

No, it isn't, the compat handler needs cmd and arg, and file is useful when you pass it to an existing ioctl handler.

cmd/arg is needed, file shouldn't. If you care for the underlying handler add a version that doesn't take the file * either.

Sorry, that didn't make any sense.

-Andi