Re: [PATCH] es1371 race fixes

[Alan Cox]

>   o es1371_mmap used to use lock_kernel to do some synchronistation,
>   this is superceeded by s->sem.
>   o remap_page_range (used in es1371_mmap) needs the mm semaphore as
> stated by a comment and the code.  I have found _NO_ driver in the
> tree so far that does this locking right...

I think they rely on the lock_kernel stuff - which Id prefer not to take out
to be honest. There is just a little too much vm related lock_kernel stuff
left to do that
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] es1371 race fixes

[Christoph Hellwig]

Hi all,

this are the sound locking fixes for es1371.  While it is inspired by the
trident fixes it contains some changes over it:

  o es1371_mmap used to use lock_kernel to do some synchronistation,
this is superceeded by s->sem.
  o remap_page_range (used in es1371_mmap) needs the mm semaphore as
stated by a comment and the code.  I have found _NO_ driver in the
tree so far that does this locking right...

Looks like we have another round of races...

Christoph

-- 
Of course it doesn't work. We've performed a software upgrade.

--- linux-2.4.5-ac6/drivers/sound/es1371.c  Sat Jun  2 14:35:08 2001
+++ linux/drivers/sound/es1371.cSat Jun  2 17:35:11 2001
@@ -417,8 +417,16 @@
unsigned sctrl;
unsigned dac1rate, dac2rate, adcrate;
 
+   /*
+* Lock order (high->low):
+*   - lock hardware lock
+*   - open_sem guards opens
+*   - sem  guard dmabuf, write re-entry etc
+*/
spinlock_t lock;
struct semaphore open_sem;
+   struct semaphore sem;
+
mode_t open_mode;
wait_queue_head_t open_wait;
 
@@ -1336,21 +1344,27 @@
 {
struct es1371_state *s = (struct es1371_state *)file->private_data;
DECLARE_WAITQUEUE(wait, current);
-   ssize_t ret;
+   ssize_t ret = 0;
unsigned long flags;
unsigned swptr;
int cnt;
 
VALIDATE_STATE(s);
+
if (ppos != &file->f_pos)
return -ESPIPE;
if (s->dma_adc.mapped)
return -ENXIO;
-   if (!s->dma_adc.ready && (ret = prog_dmabuf_adc(s)))
-   return ret;
if (!access_ok(VERIFY_WRITE, buffer, count))
return -EFAULT;
-   ret = 0;
+
+   /*
+* Guard against concurrent dmabuf programming.
+*/
+   down(&s->sem);
+   if (!s->dma_adc.ready && (ret = prog_dmabuf_adc(s)))
+   goto up_and_out;
+   
add_wait_queue(&s->dma_adc.wait, &wait);
while (count > 0) {
spin_lock_irqsave(&s->lock, flags);
@@ -1371,12 +1385,14 @@
ret = -EAGAIN;
break;
}
+   up(&s->sem);
schedule();
if (signal_pending(current)) {
if (!ret)
ret = -ERESTARTSYS;
-   break;
+   goto out;
}
+   down(&s->sem);
continue;
}
if (copy_to_user(buffer, s->dma_adc.rawbuf + swptr, cnt)) {
@@ -1395,29 +1411,45 @@
if (s->dma_adc.enabled)
start_adc(s);
}
+   up(&s->sem);
+
+out:
remove_wait_queue(&s->dma_adc.wait, &wait);
set_current_state(TASK_RUNNING);
return ret;
+up_and_out:
+   up(&s->sem);
+   return ret;
 }
 
 static ssize_t es1371_write(struct file *file, const char *buffer, size_t count, 
loff_t *ppos)
 {
struct es1371_state *s = (struct es1371_state *)file->private_data;
DECLARE_WAITQUEUE(wait, current);
-   ssize_t ret;
+   ssize_t ret = -ENXIO;
unsigned long flags;
unsigned swptr;
int cnt;
 
VALIDATE_STATE(s);
+
if (ppos != &file->f_pos)
return -ESPIPE;
+
+   /*
+* Guard against an mmap or ioctl while writing
+*/
+   down(&s->sem);
+   
if (s->dma_dac2.mapped)
-   return -ENXIO;
+   goto up_and_out;
if (!s->dma_dac2.ready && (ret = prog_dmabuf_dac2(s)))
-   return ret;
-   if (!access_ok(VERIFY_READ, buffer, count))
-   return -EFAULT;
+   goto up_and_out;
+   if (!access_ok(VERIFY_READ, buffer, count)) {
+   ret = -EFAULT;
+   goto up_and_out;
+   }
+
ret = 0;
add_wait_queue(&s->dma_dac2.wait, &wait);
while (count > 0) {
@@ -1443,12 +1475,14 @@
ret = -EAGAIN;
break;
}
+   up(&s->sem);
schedule();
if (signal_pending(current)) {
if (!ret)
ret = -ERESTARTSYS;
-   break;
+   goto out;
}
+   down(&s->sem);
continue;
}
if (copy_from_user(s->dma_dac2.rawbuf + swptr, buffer, cnt)) {
@@ -1468,9 +1502,15 @@
if (s->dma_dac2.enabled)
start_dac2(s);
}
+   up(&s->sem);
+
+out:
remove_wait_queue(&s->dma_dac2.wait, &wait);