Skip to contentSkip to conversation

Fix blktrace setup 32-bit ioctl on 64-bit kernels

PATCH In review Jens Axboe 3 participants 14 messages Oct 2, 2007 – Oct 5, 2007

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

Jens Axboe

author
#0 lore raw

PATCH 1/1 Fix blktrace setup 32-bit ioctl on 64-bit kernels

Hi,

The layout of struct blk_user_trace_setup is a bit unfortunate, it gets padded differently on 32-bit and 64-bit archs. So right now it's not possible to trace 64-bit kernels with a 32-bit app. This patch fixes that up by adding a compat ioctl handler for BLKTRACESETUP.

Signed-off-by Jens Axboe <[email protected]>
fs/compat_ioctl.c +46 -1
index 5a5b711..b18b9cc 100644
@@ -2052,6 +2052,51 @@ static int raw_ioctl(unsigned fd, unsigned cmd, unsigned long arg)
         }
         return ret;
 }
+
+struct blk_user_trace_setup32 {
+	char name[32];
+	u16 act_mask;
+	u16 pad;
+	u32 buf_size;
+	u32 buf_nr;
+	u64 start_lba;
+	u64 end_lba;
+	u32 pid;
+} __attribute__((packed));
+
+#define BLKTRACESETUP32 _IOWR(0x12,115,struct blk_user_trace_setup32)
+
+static int blktrace32_setup(int fd, unsigned cmd, unsigned long arg)
+{
+	struct blk_user_trace_setup __user *buts = compat_alloc_user_space(sizeof(*buts));
+	struct blk_user_trace_setup32 __user *buts32 = compat_ptr(arg);
+	int err;
+
+	if (copy_in_user(&buts->name, &buts32->name, BDEVNAME_SIZE) ||
+	    get_user(buts->act_mask, &buts32->act_mask) ||
+	    get_user(buts->buf_size, &buts32->buf_size) ||
+	    get_user(buts->buf_nr, &buts32->buf_nr) ||
+	    get_user(buts->start_lba, &buts32->start_lba) ||
+	    get_user(buts->end_lba, &buts32->end_lba) ||
+	    get_user(buts->pid, &buts32->pid))
+		return -EFAULT;
+
+	err = sys_ioctl(fd, BLKTRACESETUP, (unsigned long) buts);
+	if (err)
+		return err;
+
+	if (copy_to_user(&buts32->name, &buts->name, BDEVNAME_SIZE) ||
+	    put_user(buts32->act_mask, &buts->act_mask) ||
+	    put_user(buts32->buf_size, &buts->buf_size) ||
+	    put_user(buts32->buf_nr, &buts->buf_nr) ||
+	    put_user(buts32->start_lba, &buts->start_lba) ||
+	    put_user(buts32->end_lba, &buts->end_lba) ||
+	    put_user(buts32->pid, &buts->pid))
+		return -EFAULT;
+
+	return err;
+}
+
 #endif /* CONFIG_BLOCK */
 
 struct serial_struct32 {

@@ -2555,7 +2600,7 @@ COMPATIBLE_IOCTL(BLKSECTSET)
 COMPATIBLE_IOCTL(BLKSSZGET)
 COMPATIBLE_IOCTL(BLKTRACESTART)
 COMPATIBLE_IOCTL(BLKTRACESTOP)
-COMPATIBLE_IOCTL(BLKTRACESETUP)
+HANDLE_IOCTL(BLKTRACESETUP32, blktrace32_setup)
 COMPATIBLE_IOCTL(BLKTRACETEARDOWN)
 ULONG_IOCTL(BLKRASET)
 ULONG_IOCTL(BLKFRASET)
Signature · 1 line
Jens Axboe
Acked-by on v1 1/1
#1 lore raw

From: Jens Axboe <[email protected]>
Date: Tue, 2 Oct 2007 09:39:43 +0200

Quoted 8 lines

Hi,

The layout of struct blk_user_trace_setup is a bit unfortunate, it gets padded differently on 32-bit and 64-bit archs. So right now it's not possible to trace 64-bit kernels with a 32-bit app. This patch fixes that up by adding a compat ioctl handler for BLKTRACESETUP.

Signed-off-by Jens Axboe <[email protected]>
Acked-by David S. Miller <[email protected]>
#2 lore raw
In reply to Jens Axboe
Quoted 5 lines · Jens Axboe

The layout of struct blk_user_trace_setup is a bit unfortunate, it gets padded differently on 32-bit and 64-bit archs. So right now it's not possible to trace 64-bit kernels with a 32-bit app. This patch fixes that up by adding a compat ioctl handler for BLKTRACESETUP.

actually, I would guess that it is currently working on s390, sparc64, powerpc, parisc and mips, but your patch breaks it :(.

fs/compat_ioctl.c

Quoted 4 lines
fs/compat_ioctl.c +0 -0
index 5a5b711..b18b9cc 100644

I'd prefer to not add anything to fs/compat_ioctl.c at all, but always handle these in the places where the native version is handled.

In your case, I'd either mark BLKTRACESETUP32 as COMPATIBLE_IOCTL() and handle it from inside of blk_trace_ioctl(), or handle it in compat_blkdev_ioctl.

Quoted 5 lines
fs/compat_ioctl.c +1 -0
@@ -2052,6 +2052,51 @@ static int raw_ioctl(unsigned fd, unsigned cmd, unsigned long arg)
         }
         return ret;
 }
+

fs/compat_ioctl.c static int raw_ioctl(unsigned fd, unsigned cmd, unsigned long arg) · line 2065

Quoted 10 lines
fs/compat_ioctl.c +10 -0
+struct blk_user_trace_setup32 {
+	char name[32];
+	u16 act_mask;
+	u16 pad;
+	u32 buf_size;
+	u32 buf_nr;
+	u64 start_lba;
+	u64 end_lba;
+	u32 pid;
+} __attribute__((packed));

Errm, no. Everyone makes that mistake once, so you're in good company, but the packed attribute makes this incorrect on every architecture except x86_64 and ia64, because only i386 has no padding before the u64 and after the last member.

We now have the compat_u64 type that behaves like the 32 bit user space version of an unsigned long long. If you use that to define compat_blk_user_trace_setup, you don't need the attribute.

fs/compat_ioctl.c static int raw_ioctl(unsigned fd, unsigned cmd, unsigned long arg) · line 2069

Quoted 3 lines
fs/compat_ioctl.c +3 -0
+#define BLKTRACESETUP32 _IOWR(0x12,115,struct blk_user_trace_setup32)
+
+static int blktrace32_setup(int fd, unsigned cmd, unsigned long arg)

The naming convention these days is to use a 'compat_' prefix, not a '32' postfix.

Quoted 18 lines
fs/compat_ioctl.c +18 -0
+{
+	struct blk_user_trace_setup __user *buts = compat_alloc_user_space(sizeof(*buts));
+	struct blk_user_trace_setup32 __user *buts32 = compat_ptr(arg);
+	int err;
+
+	if (copy_in_user(&buts->name, &buts32->name, BDEVNAME_SIZE) ||
+	    get_user(buts->act_mask, &buts32->act_mask) ||
+	    get_user(buts->buf_size, &buts32->buf_size) ||
+	    get_user(buts->buf_nr, &buts32->buf_nr) ||
+	    get_user(buts->start_lba, &buts32->start_lba) ||
+	    get_user(buts->end_lba, &buts32->end_lba) ||
+	    get_user(buts->pid, &buts32->pid))
+		return -EFAULT;
+
+	err = sys_ioctl(fd, BLKTRACESETUP, (unsigned long) buts);
+	if (err)
+		return err;
+

fs/compat_ioctl.c static int raw_ioctl(unsigned fd, unsigned cmd, unsigned long arg) · line 2097

Quoted 10 lines
fs/compat_ioctl.c +10 -0
+	if (copy_to_user(&buts32->name, &buts->name, BDEVNAME_SIZE) ||
+	    put_user(buts32->act_mask, &buts->act_mask) ||
+	    put_user(buts32->buf_size, &buts->buf_size) ||
+	    put_user(buts32->buf_nr, &buts->buf_nr) ||
+	    put_user(buts32->start_lba, &buts->start_lba) ||
+	    put_user(buts32->end_lba, &buts->end_lba) ||
+	    put_user(buts32->pid, &buts->pid))
+		return -EFAULT;
+
+	return err;

Most of these fields are read-only for the kernel, so you should only need the first copy_to_user. I think you should split the blk_trace_setup function to have the common code take a struct blk_user_trace_setup kernel pointer, and one or two versions that just do the copy_{to,from}_user.

	Arnd <><
author
#3 lore raw
Quoted 9 lines · Arnd Bergmann
Quoted 5 lines · Jens Axboe

The layout of struct blk_user_trace_setup is a bit unfortunate, it gets padded differently on 32-bit and 64-bit archs. So right now it's not possible to trace 64-bit kernels with a 32-bit app. This patch fixes that up by adding a compat ioctl handler for BLKTRACESETUP.

actually, I would guess that it is currently working on s390, sparc64, powerpc, parisc and mips, but your patch breaks it :(.

Not so good. But how could it work on the above archs, surely the padding (and thuse resulting size of the structure) is different?

Quoted 7 lines

fs/compat_ioctl.c

Quoted 4 lines
fs/compat_ioctl.c +0 -0
index 5a5b711..b18b9cc 100644

I'd prefer to not add anything to fs/compat_ioctl.c at all, but always handle these in the places where the native version is handled.

Fine with me.

Quoted 3 lines

In your case, I'd either mark BLKTRACESETUP32 as COMPATIBLE_IOCTL() and handle it from inside of blk_trace_ioctl(), or handle it in compat_blkdev_ioctl.

OK

Quoted 21 lines
Quoted 5 lines
fs/compat_ioctl.c +1 -0
@@ -2052,6 +2052,51 @@ static int raw_ioctl(unsigned fd, unsigned cmd, unsigned long arg)
         }
         return ret;
 }
+

fs/compat_ioctl.c static int raw_ioctl(unsigned fd, unsigned cmd, unsigned long arg) · line 2065

Quoted 10 lines
fs/compat_ioctl.c +10 -0
+struct blk_user_trace_setup32 {
+	char name[32];
+	u16 act_mask;
+	u16 pad;
+	u32 buf_size;
+	u32 buf_nr;
+	u64 start_lba;
+	u64 end_lba;
+	u32 pid;
+} __attribute__((packed));

Errm, no. Everyone makes that mistake once, so you're in good company, but the packed attribute makes this incorrect on every architecture except x86_64 and ia64, because only i386 has no padding before the u64 and after the last member.

Quoted 3 lines

We now have the compat_u64 type that behaves like the 32 bit user space version of an unsigned long long. If you use that to define compat_blk_user_trace_setup, you don't need the attribute.

And ditto for u16 and u32, I gather? If you notice, it's not padding in front of the u64 that bites me here, it's before the u32.

Quoted 6 lines

fs/compat_ioctl.c static int raw_ioctl(unsigned fd, unsigned cmd, unsigned long arg) · line 2069

Quoted 3 lines
fs/compat_ioctl.c +3 -0
+#define BLKTRACESETUP32 _IOWR(0x12,115,struct blk_user_trace_setup32)
+
+static int blktrace32_setup(int fd, unsigned cmd, unsigned long arg)

The naming convention these days is to use a 'compat_' prefix, not a '32' postfix.

OK, I will adapt.

Quoted 25 lines
Quoted 25 lines
fs/compat_ioctl.c +25 -0
+{
+	struct blk_user_trace_setup __user *buts = compat_alloc_user_space(sizeof(*buts));
+	struct blk_user_trace_setup32 __user *buts32 = compat_ptr(arg);
+	int err;
+
+	if (copy_in_user(&buts->name, &buts32->name, BDEVNAME_SIZE) ||
+	    get_user(buts->act_mask, &buts32->act_mask) ||
+	    get_user(buts->buf_size, &buts32->buf_size) ||
+	    get_user(buts->buf_nr, &buts32->buf_nr) ||
+	    get_user(buts->start_lba, &buts32->start_lba) ||
+	    get_user(buts->end_lba, &buts32->end_lba) ||
+	    get_user(buts->pid, &buts32->pid))
+		return -EFAULT;
+
+	err = sys_ioctl(fd, BLKTRACESETUP, (unsigned long) buts);
+	if (err)
+		return err;
+
+	if (copy_to_user(&buts32->name, &buts->name, BDEVNAME_SIZE) ||
+	    put_user(buts32->act_mask, &buts->act_mask) ||
+	    put_user(buts32->buf_size, &buts->buf_size) ||
+	    put_user(buts32->buf_nr, &buts->buf_nr) ||
+	    put_user(buts32->start_lba, &buts->start_lba) ||
+	    put_user(buts32->end_lba, &buts->end_lba) ||
+	    put_user(buts32->pid, &buts->pid))
Quoted 8 lines

fs/compat_ioctl.c static int raw_ioctl(unsigned fd, unsigned cmd, unsigned long arg) · line 2086

Quoted 3 lines
fs/compat_ioctl.c +3 -0
+		return -EFAULT;
+
+	return err;

Most of these fields are read-only for the kernel, so you should only need the first copy_to_user. I think you should split the blk_trace_setup function to have the common code take a struct blk_user_trace_setup kernel pointer, and one or two versions that just do the copy_{to,from}_user.

I was pretty sure that we modified start_lba and end_lba and copied those back as well, so it was a conscious decision to just copy everything for safety. But checking we only do mangle and copy back ->name, so your suggestion is a good one.

Signature · 1 line
Jens Axboe
author
#4 lore raw

Hi Arnd,

Updated patch below. I kept the code in compat_ioctl.c, to me it seems like the cleanest approach. I need the BLKTRACESETUP32 define both in compat_ioctl.c and blktrace.c if I move it, and I need to hard-core the struct size or define it in both places. And guard the code in blktrace.c with an ifdef for CONFIG_COMPAT. Not pretty, imho.

I haven't tested this one yet, but at least it compiles and the sizing seems right. The u16 padding was an artifact of the __attribute__((packed)) so that could be removed.

fs/compat_ioctl.c +39 -1
index 5a5b711..54162e5 100644
@@ -2052,6 +2052,44 @@ static int raw_ioctl(unsigned fd, unsigned cmd, unsigned long arg)
         }
         return ret;
 }
+
+struct blk_user_trace_setup32 {
+	char name[32];
+	u16 act_mask;
+	u32 buf_size;
+	u32 buf_nr;
+	compat_u64 start_lba;
+	compat_u64 end_lba;
+	u32 pid;
+};
+
+#define BLKTRACESETUP32 _IOWR(0x12,115,struct blk_user_trace_setup32)
+
+static int blktrace32_setup(int fd, unsigned cmd, unsigned long arg)
+{
+	struct blk_user_trace_setup __user *buts = compat_alloc_user_space(sizeof(*buts));
+	struct blk_user_trace_setup32 __user *buts32 = compat_ptr(arg);
+	int err;
+
+	if (copy_in_user(&buts->name, &buts32->name, 32) ||
+	    get_user(buts->act_mask, &buts32->act_mask) ||
+	    get_user(buts->buf_size, &buts32->buf_size) ||
+	    get_user(buts->buf_nr, &buts32->buf_nr) ||
+	    get_user(buts->start_lba, &buts32->start_lba) ||
+	    get_user(buts->end_lba, &buts32->end_lba) ||
+	    get_user(buts->pid, &buts32->pid))
+		return -EFAULT;
+
+	err = sys_ioctl(fd, cmd, (unsigned long) buts);
+	if (err)
+		return err;
+
+	if (copy_to_user(&buts32->name, &buts->name, 32))
+		return -EFAULT;
+
+	return err;
+}
+
 #endif /* CONFIG_BLOCK */
 
 struct serial_struct32 {

@@ -2555,7 +2593,7 @@ COMPATIBLE_IOCTL(BLKSECTSET)
 COMPATIBLE_IOCTL(BLKSSZGET)
 COMPATIBLE_IOCTL(BLKTRACESTART)
 COMPATIBLE_IOCTL(BLKTRACESTOP)
-COMPATIBLE_IOCTL(BLKTRACESETUP)
+HANDLE_IOCTL(BLKTRACESETUP32, blktrace32_setup)
 COMPATIBLE_IOCTL(BLKTRACETEARDOWN)
 ULONG_IOCTL(BLKRASET)
 ULONG_IOCTL(BLKFRASET)
Signature · 1 line
Jens Axboe
#5 lore raw
Quoted 11 lines · Jens Axboe

Hi Arnd,

Updated patch below. I kept the code in compat_ioctl.c, to me it seems like the cleanest approach. I need the BLKTRACESETUP32 define both in compat_ioctl.c and blktrace.c if I move it, and I need to hard-core the struct size or define it in both places. And guard the code in blktrace.c with an ifdef for CONFIG_COMPAT. Not pretty, imho.

I haven't tested this one yet, but at least it compiles and the sizing seems right. The u16 padding was an artifact of the __attribute__((packed)) so that could be removed.

The sizes are ok now, but I still don't like the idea of adding more stuff to fs/compat_ioctl.c. I also noticed another problem now, see below.

The preferred way to define compat_ioctl handlers is to use a ->compat_ioctl file operation so you don't need any code in compat_ioctl.c at all. You still need the #ifdef in blktrace.c though if you want to building extra code on the architectures that don't need it.

Quoted 4 lines
fs/compat_ioctl.c +4 -0
+static int blktrace32_setup(int fd, unsigned cmd, unsigned long arg)
+{
+	struct blk_user_trace_setup __user *buts = compat_alloc_user_space(sizeof(*buts));
+	struct blk_user_trace_setup32 __user *buts32 = compat_ptr(arg);

fs/compat_ioctl.c static int raw_ioctl(unsigned fd, unsigned cmd, unsigned long arg) · line 2081

Quoted 10 lines
fs/compat_ioctl.c +10 -0
+	int err;
+
+	if (copy_in_user(&buts->name, &buts32->name, 32) ||
+	    get_user(buts->act_mask, &buts32->act_mask) ||
+	    get_user(buts->buf_size, &buts32->buf_size) ||
+	    get_user(buts->buf_nr, &buts32->buf_nr) ||
+	    get_user(buts->start_lba, &buts32->start_lba) ||
+	    get_user(buts->end_lba, &buts32->end_lba) ||
+	    get_user(buts->pid, &buts32->pid))
+		return -EFAULT;

You are dereferencing 'buts' here, which is a user space pointer. This is broken and cannot work on architectures that have split kernel/user address spaces, and a potential security hole on those that don't. sparse would warn about this kind of bug, but of course one of the problems with fs/compat_ioctl.c is that it isn't sparse clean in the first place.

fs/compat_ioctl.c static int raw_ioctl(unsigned fd, unsigned cmd, unsigned long arg) · line 2088

Quoted 6 lines
fs/compat_ioctl.c +6 -0
+	err = sys_ioctl(fd, cmd, (unsigned long) buts);
+	if (err)
+		return err;
+
+	if (copy_to_user(&buts32->name, &buts->name, 32))
+		return -EFAULT;

Same here, this needs to be copy_in_user.

	Arnd <><
#6 lore raw
In reply to Jens Axboe

Here's my counterproposal for the blktrace compat code. It doesn't have any of the problems I found in your patch obviously, but I haven't tested it either, so I'm sure you can spot a bug or two in here. Comments?

Jens, I think the best overall solution would be to have a block/compat_ioctl.c file with all the compat handling for block devices moved over from fs/compat_ioctl.c, and done in a nicer way. If you agree, with this approach, I'd volunteer to come up with a patch.

Signed-off-by Arnd Bergmann <[email protected]>
block/blktrace.c +81 -20
Index: linux-2.6/block/blktrace.c
===================================================================
@@ -312,33 +312,26 @@ static struct rchan_callbacks blk_relay_
 /*
  * Setup everything required to start tracing
  */
-static int blk_trace_setup(struct request_queue *q, struct block_device *bdev,
-			   char __user *arg)
+static int do_blk_trace_setup(struct request_queue *q, struct block_device *bdev,
+			struct blk_user_trace_setup *buts)
 {
-	struct blk_user_trace_setup buts;
 	struct blk_trace *old_bt, *bt = NULL;
 	struct dentry *dir = NULL;
 	char b[BDEVNAME_SIZE];
 	int ret, i;
 
-	if (copy_from_user(&buts, arg, sizeof(buts)))
-		return -EFAULT;
-
-	if (!buts.buf_size || !buts.buf_nr)
+	if (!buts->buf_size || !buts->buf_nr)
 		return -EINVAL;
 
-	strcpy(buts.name, bdevname(bdev, b));
+	strcpy(buts->name, bdevname(bdev, b));
 
 	/*
 	 * some device names have larger paths - convert the slashes
 	 * to underscores for this to work as expected
 	 */
-	for (i = 0; i < strlen(buts.name); i++)
-		if (buts.name[i] == '/')
-			buts.name[i] = '_';
-
-	if (copy_to_user(arg, &buts, sizeof(buts)))
-		return -EFAULT;
+	for (i = 0; i < strlen(buts->name); i++)
+		if (buts->name[i] == '/')
+			buts->name[i] = '_';
 
 	ret = -ENOMEM;
 	bt = kzalloc(sizeof(*bt), GFP_KERNEL);

@@ -350,7 +343,7 @@ static int blk_trace_setup(struct reques
 		goto err;
 
 	ret = -ENOENT;
-	dir = blk_create_tree(buts.name);
+	dir = blk_create_tree(buts->name);
 	if (!dir)
 		goto err;
 

@@ -363,20 +356,21 @@ static int blk_trace_setup(struct reques
 	if (!bt->dropped_file)
 		goto err;
 
-	bt->rchan = relay_open("trace", dir, buts.buf_size, buts.buf_nr, &blk_relay_callbacks, bt);
+	bt->rchan = relay_open("trace", dir, buts->buf_size,
+				buts->buf_nr, &blk_relay_callbacks, bt);
 	if (!bt->rchan)
 		goto err;
 
-	bt->act_mask = buts.act_mask;
+	bt->act_mask = buts->act_mask;
 	if (!bt->act_mask)
 		bt->act_mask = (u16) -1;
 
-	bt->start_lba = buts.start_lba;
-	bt->end_lba = buts.end_lba;
+	bt->start_lba = buts->start_lba;
+	bt->end_lba = buts->end_lba;
 	if (!bt->end_lba)
 		bt->end_lba = -1ULL;
 
-	bt->pid = buts.pid;
+	bt->pid = buts->pid;
 	bt->trace_state = Blktrace_setup;
 
 	ret = -EBUSY;

@@ -401,6 +395,26 @@ err:
 	return ret;
 }
 
+static int blk_trace_setup(struct request_queue *q, struct block_device *bdev,
+			   char __user *arg)
+{
+	struct blk_user_trace_setup buts;
+	int ret;
+
+	ret = copy_from_user(&buts, arg, sizeof(buts));
+	if (ret)
+		return -EFAULT;
+
+	ret = do_blk_trace_setup(q, bdev, &buts);
+	if (ret)
+		return ret;
+
+	if (copy_to_user(arg, &buts, sizeof(buts)))
+		return -EFAULT;
+
+	return 0;
+}
+
 static int blk_trace_startstop(struct request_queue *q, int start)
 {
 	struct blk_trace *bt;

@@ -474,6 +488,53 @@ int blk_trace_ioctl(struct block_device 
 	return ret;
 }
 
+#if defined(CONFIG_COMPAT)
+static inline int compat_blk_trace_setup(struct request_queue *q,
+			struct block_device *bdev, char __user *arg)
+{
+	struct blk_user_trace_setup buts;
+	struct compat_blk_user_trace_setup cbuts;
+	int ret;
+
+	if (copy_from_user(&cbuts, arg, sizeof(cbuts)))
+		return -EFAULT;
+
+	buts = (struct blk_user_trace_setup) {
+		.act_mask = cbuts.act_mask,
+		.buf_size = cbuts.buf_size,
+		.buf_nr = cbuts.buf_nr,
+		.start_lba = cbuts.start_lba,
+		.end_lba = cbuts.end_lba,
+		.pid = cbuts.pid,
+	};
+	memcpy(&buts.name, &cbuts.name, 32);
+
+	mutex_lock(&bdev->bd_mutex);
+	ret = do_blk_trace_setup(q, bdev, &buts);
+	mutex_unlock(&bdev->bd_mutex);
+	if (ret)
+		return ret;
+
+	if (copy_to_user(arg, &buts.name, 32))
+		return -EFAULT;
+
+	return 0;
+}
+
+int compat_blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg)
+{
+
+	if (cmd == BLKTRACESETUP32) {
+		struct request_queue *q = bdev_get_queue(bdev);
+		if (!q)
+			return -ENXIO;
+		return compat_blk_trace_setup(q, bdev, arg);
+	}
+
+	return blk_trace_ioctl(bdev, cmd, arg);
+}
+#endif
+
 /**
  * blk_trace_shutdown: - stop and cleanup trace structures
  * @q:    the request queue associated with the device
block/ioctl.c +12 -1
Index: linux-2.6/block/ioctl.c
===================================================================
@@ -290,14 +290,25 @@ int blkdev_ioctl(struct inode *inode, st
    ENOIOCTLCMD for unknown ioctls. */
 long compat_blkdev_ioctl(struct file *file, unsigned cmd, unsigned long arg)
 {
+	int ret = -ENOIOCTLCMD;
+#ifdef CONFIG_COMPAT
 	struct block_device *bdev = file->f_path.dentry->d_inode->i_bdev;
 	struct gendisk *disk = bdev->bd_disk;
-	int ret = -ENOIOCTLCMD;
+
+	switch (cmd) {
+	case BLKTRACESTART:
+	case BLKTRACESTOP:
+	case BLKTRACESETUP32:
+	case BLKTRACETEARDOWN:
+		return blk_trace_ioctl(bdev, cmd, (char __user *) arg);
+	}
+
 	if (disk->fops->compat_ioctl) {
 		lock_kernel();
 		ret = disk->fops->compat_ioctl(file, cmd, arg);
 		unlock_kernel();
 	}
+#endif
 	return ret;
 }
include/linux/blktrace_api.h +18 -0
Index: linux-2.6/include/linux/blktrace_api.h
===================================================================
@@ -142,6 +142,23 @@ struct blk_user_trace_setup {
 	u32 pid;
 };
 
+#ifdef __KERNEL__
+#include <linux/compat.h>
+
+#ifdef CONFIG_COMPAT
+struct compat_blk_user_trace_setup {
+	char name[32];
+	u16 act_mask;
+	u32 buf_size;
+	u32 buf_nr;
+	compat_u64 start_lba;
+	compat_u64 end_lba;
+	u32 pid;
+};
+#define BLKTRACESETUP32 _IOWR(0x12,115,struct compat_blk_user_trace_setup)
+
+#endif /* CONFIG_COMPAT */
+
 #if defined(CONFIG_BLK_DEV_IO_TRACE)
 extern int blk_trace_ioctl(struct block_device *, unsigned, char __user *);
 extern void blk_trace_shutdown(struct request_queue *);

@@ -287,5 +304,6 @@ static inline void blk_add_trace_remap(s
 #define blk_add_trace_pdu_int(q, what, bio, pdu)	do { } while (0)
 #define blk_add_trace_remap(q, bio, dev, f, t)	do {} while (0)
 #endif /* CONFIG_BLK_DEV_IO_TRACE */
+#endif /* __KERNEL__ */
 
 #endif
fs/compat_ioctl.c +0 -4
Index: linux-2.6/fs/compat_ioctl.c
===================================================================
@@ -2553,10 +2553,6 @@ COMPATIBLE_IOCTL(BLKRRPART)
 COMPATIBLE_IOCTL(BLKFLSBUF)
 COMPATIBLE_IOCTL(BLKSECTSET)
 COMPATIBLE_IOCTL(BLKSSZGET)
-COMPATIBLE_IOCTL(BLKTRACESTART)
-COMPATIBLE_IOCTL(BLKTRACESTOP)
-COMPATIBLE_IOCTL(BLKTRACESETUP)
-COMPATIBLE_IOCTL(BLKTRACETEARDOWN)
 ULONG_IOCTL(BLKRASET)
 ULONG_IOCTL(BLKFRASET)
 #endif
#7 lore raw
Quoted 5 lines · Arnd Bergmann

Jens, I think the best overall solution would be to have a block/compat_ioctl.c file with all the compat handling for block devices moved over from fs/compat_ioctl.c, and done in a nicer way. If you agree, with this approach, I'd volunteer to come up with a patch.

Sometimes I find it hard to stop myself once I have the idea. The patch below moves all block related ioctl conversion out of fs/compat_ioctl.c into the compat_blkdev_ioctl() function.

Is that a direction we should be heading towards? If so, I can do some testing and split this big patch into more logical units for better review.

I also found a few interesting bugs in the process:
* BLKRASET is both ULONG_IOCTL and COMPATIBLE_IOCTL, but this should be entirely harmless
* BLKSECTGET writes 2 bytes normally, our compat_ version writes 4 bytes!
* FDSETPRM32, FDDEFPRM32 and FDGETPRM32 are doing potentially dangerous stuff with kernel pointers. sparse actually warns about this, but from what I could see from floppy.c, the kernel always ignores these pointers. I did not check any other floppy drivers implementing the same calls.

	Arnd <><
block/Makefile +1 -0
Index: linux-2.6/block/Makefile
===================================================================
@@ -11,3 +11,4 @@ obj-$(CONFIG_IOSCHED_DEADLINE)	+= deadli
 obj-$(CONFIG_IOSCHED_CFQ)	+= cfq-iosched.o
 
 obj-$(CONFIG_BLK_DEV_IO_TRACE)	+= blktrace.o
+obj-$(CONFIG_COMPAT)		+= compat_ioctl.o
block/compat_ioctl.c new file +772 -0

772 lines not on this page · raw message

block/ioctl.c +0 -28
Index: linux-2.6/block/ioctl.c
===================================================================
@@ -284,32 +284,4 @@ int blkdev_ioctl(struct inode *inode, st
 
 	return blkdev_driver_ioctl(inode, file, disk, cmd, arg);
 }
-
-/* 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)
-{
-	int ret = -ENOIOCTLCMD;
-#ifdef CONFIG_COMPAT
-	struct block_device *bdev = file->f_path.dentry->d_inode->i_bdev;
-	struct gendisk *disk = bdev->bd_disk;
-
-	switch (cmd) {
-	case BLKTRACESTART:
-	case BLKTRACESTOP:
-	case BLKTRACESETUP32:
-	case BLKTRACETEARDOWN:
-		return blk_trace_ioctl(bdev, cmd, (char __user *) arg);
-	}
-
-	if (disk->fops->compat_ioctl) {
-		lock_kernel();
-		ret = disk->fops->compat_ioctl(file, cmd, arg);
-		unlock_kernel();
-	}
-#endif
-	return ret;
-}
-
 EXPORT_SYMBOL_GPL(blkdev_ioctl);
fs/compat_ioctl.c +0 -675

770 lines not on this page · raw message

author
#8 lore raw
Quoted 7 lines · Arnd Bergmann
Quoted 5 lines · Arnd Bergmann

Jens, I think the best overall solution would be to have a block/compat_ioctl.c file with all the compat handling for block devices moved over from fs/compat_ioctl.c, and done in a nicer way. If you agree, with this approach, I'd volunteer to come up with a patch.

Quoted 7 lines

Sometimes I find it hard to stop myself once I have the idea. The patch below moves all block related ioctl conversion out of fs/compat_ioctl.c into the compat_blkdev_ioctl() function.

Is that a direction we should be heading towards? If so, I can do some testing and split this big patch into more logical units for better review.

This looks a lot better! I don't mind seperating the block bits, when we get the whole bunch in there. Just seemed overly silly and complicated to do it for just one ioctl command. When you are happy with this patch, I'll add it to the pending block stuff for 2.6.24. The blktrace fix isn't a regression so need not go into 2.6.23.

It seems some bits are missing though (like BLKTRACESETUP32), did you send the whole thing?

Quoted 10 lines

I also found a few interesting bugs in the process:
* BLKRASET is both ULONG_IOCTL and COMPATIBLE_IOCTL, but this should be entirely harmless
* BLKSECTGET writes 2 bytes normally, our compat_ version writes 4 bytes!
* FDSETPRM32, FDDEFPRM32 and FDGETPRM32 are doing potentially dangerous stuff with kernel pointers. sparse actually warns about this, but from what I could see from floppy.c, the kernel always ignores these pointers. I did not check any other floppy drivers implementing the same calls.

Always nice to find bugs when doing cleanups ;-)

Signature · 1 line
Jens Axboe
#9 lore raw
Quoted 5 lines · you

This looks a lot better! I don't mind seperating the block bits, when we get the whole bunch in there. Just seemed overly silly and complicated to do it for just one ioctl command. When you are happy with this patch, I'll add it to the pending block stuff for 2.6.24. The blktrace fix isn't a regression so need not go into 2.6.23.

ok.

Quoted 2 lines

It seems some bits are missing though (like BLKTRACESETUP32), did you send the whole thing?

I did the large patch on top of the blktrace patch I sent earlier, so it doesn't apply cleanly. When doing the final version, I'll use a more sensible patch order.

	Arnd <><
author
#10 lore raw
Quoted 7 lines · Arnd Bergmann
Quoted 5 lines · you

This looks a lot better! I don't mind seperating the block bits, when we get the whole bunch in there. Just seemed overly silly and complicated to do it for just one ioctl command. When you are happy with this patch, I'll add it to the pending block stuff for 2.6.24. The blktrace fix isn't a regression so need not go into 2.6.23.

Quoted 8 lines

ok.

Quoted 2 lines

It seems some bits are missing though (like BLKTRACESETUP32), did you send the whole thing?

I did the large patch on top of the blktrace patch I sent earlier, so it doesn't apply cleanly. When doing the final version, I'll use a more sensible patch order.

Ah I see, can you generate one against current git?

Signature · 1 line
Jens Axboe
#11 lore raw

PATCH 1/1 block: move ioctl conversion to compat_blkdev_ioctl

fs/compat_ioctl.c is still a mess and I'd prefer to get rid of it over time, by moving everything to the respective drivers.

A significant portion of our ioctl numbers are common to a number of block drivers, so should not handle them in each driver separately, but rather in the block layer.

In the process, we remove a few indirections from the call graph for compat block ioctls. This patch is still an entirely untested preview and should be split into smaller chunks.

Signed-off-by Arnd Bergmann <[email protected]>

---

 block/Makefile               |    1
 block/blktrace.c             |   54 +-
 block/compat_ioctl.c         |  820 ++++++++++++++++++++++++++++++++++++++
 block/ioctl.c                |   17
 fs/compat_ioctl.c            |  679 -------------------------------
 include/linux/blktrace_api.h |    7
 6 files changed, 861 insertions(+), 717 deletions(-)
Quoted 8 lines · Jens Axboe
Quoted 3 lines · Arnd Bergmann

I did the large patch on top of the blktrace patch I sent earlier, so it doesn't apply cleanly. When doing the final version, I'll use a more sensible patch order.

Ah I see, can you generate one against current git?

block/Makefile +1 -0
Index: linux-2.6/block/Makefile
===================================================================
@@ -11,3 +11,4 @@ obj-$(CONFIG_IOSCHED_DEADLINE)	+= deadli
 obj-$(CONFIG_IOSCHED_CFQ)	+= cfq-iosched.o
 
 obj-$(CONFIG_BLK_DEV_IO_TRACE)	+= blktrace.o
+obj-$(CONFIG_COMPAT)		+= compat_ioctl.o
block/blktrace.c +34 -20

100 lines not on this page · raw message

block/compat_ioctl.c new file +820 -0

820 lines not on this page · raw message

block/ioctl.c +0 -17
Index: linux-2.6/block/ioctl.c
===================================================================
@@ -284,21 +284,4 @@ int blkdev_ioctl(struct inode *inode, st
 
 	return blkdev_driver_ioctl(inode, file, disk, cmd, arg);
 }
-
-/* 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_path.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;
-}
-
 EXPORT_SYMBOL_GPL(blkdev_ioctl);
fs/compat_ioctl.c +0 -679

774 lines not on this page · raw message

include/linux/blktrace_api.h +6 -1
Index: linux-2.6/include/linux/blktrace_api.h
===================================================================
@@ -142,10 +142,14 @@ struct blk_user_trace_setup {
 	u32 pid;
 };
 
+#ifdef __KERNEL__
 #if defined(CONFIG_BLK_DEV_IO_TRACE)
 extern int blk_trace_ioctl(struct block_device *, unsigned, char __user *);
 extern void blk_trace_shutdown(struct request_queue *);
 extern void __blk_add_trace(struct blk_trace *, sector_t, int, int, u32, int, int, void *);
+extern int do_blk_trace_setup(struct request_queue *q, struct block_device *bdev,
+			struct blk_user_trace_setup *buts);
+
 
 /**
  * blk_add_trace_rq - Add a trace for a request oriented action

@@ -286,6 +290,7 @@ static inline void blk_add_trace_remap(s
 #define blk_add_trace_generic(q, rq, rw, what)	do { } while (0)
 #define blk_add_trace_pdu_int(q, what, bio, pdu)	do { } while (0)
 #define blk_add_trace_remap(q, bio, dev, f, t)	do {} while (0)
+#define do_blk_trace_setup(q, bdev, buts)	do {} while (0)
 #endif /* CONFIG_BLK_DEV_IO_TRACE */
-
+#endif /* __KERNEL__ */
 #endif
author
#12 lore raw

Re: [PATCH] block: move ioctl conversion to compat_blkdev_ioctl

Quoted 10 lines · Arnd Bergmann

fs/compat_ioctl.c is still a mess and I'd prefer to get rid of it over time, by moving everything to the respective drivers.

A significant portion of our ioctl numbers are common to a number of block drivers, so should not handle them in each driver separately, but rather in the block layer.

In the process, we remove a few indirections from the call graph for compat block ioctls. This patch is still an entirely untested preview and should be split into smaller chunks.

As before, this looks pretty good. I'll toss it in the tester and see if things still work.

Signature · 1 line
Jens Axboe
#13 lore raw

Re: [PATCH] block: move ioctl conversion to compat_blkdev_ioctl

Quoted 3 lines · Jens Axboe

As before, this looks pretty good. I'll toss it in the tester and see if things still work.

Ok, thanks a lot.

	Arnd <><