| Message ID | 20220728125945.29533-2-tiwai@suse.de |
|---|---|
| State | Accepted |
| Commit | ef34a0ae7a2654bc9e58675e36898217fb2799d8 |
| Headers | show |
| Series | ALSA: Defer async signal handling | expand |
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
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
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
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
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);
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(+)