[PATCH v2 0/4] ALSA: Defer async signal handling

[PATCH v2 0/4] ALSA: Defer async signal handling

[Takashi Iwai]

Hi,

this is a revised patch series for another attempt to address the
potential deadlocks via kill_fasync() calls that have been reported by
syzbot for long time.  Only a missing linux/fs.h inclusion was added
from v1 series[1].

Instead of the previous series to drop the async handler[2], this
tries to defer the kill_fasync() call.  A few new common helpers are
introduced at first, then the actual usages are replaced in the
following patches.

The patches passed the quick tests with alsa-lib test cases.


Takashi

[1] https://lore.kernel.org/r/20220726153420.3403-1-tiwai@suse.de
[2] https://lore.kernel.org/r/20220717070549.5993-1-tiwai@suse.de

===

Takashi Iwai (4):
  ALSA: core: Add async signal helpers
  ALSA: timer: Use deferred fasync helper
  ALSA: pcm: Use deferred fasync helper
  ALSA: control: Use deferred fasync helper

 include/sound/control.h |  2 +-
 include/sound/core.h    |  8 ++++
 include/sound/pcm.h     |  2 +-
 sound/core/control.c    |  7 +--
 sound/core/misc.c       | 94 +++++++++++++++++++++++++++++++++++++++++
 sound/core/pcm.c        |  1 +
 sound/core/pcm_lib.c    |  2 +-
 sound/core/pcm_native.c |  2 +-
 sound/core/timer.c      | 11 ++---
 9 files changed, 117 insertions(+), 12 deletions(-)

-- 
2.35.3

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

[Takashi Iwai]

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>
---
 include/sound/core.h |  8 ++++
 sound/core/misc.c    | 94 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+)

diff --git a/include/sound/core.h b/include/sound/core.h
index dd28de2343b8..4365c35d038b 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -507,4 +507,12 @@ snd_pci_quirk_lookup_id(u16 vendor, u16 device,
 }
 #endif
 
+/* async signal helpers */
+struct snd_fasync;
+
+int snd_fasync_helper(int fd, struct file *file, int on,
+		      struct snd_fasync **fasyncp);
+void snd_kill_fasync(struct snd_fasync *fasync, int signal, int poll);
+void snd_fasync_free(struct snd_fasync *fasync);
+
 #endif /* __SOUND_CORE_H */
diff --git a/sound/core/misc.c b/sound/core/misc.c
index 50e4aaa6270d..d32a19976a2b 100644
--- a/sound/core/misc.c
+++ b/sound/core/misc.c
@@ -10,6 +10,7 @@
 #include <linux/time.h>
 #include <linux/slab.h>
 #include <linux/ioport.h>
+#include <linux/fs.h>
 #include <sound/core.h>
 
 #ifdef CONFIG_SND_DEBUG
@@ -145,3 +146,96 @@ snd_pci_quirk_lookup(struct pci_dev *pci, const struct snd_pci_quirk *list)
 }
 EXPORT_SYMBOL(snd_pci_quirk_lookup);
 #endif
+
+/*
+ * Deferred async signal helpers
+ *
+ * Below are a few helper functions to wrap the async signal handling
+ * in the deferred work.  The main purpose is to avoid the messy deadlock
+ * around tasklist_lock and co at the kill_fasync() invocation.
+ * fasync_helper() and kill_fasync() are replaced with snd_fasync_helper()
+ * and snd_kill_fasync(), respectively.  In addition, snd_fasync_free() has
+ * to be called at releasing the relevant file object.
+ */
+struct snd_fasync {
+	struct fasync_struct *fasync;
+	int signal;
+	int poll;
+	int on;
+	struct list_head list;
+};
+
+static DEFINE_SPINLOCK(snd_fasync_lock);
+static LIST_HEAD(snd_fasync_list);
+
+static void snd_fasync_work_fn(struct work_struct *work)
+{
+	struct snd_fasync *fasync;
+
+	spin_lock_irq(&snd_fasync_lock);
+	while (!list_empty(&snd_fasync_list)) {
+		fasync = list_first_entry(&snd_fasync_list, struct snd_fasync, list);
+		list_del_init(&fasync->list);
+		spin_unlock_irq(&snd_fasync_lock);
+		if (fasync->on)
+			kill_fasync(&fasync->fasync, fasync->signal, fasync->poll);
+		spin_lock_irq(&snd_fasync_lock);
+	}
+	spin_unlock_irq(&snd_fasync_lock);
+}
+
+static DECLARE_WORK(snd_fasync_work, snd_fasync_work_fn);
+
+int snd_fasync_helper(int fd, struct file *file, int on,
+		      struct snd_fasync **fasyncp)
+{
+	struct snd_fasync *fasync = NULL;
+
+	if (on) {
+		fasync = kzalloc(sizeof(*fasync), GFP_KERNEL);
+		if (!fasync)
+			return -ENOMEM;
+		INIT_LIST_HEAD(&fasync->list);
+	}
+
+	spin_lock_irq(&snd_fasync_lock);
+	if (*fasyncp) {
+		kfree(fasync);
+		fasync = *fasyncp;
+	} else {
+		if (!fasync) {
+			spin_unlock_irq(&snd_fasync_lock);
+			return 0;
+		}
+		*fasyncp = fasync;
+	}
+	fasync->on = on;
+	spin_unlock_irq(&snd_fasync_lock);
+	return fasync_helper(fd, file, on, &fasync->fasync);
+}
+EXPORT_SYMBOL_GPL(snd_fasync_helper);
+
+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);
+}
+EXPORT_SYMBOL_GPL(snd_kill_fasync);
+
+void snd_fasync_free(struct snd_fasync *fasync)
+{
+	if (!fasync)
+		return;
+	fasync->on = 0;
+	flush_work(&snd_fasync_work);
+	kfree(fasync);
+}
+EXPORT_SYMBOL_GPL(snd_fasync_free);
-- 
2.35.3

[PATCH v2 2/4] ALSA: timer: Use deferred fasync helper

[Takashi Iwai]

For avoiding the potential deadlock via kill_fasync() call, use the
new fasync helpers to defer the invocation from PCI API.  Note that
it's merely a workaround.

Reported-by: syzbot+1ee0910eca9c94f71f25@syzkaller.appspotmail.com
Reported-by: syzbot+49b10793b867871ee26f@syzkaller.appspotmail.com
Reported-by: syzbot+8285e973a41b5aa68902@syzkaller.appspotmail.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/timer.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/sound/core/timer.c b/sound/core/timer.c
index b3214baa8919..e08a37c23add 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -83,7 +83,7 @@ struct snd_timer_user {
 	unsigned int filter;
 	struct timespec64 tstamp;		/* trigger tstamp */
 	wait_queue_head_t qchange_sleep;
-	struct fasync_struct *fasync;
+	struct snd_fasync *fasync;
 	struct mutex ioctl_lock;
 };
 
@@ -1345,7 +1345,7 @@ static void snd_timer_user_interrupt(struct snd_timer_instance *timeri,
 	}
       __wake:
 	spin_unlock(&tu->qlock);
-	kill_fasync(&tu->fasync, SIGIO, POLL_IN);
+	snd_kill_fasync(tu->fasync, SIGIO, POLL_IN);
 	wake_up(&tu->qchange_sleep);
 }
 
@@ -1383,7 +1383,7 @@ static void snd_timer_user_ccallback(struct snd_timer_instance *timeri,
 	spin_lock_irqsave(&tu->qlock, flags);
 	snd_timer_user_append_to_tqueue(tu, &r1);
 	spin_unlock_irqrestore(&tu->qlock, flags);
-	kill_fasync(&tu->fasync, SIGIO, POLL_IN);
+	snd_kill_fasync(tu->fasync, SIGIO, POLL_IN);
 	wake_up(&tu->qchange_sleep);
 }
 
@@ -1453,7 +1453,7 @@ static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri,
 	spin_unlock(&tu->qlock);
 	if (append == 0)
 		return;
-	kill_fasync(&tu->fasync, SIGIO, POLL_IN);
+	snd_kill_fasync(tu->fasync, SIGIO, POLL_IN);
 	wake_up(&tu->qchange_sleep);
 }
 
@@ -1521,6 +1521,7 @@ static int snd_timer_user_release(struct inode *inode, struct file *file)
 			snd_timer_instance_free(tu->timeri);
 		}
 		mutex_unlock(&tu->ioctl_lock);
+		snd_fasync_free(tu->fasync);
 		kfree(tu->queue);
 		kfree(tu->tqueue);
 		kfree(tu);
@@ -2135,7 +2136,7 @@ static int snd_timer_user_fasync(int fd, struct file * file, int on)
 	struct snd_timer_user *tu;
 
 	tu = file->private_data;
-	return fasync_helper(fd, file, on, &tu->fasync);
+	return snd_fasync_helper(fd, file, on, &tu->fasync);
 }
 
 static ssize_t snd_timer_user_read(struct file *file, char __user *buffer,
-- 
2.35.3

[PATCH v2 3/4] ALSA: pcm: Use deferred fasync helper

[Takashi Iwai]

For avoiding the potential deadlock via kill_fasync() call, use the
new fasync helpers to defer the invocation from timer API.  Note that
it's merely a workaround.

Reported-by: syzbot+8285e973a41b5aa68902@syzkaller.appspotmail.com
Reported-by: syzbot+669c9abf11a6a011dd09@syzkaller.appspotmail.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/pcm.h     | 2 +-
 sound/core/pcm.c        | 1 +
 sound/core/pcm_lib.c    | 2 +-
 sound/core/pcm_native.c | 2 +-
 4 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 2d03c10f6a36..8c48a5bce88c 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -399,7 +399,7 @@ struct snd_pcm_runtime {
 	snd_pcm_uframes_t twake; 	/* do transfer (!poll) wakeup if non-zero */
 	wait_queue_head_t sleep;	/* poll sleep */
 	wait_queue_head_t tsleep;	/* transfer sleep */
-	struct fasync_struct *fasync;
+	struct snd_fasync *fasync;
 	bool stop_operating;		/* sync_stop will be called */
 	struct mutex buffer_mutex;	/* protect for buffer changes */
 	atomic_t buffer_accessing;	/* >0: in r/w operation, <0: blocked */
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index 03fc5fa5813e..2ac742035310 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -1007,6 +1007,7 @@ void snd_pcm_detach_substream(struct snd_pcm_substream *substream)
 		substream->runtime = NULL;
 	}
 	mutex_destroy(&runtime->buffer_mutex);
+	snd_fasync_free(runtime->fasync);
 	kfree(runtime);
 	put_pid(substream->pid);
 	substream->pid = NULL;
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 1fc7c50ffa62..40751e5aff09 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -1822,7 +1822,7 @@ void snd_pcm_period_elapsed_under_stream_lock(struct snd_pcm_substream *substrea
 		snd_timer_interrupt(substream->timer, 1);
 #endif
  _end:
-	kill_fasync(&runtime->fasync, SIGIO, POLL_IN);
+	snd_kill_fasync(runtime->fasync, SIGIO, POLL_IN);
 }
 EXPORT_SYMBOL(snd_pcm_period_elapsed_under_stream_lock);
 
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index aa0453e51595..ad0541e9e888 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3951,7 +3951,7 @@ static int snd_pcm_fasync(int fd, struct file * file, int on)
 	runtime = substream->runtime;
 	if (runtime->status->state == SNDRV_PCM_STATE_DISCONNECTED)
 		return -EBADFD;
-	return fasync_helper(fd, file, on, &runtime->fasync);
+	return snd_fasync_helper(fd, file, on, &runtime->fasync);
 }
 
 /*
-- 
2.35.3

[PATCH v2 4/4] ALSA: control: Use deferred fasync helper

[Takashi Iwai]

For avoiding the potential deadlock via kill_fasync() call, use the
new fasync helpers to defer the invocation from the control API.  Note
that it's merely a workaround.

Another note: although we haven't received reports about the deadlock
with the control API, the deadlock is still potentially possible, and
it's better to align the behavior with other core APIs (PCM and
timer); so let's move altogether.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/control.h | 2 +-
 sound/core/control.c    | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/sound/control.h b/include/sound/control.h
index fcd3cce673ec..eae443ba79ba 100644
--- a/include/sound/control.h
+++ b/include/sound/control.h
@@ -109,7 +109,7 @@ struct snd_ctl_file {
 	int preferred_subdevice[SND_CTL_SUBDEV_ITEMS];
 	wait_queue_head_t change_sleep;
 	spinlock_t read_lock;
-	struct fasync_struct *fasync;
+	struct snd_fasync *fasync;
 	int subscribed;			/* read interface is activated */
 	struct list_head events;	/* waiting events for read */
 };
diff --git a/sound/core/control.c b/sound/core/control.c
index 4dba3a342458..f3e893715369 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -127,6 +127,7 @@ static int snd_ctl_release(struct inode *inode, struct file *file)
 			if (control->vd[idx].owner == ctl)
 				control->vd[idx].owner = NULL;
 	up_write(&card->controls_rwsem);
+	snd_fasync_free(ctl->fasync);
 	snd_ctl_empty_read_queue(ctl);
 	put_pid(ctl->pid);
 	kfree(ctl);
@@ -181,7 +182,7 @@ void snd_ctl_notify(struct snd_card *card, unsigned int mask,
 	_found:
 		wake_up(&ctl->change_sleep);
 		spin_unlock(&ctl->read_lock);
-		kill_fasync(&ctl->fasync, SIGIO, POLL_IN);
+		snd_kill_fasync(ctl->fasync, SIGIO, POLL_IN);
 	}
 	read_unlock_irqrestore(&card->ctl_files_rwlock, flags);
 }
@@ -2134,7 +2135,7 @@ static int snd_ctl_fasync(int fd, struct file * file, int on)
 	struct snd_ctl_file *ctl;
 
 	ctl = file->private_data;
-	return fasync_helper(fd, file, on, &ctl->fasync);
+	return snd_fasync_helper(fd, file, on, &ctl->fasync);
 }
 
 /* return the preferred subdevice number if already assigned;
@@ -2302,7 +2303,7 @@ static int snd_ctl_dev_disconnect(struct snd_device *device)
 	read_lock_irqsave(&card->ctl_files_rwlock, flags);
 	list_for_each_entry(ctl, &card->ctl_files, list) {
 		wake_up(&ctl->change_sleep);
-		kill_fasync(&ctl->fasync, SIGIO, POLL_ERR);
+		snd_kill_fasync(ctl->fasync, SIGIO, POLL_ERR);
 	}
 	read_unlock_irqrestore(&card->ctl_files_rwlock, flags);
 
-- 
2.35.3

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

[Jaroslav Kysela]

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.

				Jaroslav

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

Re: [PATCH v2 0/4] ALSA: Defer async signal handling

[Jaroslav Kysela]

On 28. 07. 22 14:59, Takashi Iwai wrote:
> Hi,
> 
> this is a revised patch series for another attempt to address the
> potential deadlocks via kill_fasync() calls that have been reported by
> syzbot for long time.  Only a missing linux/fs.h inclusion was added
> from v1 series[1].
> 
> Instead of the previous series to drop the async handler[2], this
> tries to defer the kill_fasync() call.  A few new common helpers are
> introduced at first, then the actual usages are replaced in the
> following patches.
> 
> The patches passed the quick tests with alsa-lib test cases.

Thank you for your work. The code looks good (I would only move 
schedule_work() call outside the spin lock context - commented in the patch 
thread).

Reviewed-by: Jaroslav Kysela <perex@perex.cz>

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

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

[Takashi Iwai]

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.


thanks,

Takashi

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

[Jaroslav Kysela]

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.

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

[Takashi Iwai]

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@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.

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