raw1394: Push the BKL down into the driver ioctls

PATCH In review Alan Cox 2 participants 4 messages May 22, 2008 – May 23, 2008

lore

Share & tools

lore

Alan Cox

author

2008-05-22 21:46 UTC #0 lore raw

PATCH 1/1 raw1394: Push the BKL down into the driver ioctls

Actually in this case wrap the function for now.

Signed-off-by Alan Cox <[email protected]>

drivers/ieee1394/raw1394.c +14 -4

index ec2a0ad..8ec0278 100644
@@ -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

Stefan Richter

2008-05-23 10:24 UTC #1 lore raw

Quoted 3 lines · Alan Cox

Actually in this case wrap the function for now.
Signed-off-by Alan Cox <[email protected]>

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:

Quoted 33 lines

drivers/ieee1394/raw1394.c +12 -2

index ec2a0ad..8ec0278 100644
@@ -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;

drivers/ieee1394/raw1394.c static long raw1394_iso_xmit_recv_packets32(struct file *file, unsigned int cmd, · line 2706

Quoted 8 lines

drivers/ieee1394/raw1394.c +1 -1

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

Quoted 9 lines

drivers/ieee1394/raw1394.c +1 -1

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

Signature · 3 lines

Stefan Richter
-=====-==--- -=-= =-===
http://arcgraph.de/sr/

Alan Cox

author

2008-05-23 11:10 UTC #2 lore raw

Quoted 7 lines · On Fri, 23 May 2008 12:23:09 +0200 Stefan Richter <[email protected]>…

Quoted 3 lines · Alan Cox
Actually in this case wrap the function for now.
Signed-off-by Alan Cox <[email protected]>
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).

Quoted 5 lines

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.

Quoted 3 lines

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

Stefan Richter

2008-05-23 15:39 UTC #3 lore raw

Quoted 2 lines

Quoted 2 lines
Quoted 1 line · Alan Cox
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?

Signature · 3 lines

Stefan Richter
-=====-==--- -=-= =-===
http://arcgraph.de/sr/