Re: [PATCH v2 1/4] ALSA: core: Add async signal helpers

From: Jaroslav Kysela <perex@perex.cz>
To: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org
Subject: Re: [PATCH v2 1/4] ALSA: core: Add async signal helpers
Date: Mon, 1 Aug 2022 12:43:36 +0200	[thread overview]
Message-ID: <408af362-0839-e4f0-8dea-15103b269836@perex.cz> (raw)
In-Reply-To: <871qu0nuei.wl-tiwai@suse.de>

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

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.