Re: TODO: drivers/pcmcia/ds.c: ds_read & ds_write. SMP locks are missing fix - TAKE 2

[Yong Chi]

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 [EMAIL PROTECTED]
> > 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 */
 
 /*====================================================================*/