mbox series

[v2,0/5] ALSA: Drop async signal support

Message ID 20220717070549.5993-1-tiwai@suse.de (mailing list archive)
Headers show
Series ALSA: Drop async signal support | expand

Message

Takashi Iwai July 17, 2022, 7:05 a.m. UTC 
Hi,

this is a revised patch set for dropping fasync support from ALSA
core.

The async signal itself is very difficult to use properly due to
various restrictions (e.g. you cannot perform any I/O in the context),
hence it's a feature that has been never used by real applications.

OTOH, the real problem is that there have been quite a few syzcaller
reports indicating that fasync code path may lead to some potential
deadlocks for long time.  Dropping the feature is the easiest
solution, obviously.

The corresponding update for alsa-lib will follow once when we agree
with this approach.


thanks,

Takashi

===

v1: https://lore.kernel.org/r/20220715102935.4695-1-tiwai@suse.de
v1->v2: Fix unused variable warning in patch 2

===

Takashi Iwai (5):
  ALSA: timer: Drop async signal support
  ALSA: pcm: Drop async signal support
  ALSA: control: Drop async signal support
  ALSA: core: Drop async signal support
  ALSA: doc: Drop stale fasync entry

 .../kernel-api/writing-an-alsa-driver.rst      |  1 -
 include/sound/control.h                        |  1 -
 include/sound/pcm.h                            |  1 -
 sound/core/control.c                           | 11 -----------
 sound/core/init.c                              | 11 +----------
 sound/core/pcm_lib.c                           |  8 +-------
 sound/core/pcm_native.c                        | 18 ------------------
 sound/core/timer.c                             | 13 -------------
 8 files changed, 2 insertions(+), 62 deletions(-)

Comments

Jaroslav Kysela July 17, 2022, 10:16 a.m. UTC | #1 
Dne 17. 07. 22 v 9:05 Takashi Iwai napsal(a):
> Hi,
> 
> this is a revised patch set for dropping fasync support from ALSA
> core.
> 
> The async signal itself is very difficult to use properly due to
> various restrictions (e.g. you cannot perform any I/O in the context),
> hence it's a feature that has been never used by real applications.
> 
> OTOH, the real problem is that there have been quite a few syzcaller
> reports indicating that fasync code path may lead to some potential
> deadlocks for long time.  Dropping the feature is the easiest
> solution, obviously.

I would probably prefer to fix the problem (deferred async kill or so). The
SIGIO is just another way to wakeup the applications and there may be some
corner cases, where this wakeup is usable (threaded apps). Note that we had
some users (or testers) of SIGIO in past (at least there were questions about
this support).

					Jaroslav
Takashi Iwai July 25, 2022, 2:52 p.m. UTC | #2 
On Sun, 17 Jul 2022 12:16:13 +0200,
Jaroslav Kysela wrote:
> 
> Dne 17. 07. 22 v 9:05 Takashi Iwai napsal(a):
> > Hi,
> > 
> > this is a revised patch set for dropping fasync support from ALSA
> > core.
> > 
> > The async signal itself is very difficult to use properly due to
> > various restrictions (e.g. you cannot perform any I/O in the context),
> > hence it's a feature that has been never used by real applications.
> > 
> > OTOH, the real problem is that there have been quite a few syzcaller
> > reports indicating that fasync code path may lead to some potential
> > deadlocks for long time.  Dropping the feature is the easiest
> > solution, obviously.
> 
> I would probably prefer to fix the problem (deferred async kill or so). The
> SIGIO is just another way to wakeup the applications and there may be some
> corner cases, where this wakeup is usable (threaded apps). Note that we had
> some users (or testers) of SIGIO in past (at least there were questions about
> this support).

Hm, OK, then let's discard it for now.

The deferred kill_async() implementation would be something like below.
A quick test looks OK, so far, with the given syzkaller reproducer.

Ideally speaking, this should be fixed in the fasync sighandler side,
but it appears fragile -- kill_fasync() calls send_sigio(), and
send_sigio() takes read_lock(&tasklist_lock).  And
read_lock(&tasklist_lock) itself is a common pattern taken in the
non/soft-IRQ contexts, which would conflict with the call in an
hard-IRQ context...


thanks,

Takashi

-- 8< --
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/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/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/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);
 
diff --git a/sound/core/misc.c b/sound/core/misc.c
index 50e4aaa6270d..f9d100905b98 100644
--- a/sound/core/misc.c
+++ b/sound/core/misc.c
@@ -145,3 +145,53 @@ snd_pci_quirk_lookup(struct pci_dev *pci, const struct snd_pci_quirk *list)
 }
 EXPORT_SYMBOL(snd_pci_quirk_lookup);
 #endif
+
+/* async signal helpers */
+struct snd_fasync {
+	struct fasync_struct *fasync;
+	struct work_struct work;
+	int signal;
+	int poll;
+};
+
+static void snd_fasync_work(struct work_struct *work)
+{
+	struct snd_fasync *fasync = container_of(work, struct snd_fasync, work);
+
+	kill_fasync(&fasync->fasync, fasync->signal, fasync->poll);
+}
+
+int snd_fasync_helper(int fd, struct file *file, int on,
+		      struct snd_fasync **fasyncp)
+{
+	struct snd_fasync *fasync = *fasyncp;
+
+	if (!fasync) {
+		fasync = kzalloc(sizeof(*fasync), GFP_KERNEL);
+		if (!fasync)
+			return -ENOMEM;
+		INIT_WORK(&fasync->work, snd_fasync_work);
+		*fasyncp = fasync;
+	}
+	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)
+{
+	if (!fasync)
+		return;
+	fasync->signal = signal;
+	fasync->poll = poll;
+	schedule_work(&fasync->work);
+}
+EXPORT_SYMBOL_GPL(snd_kill_fasync);
+
+void snd_fasync_free(struct snd_fasync *fasync)
+{
+	if (!fasync)
+		return;
+	cancel_work_sync(&fasync->work);
+	kfree(fasync);
+}
+EXPORT_SYMBOL_GPL(snd_fasync_free);
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);
 }
 
 /*
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,