Possible race in sound/oss/forte.c ?

Possible race in sound/oss/forte.c ?

[John M. King]

I know the OSS drivers are deprecated, but I'm trying to figure this out
for my own understanding.

Here's code from sound/oss/forte.c, in the write system call handler.  A
test has already been performed (under the protection of the lock) and
the driver has decided to sleep.

    add_wait_queue (&channel->wait, &wait);

    for (;;) {
        spin_unlock_irqrestore (&chip->lock, flags);

        set_current_state (TASK_INTERRUPTIBLE);
        schedule();

        spin_lock_irqsave (&chip->lock, flags);

        if (channel->frag_num - channel->filled_frags)
            break;
    }

    remove_wait_queue (&channel->wait, &wait);
    set_current_state (TASK_RUNNING);

The driver's interrupt handler calls wake_up_all().  What if an
interrupt occurs just after the spin_unlock_irqrestore() but before
setting TASK_INTERRUPTIBLE (and the interrupt handler does stuff that
causes the tested conditional to be true as well)?  The interrupt calls
wake_up_all(), but then when control returns here, the process will mark
itself TASK_INTERRUPTIBLE right away and sleep, effectively missing the
wake_up_all().

Is this a race condition?  If not, can someone point out the error(s) in
my reasoning?  Please CC me as I'm not subscribed to the list.

Thanks,
John

Re: Possible race in sound/oss/forte.c ?

[Nish Aravamudan]

On 8/11/05, John M. King <jmking1@uiuc.edu> wrote:
> I know the OSS drivers are deprecated, but I'm trying to figure this out
> for my own understanding.
> 
> Here's code from sound/oss/forte.c, in the write system call handler.  A
> test has already been performed (under the protection of the lock) and
> the driver has decided to sleep.
> 
>     add_wait_queue (&channel->wait, &wait);
> 
>     for (;;) {
>         spin_unlock_irqrestore (&chip->lock, flags);
> 
>         set_current_state (TASK_INTERRUPTIBLE);
>         schedule();
> 
>         spin_lock_irqsave (&chip->lock, flags);
> 
>         if (channel->frag_num - channel->filled_frags)
>             break;
>     }
> 
>     remove_wait_queue (&channel->wait, &wait);
>     set_current_state (TASK_RUNNING);
> 
> The driver's interrupt handler calls wake_up_all().  What if an
> interrupt occurs just after the spin_unlock_irqrestore() but before
> setting TASK_INTERRUPTIBLE (and the interrupt handler does stuff that
> causes the tested conditional to be true as well)?  The interrupt calls
> wake_up_all(), but then when control returns here, the process will mark
> itself TASK_INTERRUPTIBLE right away and sleep, effectively missing the
> wake_up_all().
> 
> Is this a race condition?  If not, can someone point out the error(s) in
> my reasoning?  Please CC me as I'm not subscribed to the list.

This is broken code. You are supposed to set the state before adding
oneself to the wait-queue, if you are going to unconditionally sleep,
I believe.

So, the loop probably should be:

prepare_to_wait(&channel->wait, &wait, TASK_INTERRUPTIBLE);

for (;;) {
         spin_unlock_irqrestore (&chip->lock, flags);
 
         set_current_state (TASK_INTERRUPTIBLE); // redundant in the
first iteration
         schedule();
 
         spin_lock_irqsave (&chip->lock, flags);
 
         if (channel->frag_num - channel->filled_frags)
             break;
}
 
finish_wait(&channel->wait, &wait);

Thanks,
Nish