On Mon, 01 Aug 2022 12:43:36 +0200,
Jaroslav Kysela wrote:
>
> 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@xxxxxxx>
> >>
> >> ...
> >>
> >>> +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.
It depends on how often this code path is used. Supposing the rare
use case of this, we don't need to care too much, IMO.
And, if we really want better concurrency, it should be replaced with
RCU :)
> But either code is acceptable for me.
As I've already queued the patches in the original form in the last
week, let's keep it as is.
thanks,
Takashi
