snd_dummy: Fix callout(9) races

[christos]

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

[christos]

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

[markj]

sys/dev/sound/dummy.c

328

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?

[christos]

sys/dev/sound/dummy.c

328

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?

[dev_submerge.ch]

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

328

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()?

[christos]

In D47459#1087837, @dev_submerge.ch wrote:

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.

[christos]

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.