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

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

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

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/