On Tuesday 02 October 2007, Jens Axboe wrote:
>
> 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 :(.
> diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
> index 5a5b711..b18b9cc 100644
> --- a/fs/compat_ioctl.c
> +++ b/fs/compat_ioctl.c
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.
> @@ -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));
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.
> +#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.
> +{
> + 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;
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 <><
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
