lkml.org 
[lkml]   [2001]   [Sep]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
Subject[PATCH] broken locking in CPiA driver
This patch fixes a locking issue with the CPiA driver, and cleans up the
locking. There is a potential race condition in cpia_pp_detach(). The
REMOVE_FROM_LIST macro uses the BKL to provide protection for the
cam_list. However, the BKL is not held during the whole for loop, only
during the macro.

This patch does several things:
1. rename REMOVE_FROM_LIST and ADD_TO_LIST to cpia_remove_from_list and
cpia_add_to_list, respectively
2. make them "static inline" functions instead of #define macros. (take
a look at this, they probably should be functions)
3. add one static spinlock cam_list_lock_{pp|usb} for each of the pp
and usb drivers to replace BKL
4. do locking around all cam_list references to replace BKL locking
from the macros
5. fix race condition
6. initialize cam_list to NULL in pp driver, just like the usb driver.



--
David C. Hansen
haveblue@us.ibm.com
IBM LTC Base/OS Group
(503)578-4080


--- ../clean/linux/drivers/media/video/cpia_pp.c Fri Mar 2 11:12:10
2001
+++ linux/drivers/media/video/cpia_pp.c Mon Sep 10 16:02:42 2001
@@ -158,6 +158,7 @@
};

static struct cam_data *cam_list;
+static spinlock_t cam_list_lock_pp;

#ifdef _CPIA_DEBUG_
#define DEB_PORT(port) { \
@@ -582,7 +583,9 @@
kfree(cam);
return -ENXIO;
}
- ADD_TO_LIST(cam_list, cpia);
+ spin_lock( &cam_list_lock_pp );
+ cpia_add_to_list(cam_list, cpia);
+ spin_unlock( &cam_list_lock_pp );

return 0;
}
@@ -591,11 +594,12 @@
{
struct cam_data *cpia;

+ spin_lock( &cam_list_lock_pp );
for(cpia = cam_list; cpia != NULL; cpia = cpia->next) {
struct pp_cam_entry *cam = cpia->lowlevel_data;
if (cam && cam->port->number == port->number) {
- REMOVE_FROM_LIST(cpia);
-
+ cpia_remove_from_list(cpia);
+ spin_unlock( &cam_list_lock_pp );
cpia_unregister_camera(cpia);

if(cam->open_count > 0) {
@@ -606,9 +610,10 @@

kfree(cam);
cpia->lowlevel_data = NULL;
- break;
+ return;
}
}
+ spin_unlock( &cam_list_lock_pp );
}

static void cpia_pp_attach (struct parport *port)
@@ -660,6 +665,9 @@
LOG ("unable to register with parport\n");
return -EIO;
}
+
+ cam_list = NULL;
+ spinlock_init( cam_list_lock_pp );

return 0;
}
--- ../clean/linux/drivers/media/video/cpia_usb.c Mon Feb 19 10:18:18
2001
+++ linux/drivers/media/video/cpia_usb.c Mon Sep 10 15:19:19 2001
@@ -104,6 +104,7 @@
};

static struct cam_data *cam_list;
+static spinlock_t cam_list_lock_usb;

static void cpia_usb_complete(struct urb *urb)
{
@@ -536,7 +537,9 @@
goto fail_all;
}

- ADD_TO_LIST(cam_list, cam);
+ spin_lock( &cam_list_lock_usb );
+ cpia_add_to_list(cam_list, cam);
+ spin_unlock( &cam_list_lock_usb );

return cam;

@@ -579,8 +582,10 @@
struct cam_data *cam = (struct cam_data *) ptr;
struct usb_cpia *ucpia = (struct usb_cpia *) cam->lowlevel_data;

- REMOVE_FROM_LIST(cam);
-
+ spin_lock( &cam_list_lock_usb );
+ cpia_remove_from_list(cam);
+ spin_unlock( &cam_list_lock_usb );
+
/* Don't even try to reset the altsetting if we're disconnected */
cpia_usb_free_resources(ucpia, 0);

@@ -620,7 +625,7 @@
static int __init usb_cpia_init(void)
{
cam_list = NULL;
-
+ spin_lock_init(&cam_list_lock_usb);
return usb_register(&cpia_driver);
}

--- ../clean/linux/drivers/media/video/cpia.h Fri Mar 2 11:12:10 2001
+++ linux/drivers/media/video/cpia.h Mon Sep 10 15:24:03 2001
@@ -393,27 +393,24 @@
(p)&0x80?1:0, (p)&0x40?1:0, (p)&0x20?1:0, (p)&0x10?1:0,\
(p)&0x08?1:0, (p)&0x04?1:0, (p)&0x02?1:0, (p)&0x01?1:0);

-#define ADD_TO_LIST(l, drv) \
- {\
- lock_kernel();\
- (drv)->next = l;\
- (drv)->previous = &(l);\
- (l) = drv;\
- unlock_kernel();\
- } while(0)
+static inline void cpia_add_to_list(struct cam_data* l, struct
cam_data* drv)
+{
+ drv->next = l;
+ drv->previous = &l;
+ l = drv;
+}

-#define REMOVE_FROM_LIST(drv) \
- {\
- if ((drv)->previous != NULL) {\
- lock_kernel();\
- if ((drv)->next != NULL)\
- (drv)->next->previous = (drv)->previous;\
- *((drv)->previous) = (drv)->next;\
- (drv)->previous = NULL;\
- (drv)->next = NULL;\
- unlock_kernel();\
- }\
- } while (0)
+
+static inline void cpia_remove_from_list(struct cam_data* drv)
+{
+ if (drv->previous != NULL) {
+ if (drv->next != NULL)
+ drv->next->previous = drv->previous;
+ *(drv->previous) = drv->next;
+ drv->previous = NULL;
+ drv->next = NULL;
+ }
+}


#endif /* __KERNEL__ */
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
\
 
 \ /
  Last update: 2005-03-22 13:03    [W:0.051 / U:0.264 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site