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

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

[Alan Cox]

Actually in this case wrap the function for now.

Signed-off-by: Alan Cox <alan@redhat.com>

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;
 }
@@ -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

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

[Stefan Richter]

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/

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

[Alan Cox]

On Fri, 23 May 2008 12:23:09 +0200
Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> 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?

Yes. And this btw is true even with the old locked ioctl call if you ever
sleep (eg a copy_to/from_user).

> 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.

Quite a few drivers end up with a private mutex and do mutex_lock/unlock
around the ioctl and write paths (and if the write path can be slow using
_trylock and O_NDELAY check when appropriate).

The goal of pushing it down is to enable driver authors to see and to do
the locking at a driver level instead - plus fix lots of bugs where the
BKL "sleep and drop" behaviour isn't anticipated.

> 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.

Agreed.

Alan

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

[Stefan Richter]

>> Alan Cox wrote:
>>> Actually in this case wrap the function for now.

I committed it to linux1394-2.6.git#for-next and will send it to Linus 
the next time I have something to pull for him.  I expect that to be 
shortly after 2.6.26 was released.  Is this early enough for your 
file-ops .ioctl removal plan?
-- 
Stefan Richter
-=====-==--- -=-= =-===
http://arcgraph.de/sr/