Page MenuHomeFreeBSD

snd_dummy: Fix callout(9) races
ClosedPublic

Authored by christos on Tue, Nov 5, 9:27 PM.
Tags
None
Referenced Files
F104262692: D47459.id146970.diff
Thu, Dec 5, 1:05 PM
Unknown Object (File)
Tue, Dec 3, 1:30 PM
Unknown Object (File)
Fri, Nov 29, 12:00 AM
Unknown Object (File)
Wed, Nov 27, 5:48 PM
Unknown Object (File)
Tue, Nov 26, 5:26 PM
Unknown Object (File)
Mon, Nov 25, 11:00 PM
Unknown Object (File)
Sat, Nov 23, 10:12 AM
Unknown Object (File)
Wed, Nov 20, 1:07 AM
Subscribers

Details

Summary

Use callout_init_mtx(9) to associate the callback with the driver's
lock. Also make sure the callout is stopped properly during detach.

While here, get rid of some unnecessary if statements in
dummy_chan_trigger().

Sponsored by: The FreeBSD Foundation
MFC after: 2 days

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This revision is now accepted and ready to land.Sat, Nov 9, 7:40 PM

Fix one more race by moving pcm_unregister() before callout_stop() and
callout_drain().

This revision now requires review to proceed.Wed, Nov 20, 7:16 PM
sys/dev/sound/dummy.c
352

Now suppose that the callout runs right after the pcm_unregister() call. Will it do anything unsafe? That is, does it assume that pcm_unregister has not yet been called?

sys/dev/sound/dummy.c
352

dummy_chan_io() assumes that the channels are not freed, so if it somehow runs after pcm_unregister(), we'll get a user-after-free in the channel loop. However, pcm_unregister() calls chn_abort() (and callout_stop() as a result) on all channels before freeing them (so starting the callout from within pcm_unregister() shouldn't be a concern), and because we've also cleared SD_F_REGISTERED at this point, I don't see how we could trigger the channels with PCMTRIG_START (i.e. call chn_start()) after pcm_unregister() in order to re-schedule the callout.

Whereas, if pcm_unregister() is called after callout_drain(), we do indeed run the risk of re-scheduling the callout, since we might end up calling chn_start() in the time window between callout_drain() and pcm_unregister(). IIRC that's a scenario I managed to reproduce with stress2, for example.

Am I missing something?

On a related note, shouldn't dummy_chan_io() only reschedule itself when one of the channels is running? Whereas we indiscriminately stop the callout for both channels if one of them is stopped, even if the other would be still running? Or maybe I have a misconception about the callout handling.

sys/dev/sound/dummy.c
352

pcm_unregister() should prevent the channels from starting again. But I'm trying to wrap my head around whether chn_abort() executing callout_stop() in dummy_chan_trigger() is actually guaranteed to stop the rescheduling of dummy_chan_io(). We don't check the return value of callout_stop(), and dummy_chan_io() might reschedule itself when it is running at the same time?

Also, shouldn't a callout_drain() be enough here? Why the additional callout_stop()?

On a related note, shouldn't dummy_chan_io() only reschedule itself when one of the channels is running? Whereas we indiscriminately stop the callout for both channels if one of them is stopped, even if the other would be still running? Or maybe I have a misconception about the callout handling.

This is correct. I will upload the fix soon.

Do not reschedule callout if no channels are running.

Also get rid of the callout_stop() in pcm_unregister(). I had added this after
a discussion on IRC, but it turns out the use for the lock -> callout_stop() ->
unlock sequence is needed when there is some hardware shutdown we need to take
care of, so in our case just a callout_drain() is enough.

This revision is now accepted and ready to land.Mon, Nov 25, 8:26 PM
This revision was automatically updated to reflect the committed changes.