Hi,
Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:
> On Fri, 2 Sep 2016, Felipe Balbi wrote:
>
>> >> I just noticed that the kerneldoc for wake_up_process() says that the
>> >> caller should assume a write memory barrier if and only if any tasks
>> >> are woken up. So if the task is already running, there is no barrier.
>> >>
>> >> This means that in f_mass_storage's wakeup_thread(), there may be a
>> >> race involving wake_up_process() reading the thread's state and the
>> >> write to common->thread_wakeup_needed.
>> >>
>> >> wakeup_thread():
>> >> common->thread_wakeup_needed = 1;
>> >> wake_up_process(common->thread_task);
>> >> /* reads the thread's state */
>> >>
>> >> sleep_thread():
>> >> set_current_state(TASK_INTERRUPTIBLE);
>> >> if (common->thread_wakeup_needed)
>> >> break;
>> >>
>> >> Now, set_current_state() implicitly has a memory barrier at the end.
>> >> But since wake_up_process() doesn't always add a barrier the start,
>> >> we may need to do this explicitly.
>> >>
>> >> Without that barrier, the CPU might execute the read in
>> >> wake_up_process() before setting thread_wakeup_needed to 1. Then
>> >> sleep_thread() would slip through the crack, and the thread wouldn't
>> >> run.
>> >>
>> >> You can try adding smp_mb() in wakeup_thread(), just before the call to
>> >> wake_up_process().
>> >
>> > heh, it would've taken me weeks to figure this out :-) thanks
>> >
>> > BTW, what are those barriers protecting in g_mass_storage? Its own
>> > internal flags or the task state?
>
> I'm not sure which barriers you're referring to; there's a number of
> them. But I think they are all intended to protect the driver's
> internal variables, not the task state.
>
>> > If it's protecting own flags, then isn't the following enough?
>> >
>> > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
>> > index 8f3659b65f53..e3b03decdb6b 100644
>> > --- a/drivers/usb/gadget/function/f_mass_storage.c
>> > +++ b/drivers/usb/gadget/function/f_mass_storage.c
>> > @@ -395,7 +395,7 @@ static int fsg_set_halt(struct fsg_dev *fsg, struct usb_ep *ep)
>> > /* Caller must hold fsg->lock */
>> > static void wakeup_thread(struct fsg_common *common)
>> > {
>> > - smp_wmb(); /* ensure the write of bh->state is complete */
>> > + smp_mb(); /* ensure the write of bh->state is complete */
>> > /* Tell the main thread that something has happened */
>> > common->thread_wakeup_needed = 1;
>> > if (common->thread_task)
>> >
>> > I'm probably missing some detail here :-s
>>
>> oh no, you're trying to make sure the write to
>> common->thread_wakeup_needed is seen by sleep_thread(), right?
>
> Exactly.
>
>> In that case, couldn't we conditionally add smp_mb() based on the result
>> of wake_up_process()? IOW:
>>
>> if (!wake_up_process(common->thread_task))
>> smp_mb();
>>
>> would this still work?
>
> No, that would be too late. The barrier needs to go between the write
> to common->thead_wakeup_needed and the call to wake_up_process().
alright, I'll test this out and keep it running over the weekend.
> Otherwise what I wrote above could happen: The CPU could read the task
> state (in wake_up_process()) before it writes out the new value for
> thread_wakeup_needed, and sleep_thread() could run on another CPU
> during that tiny window. wake_up_process() would see that the thread's
> state was still TASK_RUNNING, so it wouldn't do anything, and
> sleep_thread() would see that thread_wakeup_needed was still 0, so it
> would put the thread to sleep.
Oh I see.
> Now, I can't tell if this is really what caused your problem. Still,
> it seems like a necessary thing to do. Maybe I'll check this with Paul
> McKenney.
>
> (IMO, the default version of wake_up_process() should have this memory
> barrier built-in. Otherwise races like this are too hard to track
> down. There could be a different version without the barrier, say
> __wake_up_process(), for places where it's known to be unnecessary.
> That's how set_current_state() and __set_current_state() are
> implemented.)
makes sense to me. On Monday I'll let you know about results.
--
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
