On 01. 08. 22 12:13, Takashi Iwai wrote:
On Mon, 01 Aug 2022 10:05:59 +0200, Jaroslav Kysela wrote:
On 28. 07. 22 14:59, Takashi Iwai wrote:
Currently the call of kill_fasync() from an interrupt handler might lead to potential spin deadlocks, as spotted by syzkaller. Unfortunately, it's not so trivial to fix this lock chain as it's involved with the tasklist_lock that is touched in allover places.
As a temporary workaround, this patch provides the way to defer the async signal notification in a work. The new helper functions, snd_fasync_helper() and snd_kill_faync() are replacements for fasync_helper() and kill_fasync(), respectively. In addition, snd_fasync_free() needs to be called at the destructor of the relevant file object.
Signed-off-by: Takashi Iwai tiwai@suse.de
...
+void snd_kill_fasync(struct snd_fasync *fasync, int signal, int poll) +{
- unsigned long flags;
- if (!fasync || !fasync->on)
return;- spin_lock_irqsave(&snd_fasync_lock, flags);
- fasync->signal = signal;
- fasync->poll = poll;
- list_move(&fasync->list, &snd_fasync_list);
- schedule_work(&snd_fasync_work);
- spin_unlock_irqrestore(&snd_fasync_lock, flags);
+}
The schedule_work() may be called outside the spinlock - it calls queue_work_on() / __queue_work() which has already own protection for the concurrent execution.
It can be outside, too, but scheduling earlier reduces the possible unnecessary scheduling. Suppose that a list is added while the work is already running in another CPU. If we call schedule_work() outside this lock, it might be already the time after the work has processed the queued item, and hence it can be a superfluous scheduling call.
It's really a negligible optimization. It would be better to not block other CPUs here to allow insertion of the new event. Also the __queue_work() is a bit complex code, so the call outside the spin lock may be better.
But either code is acceptable for me.
Jaroslav