ieee1394: raw1394: replace BKL by local mutex, make ioctl() and mmap() thread-safe

This removes the last usage of the Big Kernel Lock from the ieee1394 stack, i.e. from raw1394's (unlocked_)ioctl and compat_ioctl. The ioctl()s don't need to take the BKL, but they need to be serialized per struct file *. In particular, accesses to ->iso_state need to be serial. We simply use a blocking mutex for this purpose because libraw1394 does not use O_NONBLOCK. In practice, there is no lock contention anyway because most if not all libraw1394 clients use a libraw1394 handle only in a single thread. mmap() also accesses ->iso_state. Until now this was unprotected against concurrent changes by ioctls. Fix this bug while we are at it. Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

ieee1394: raw1394: replace BKL by local mutex, make ioctl() and mmap() thread-safe
This removes the last usage of the Big Kernel Lock from the ieee1394 stack, i.e. from raw1394's (unlocked_)ioctl and compat_ioctl. The ioctl()s don't need to take the BKL, but they need to be serialized per struct file *. In particular, accesses to ->iso_state need to be serial. We simply use a blocking mutex for this purpose because libraw1394 does not use O_NONBLOCK. In practice, there is no lock contention anyway because most if not all libraw1394 clients use a libraw1394 handle only in a single thread. mmap() also accesses ->iso_state. Until now this was unprotected against concurrent changes by ioctls. Fix this bug while we are at it. Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
Stefan Richter
1 parent ed6ffd0808
Showing 2 changed files with 18 additions and 6 deletions Side-by-side Diff
drivers/ieee1394/raw1394-private.h
drivers/ieee1394/raw1394.c
@@ -22,6 +22,7 @@
 struct file_info {
         struct list_head list;
  
+	struct mutex state_mutex;
         enum { opened, initialized, connected } state;
         unsigned int protocol_version;
  
@@ -34,6 +34,7 @@
 #include <linux/fs.h>
 #include <linux/poll.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/vmalloc.h>
  
  
  
@@ -2541,11 +2542,18 @@
 static int raw1394_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct file_info *fi = file->private_data;
+	int ret;
  
+	mutex_lock(&fi->state_mutex);
+
 	if (fi->iso_state == RAW1394_ISO_INACTIVE)
-		return -EINVAL;
+		ret = -EINVAL;
+	else
+		ret = dma_region_mmap(&fi->iso_handle->data_buf, file, vma);
  
-	return dma_region_mmap(&fi->iso_handle->data_buf, file, vma);
+	mutex_unlock(&fi->state_mutex);
+
+	return ret;
 }
  
 /* ioctl is only used for rawiso operations */
  
  
@@ -2659,10 +2667,12 @@
 static long raw1394_ioctl(struct file *file, unsigned int cmd,
 							unsigned long arg)
 {
+	struct file_info *fi = file->private_data;
 	long ret;
-	lock_kernel();
+
+	mutex_lock(&fi->state_mutex);
 	ret = do_raw1394_ioctl(file, cmd, arg);
-	unlock_kernel();
+	mutex_unlock(&fi->state_mutex);
 	return ret;
 }
  
@@ -2724,7 +2734,7 @@
 	void __user *argp = (void __user *)arg;
 	long err;
  
-	lock_kernel();
+	mutex_lock(&fi->state_mutex);
 	switch (cmd) {
 	/* These requests have same format as long as 'int' has same size. */
 	case RAW1394_IOC_ISO_RECV_INIT:
@@ -2757,7 +2767,7 @@
 		err = -EINVAL;
 		break;
 	}
-	unlock_kernel();
+	mutex_unlock(&fi->state_mutex);
  
 	return err;
 }
@@ -2791,6 +2801,7 @@
 	fi->notification = (u8) RAW1394_NOTIFY_ON;	/* busreset notification */
  
 	INIT_LIST_HEAD(&fi->list);
+	mutex_init(&fi->state_mutex);
 	fi->state = opened;
 	INIT_LIST_HEAD(&fi->req_pending);
 	INIT_LIST_HEAD(&fi->req_complete);
...	...	@@ -22,6 +22,7 @@
22	22	struct file_info {
23	23	struct list_head list;
24	24
	25	+ struct mutex state_mutex;
25	26	enum { opened, initialized, connected } state;
26	27	unsigned int protocol_version;
27	28
...	...	@@ -34,6 +34,7 @@
34	34	#include <linux/fs.h>
35	35	#include <linux/poll.h>
36	36	#include <linux/module.h>
	37	+#include <linux/mutex.h>
37	38	#include <linux/init.h>
38	39	#include <linux/interrupt.h>
39	40	#include <linux/vmalloc.h>
40	41
41	42
42	43
...	...	@@ -2541,11 +2542,18 @@
2541	2542	static int raw1394_mmap(struct file file, struct vm_area_struct vma)
2542	2543	{
2543	2544	struct file_info *fi = file->private_data;
	2545	+ int ret;
2544	2546
	2547	+ mutex_lock(&fi->state_mutex);
	2548	+
2545	2549	if (fi->iso_state == RAW1394_ISO_INACTIVE)
2546		- return -EINVAL;
	2550	+ ret = -EINVAL;
	2551	+ else
	2552	+ ret = dma_region_mmap(&fi->iso_handle->data_buf, file, vma);
2547	2553
2548		- return dma_region_mmap(&fi->iso_handle->data_buf, file, vma);
	2554	+ mutex_unlock(&fi->state_mutex);
	2555	+
	2556	+ return ret;
2549	2557	}
2550	2558
2551	2559	/* ioctl is only used for rawiso operations */
2552	2560
2553	2561
...	...	@@ -2659,10 +2667,12 @@
2659	2667	static long raw1394_ioctl(struct file *file, unsigned int cmd,
2660	2668	unsigned long arg)
2661	2669	{
	2670	+ struct file_info *fi = file->private_data;
2662	2671	long ret;
2663		- lock_kernel();
	2672	+
	2673	+ mutex_lock(&fi->state_mutex);
2664	2674	ret = do_raw1394_ioctl(file, cmd, arg);
2665		- unlock_kernel();
	2675	+ mutex_unlock(&fi->state_mutex);
2666	2676	return ret;
2667	2677	}
2668	2678
...	...	@@ -2724,7 +2734,7 @@
2724	2734	void __user argp = (void __user )arg;
2725	2735	long err;
2726	2736
2727		- lock_kernel();
	2737	+ mutex_lock(&fi->state_mutex);
2728	2738	switch (cmd) {
2729	2739	/* These requests have same format as long as 'int' has same size. */
2730	2740	case RAW1394_IOC_ISO_RECV_INIT:
...	...	@@ -2757,7 +2767,7 @@
2757	2767	err = -EINVAL;
2758	2768	break;
2759	2769	}
2760		- unlock_kernel();
	2770	+ mutex_unlock(&fi->state_mutex);
2761	2771
2762	2772	return err;
2763	2773	}
...	...	@@ -2791,6 +2801,7 @@
2791	2801	fi->notification = (u8) RAW1394_NOTIFY_ON; /* busreset notification */
2792	2802
2793	2803	INIT_LIST_HEAD(&fi->list);
	2804	+ mutex_init(&fi->state_mutex);
2794	2805	fi->state = opened;
2795	2806	INIT_LIST_HEAD(&fi->req_pending);
2796	2807	INIT_LIST_HEAD(&fi->req_complete);