lkml.org 
[lkml]   [2000]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: TODO: drivers/pcmcia/ds.c: ds_read & ds_write. SMP locks are missing fix - TAKE 2
Take 2 based on semaphore -)


Jan-Simon Pendry wrote:

> holding a spin lock across a (potential) sleep is a bug - it can
> lead to deadlock.
>
> jan-simon.
>
> Jakub Jelinek wrote:
> >
> > On Thu, Oct 12, 2000 at 11:38:11AM -0400, Yong Chi wrote:
> > > Hopefully this will do for SMP locks. =)
> >
> > Holding a spinlock for this long (especially when you might sleep there in two
> > places (interruptible_sleep_on, put_user)) is basically a bad idea.
> > spinlocks are designed to be holded only for short time.
> > Either protect just a small critical section with a spinlock, or use
> > semaphores.
> >
> > > --- ds.c.bak Wed Oct 11 13:05:16 2000
> > > +++ ds.c Thu Oct 12 11:25:20 2000
> > > @@ -95,6 +95,7 @@
> > > u_int user_magic;
> > > int event_head, event_tail;
> > > event_t event[MAX_EVENTS];
> > > + spinlock_t lock;
> > > struct user_info_t *next;
> > > } user_info_t;
> > >
> > > @@ -567,6 +568,7 @@
> > > user->event_tail = user->event_head = 0;
> > > user->next = s->user;
> > > user->user_magic = USER_MAGIC;
> > > + spin_lock_init(&user->lock);
> > > s->user = user;
> > > file->private_data = user;
> > >
> > > @@ -616,6 +618,7 @@
> > > socket_t i = MINOR(file->f_dentry->d_inode->i_rdev);
> > > socket_info_t *s;
> > > user_info_t *user;
> > > + ssize_t retval=4;
> > >
> > > DEBUG(2, "ds_read(socket %d)\n", i);
> > >
> > > @@ -625,16 +628,23 @@
> > > return -EINVAL;
> > > s = &socket_table[i];
> > > user = file->private_data;
> > > - if (CHECK_USER(user))
> > > - return -EIO;
> > > -
> > > + spin_lock(&user->lock);
> > > + if (CHECK_USER(user)) {
> > > + retval= -EIO;
> > > + goto read_out;
> > > + }
> > > +
> > > if (queue_empty(user)) {
> > > interruptible_sleep_on(&s->queue);
> > > if (signal_pending(current))
> > > - return -EINTR;
> > > + retval= -EINTR;
> > > + goto read_out;
> > > }
> > > put_user(get_queued_event(user), (int *)buf);
> > > - return 4;
> > > +
> > > +read_out:
> > > + spin_unlock(&user->lock);
> > > + return retval;
> > > } /* ds_read */
> > >
> > > /*====================================================================*/
> > > @@ -645,6 +655,7 @@
> > > socket_t i = MINOR(file->f_dentry->d_inode->i_rdev);
> > > socket_info_t *s;
> > > user_info_t *user;
> > > + ssize_t retval=4;
> > >
> > > DEBUG(2, "ds_write(socket %d)\n", i);
> > >
> > > @@ -656,18 +667,25 @@
> > > return -EBADF;
> > > s = &socket_table[i];
> > > user = file->private_data;
> > > - if (CHECK_USER(user))
> > > - return -EIO;
> > > + spin_lock(&user->lock);
> > > + if (CHECK_USER(user)) {
> > > + retval= -EIO;
> > > + goto write_out;
> > > + }
> > >
> > > if (s->req_pending) {
> > > s->req_pending--;
> > > get_user(s->req_result, (int *)buf);
> > > if ((s->req_result != 0) || (s->req_pending == 0))
> > > wake_up_interruptible(&s->request);
> > > - } else
> > > - return -EIO;
> > > + } else {
> > > + retval= -EIO;
> > > + goto write_out;
> > > + }
> > >
> > > - return 4;
> > > +write_out:
> > > + spin_unlock(&user->lock);
> > > + return retval;
> > > } /* ds_write */
> > >
> > > /*====================================================================*/
> >
> > Jakub
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > Please read the FAQ at http://www.tux.org/lkml/
--- ds.c.bak Wed Oct 11 13:05:16 2000
+++ ds.c Thu Oct 12 13:05:28 2000
@@ -95,6 +95,7 @@
u_int user_magic;
int event_head, event_tail;
event_t event[MAX_EVENTS];
+ struct semaphore sem;
struct user_info_t *next;
} user_info_t;

@@ -567,6 +568,7 @@
user->event_tail = user->event_head = 0;
user->next = s->user;
user->user_magic = USER_MAGIC;
+ init_MUTEX(&user->sem);
s->user = user;
file->private_data = user;

@@ -616,6 +618,7 @@
socket_t i = MINOR(file->f_dentry->d_inode->i_rdev);
socket_info_t *s;
user_info_t *user;
+ ssize_t retval=4;

DEBUG(2, "ds_read(socket %d)\n", i);

@@ -625,16 +628,26 @@
return -EINVAL;
s = &socket_table[i];
user = file->private_data;
- if (CHECK_USER(user))
- return -EIO;
-
+ down_interruptible(&user->sem);
+ if (signal_pending(current))
+ return -EINTR;
+
+ if (CHECK_USER(user)) {
+ retval= -EIO;
+ goto read_out;
+ }
+
if (queue_empty(user)) {
interruptible_sleep_on(&s->queue);
if (signal_pending(current))
- return -EINTR;
+ retval= -EINTR;
+ goto read_out;
}
put_user(get_queued_event(user), (int *)buf);
- return 4;
+
+read_out:
+ up(&user->sem);
+ return retval;
} /* ds_read */

/*====================================================================*/
@@ -645,6 +658,7 @@
socket_t i = MINOR(file->f_dentry->d_inode->i_rdev);
socket_info_t *s;
user_info_t *user;
+ ssize_t retval=4;

DEBUG(2, "ds_write(socket %d)\n", i);

@@ -656,18 +670,28 @@
return -EBADF;
s = &socket_table[i];
user = file->private_data;
- if (CHECK_USER(user))
- return -EIO;
+ down_interruptible(&user->sem);
+ if (signal_pending(current))
+ return -EINTR;
+
+ if (CHECK_USER(user)) {
+ retval= -EIO;
+ goto write_out;
+ }

if (s->req_pending) {
s->req_pending--;
get_user(s->req_result, (int *)buf);
if ((s->req_result != 0) || (s->req_pending == 0))
wake_up_interruptible(&s->request);
- } else
- return -EIO;
+ } else {
+ retval= -EIO;
+ goto write_out;
+ }

- return 4;
+write_out:
+ up(&user->sem);
+ return retval;
} /* ds_write */

/*====================================================================*/
\
 
 \ /
  Last update: 2005-03-22 12:41    [W:0.086 / U:0.224 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site