Re: [PATCH] raw1394: Push the BKL down into the driver ioctls

From: Stefan Richter
Date: Fri May 23 2008 - 06:24:52 EST


Alan Cox wrote:
> Actually in this case wrap the function for now.
>
> Signed-off-by: Alan Cox <alan@xxxxxxxxxx>

Can an .unlocked_ioctl() preempt another .unlocked_ioctl() to the very
same instance of struct file?

If not, we can immediately remove lock_kernel() from raw1394.

If yes, we need to serialize do_raw1394_ioctl against itself or come up
with another protection against concurrent fi->iso_state switches before
we can remove lock_kernel(). And if a .write() can preempt another
.write() to the same instance of struct file, raw1394_write() already
has a problem with concurrent fi->state switches.

I have another minor comment below:

> diff --git a/drivers/ieee1394/raw1394.c b/drivers/ieee1394/raw1394.c
> index ec2a0ad..8ec0278 100644
> --- a/drivers/ieee1394/raw1394.c
> +++ b/drivers/ieee1394/raw1394.c
> @@ -2549,8 +2549,8 @@ static int raw1394_mmap(struct file *file, struct vm_area_struct *vma)
> }
>
> /* ioctl is only used for rawiso operations */
> -static int raw1394_ioctl(struct inode *inode, struct file *file,
> - unsigned int cmd, unsigned long arg)
> +static long do_raw1394_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> {
> struct file_info *fi = file->private_data;
> void __user *argp = (void __user *)arg;
> @@ -2656,6 +2656,16 @@ static int raw1394_ioctl(struct inode *inode, struct file *file,
> return -EINVAL;
> }
>
> +static long raw1394_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + long ret;
> + lock_kernel();
> + ret = do_raw1394_ioctl(file, cmd, arg);
> + unlock_kernel();
> + return ret;
> +}
> +
> #ifdef CONFIG_COMPAT
> struct raw1394_iso_packets32 {
> __u32 n_packets;
> @@ -2690,7 +2700,7 @@ static long raw1394_iso_xmit_recv_packets32(struct file *file, unsigned int cmd,
> !copy_from_user(&infos32, &arg->infos, sizeof infos32)) {
> infos = compat_ptr(infos32);
> if (!copy_to_user(&dst->infos, &infos, sizeof infos))
> - err = raw1394_ioctl(NULL, file, cmd, (unsigned long)dst);
> + err = do_raw1394_ioctl(file, cmd, (unsigned long)dst);
> }
> return err;
> }

The same s/raw1394_ioctl/do_raw1394_ioctl/ should be done in
raw1394_compat_ioctl(). But I suppose it doesn't really matter because
lock_kernel() is allowed to nest.

> @@ -2984,7 +2994,7 @@ static const struct file_operations raw1394_fops = {
> .read = raw1394_read,
> .write = raw1394_write,
> .mmap = raw1394_mmap,
> - .ioctl = raw1394_ioctl,
> + .unlocked_ioctl = raw1394_ioctl,
> #ifdef CONFIG_COMPAT
> .compat_ioctl = raw1394_compat_ioctl,
> #endif

--
Stefan Richter
-=====-==--- -=-= =-===
http://arcgraph.de/sr/
--
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/