[PATCH 0/5] ALSA: Drop async signal support

older
[PATCH 0/4] ASoC: SOF: Intel: add...

Takashi Iwai

15 Jul 2022 15 Jul '22

12:29 p.m.

Hi,

this is a patch series to drop one of unused features, the async signal 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

===

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

-- 2.35.3

Show replies by date

Takashi Iwai

15 Jul 15 Jul

12:29 p.m.

New subject: [PATCH 1/5] ALSA: timer: Drop async signal support

The async signal (SIGIO) support for ALSA timer API has been never used by real applications, but yet it can be a cause of various potential deadlocks, as spotted by syzkaller. Let's drop the feature as the simplest solution.

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 | 13 ------------- 1 file changed, 13 deletions(-)

diff --git a/sound/core/timer.c b/sound/core/timer.c index b3214baa8919..4ac3ab2cf575 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -83,7 +83,6 @@ struct snd_timer_user { unsigned int filter; struct timespec64 tstamp; /* trigger tstamp */ wait_queue_head_t qchange_sleep; - struct fasync_struct *fasync; struct mutex ioctl_lock; };

@@ -1345,7 +1344,6 @@ static void snd_timer_user_interrupt(struct snd_timer_instance *timeri, } __wake: spin_unlock(&tu->qlock); - kill_fasync(&tu->fasync, SIGIO, POLL_IN); wake_up(&tu->qchange_sleep); }

@@ -1383,7 +1381,6 @@ 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); wake_up(&tu->qchange_sleep); }

@@ -1453,7 +1450,6 @@ 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); wake_up(&tu->qchange_sleep); }

@@ -2130,14 +2126,6 @@ static long snd_timer_user_ioctl(struct file *file, unsigned int cmd, return ret; }

-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); -} - static ssize_t snd_timer_user_read(struct file *file, char __user *buffer, size_t count, loff_t *offset) { @@ -2280,7 +2268,6 @@ static const struct file_operations snd_timer_f_ops = .poll = snd_timer_user_poll, .unlocked_ioctl = snd_timer_user_ioctl, .compat_ioctl = snd_timer_user_ioctl_compat, - .fasync = snd_timer_user_fasync, };

/* unregister the system timer */

-- 2.35.3

Takashi Iwai

12:29 p.m.

New subject: [PATCH 2/5] ALSA: pcm: Drop async signal support

The async signal (SIGIO) support for ALSA PCM API has been never used by real applications, but yet it can be a cause of various potential deadlocks, as spotted by syzkaller. Let's drop the feature as the simplest solution.

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 | 1 - sound/core/pcm_lib.c | 4 +--- sound/core/pcm_native.c | 18 ------------------ 3 files changed, 1 insertion(+), 22 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 08cf4a5801f3..9bbd3742ef59 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -399,7 +399,6 @@ 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; 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_lib.c b/sound/core/pcm_lib.c index 1fc7c50ffa62..a7b10ee5d57d 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -1815,14 +1815,12 @@ void snd_pcm_period_elapsed_under_stream_lock(struct snd_pcm_substream *substrea

if (!snd_pcm_running(substream) || snd_pcm_update_hw_ptr0(substream, 1) < 0) - goto _end; + return;

#ifdef CONFIG_SND_PCM_TIMER if (substream->timer_running) snd_timer_interrupt(substream->timer, 1); #endif - _end: - 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..c8e340b97277 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -3938,22 +3938,6 @@ static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area) return 0; }

-static int snd_pcm_fasync(int fd, struct file * file, int on) -{ - struct snd_pcm_file * pcm_file; - struct snd_pcm_substream *substream; - struct snd_pcm_runtime *runtime; - - pcm_file = file->private_data; - substream = pcm_file->substream; - if (PCM_RUNTIME_CHECK(substream)) - return -ENXIO; - runtime = substream->runtime; - if (runtime->status->state == SNDRV_PCM_STATE_DISCONNECTED) - return -EBADFD; - return fasync_helper(fd, file, on, &runtime->fasync); -} - /* * ioctl32 compat */ @@ -4118,7 +4102,6 @@ const struct file_operations snd_pcm_f_ops[2] = { .unlocked_ioctl = snd_pcm_ioctl, .compat_ioctl = snd_pcm_ioctl_compat, .mmap = snd_pcm_mmap, - .fasync = snd_pcm_fasync, .get_unmapped_area = snd_pcm_get_unmapped_area, }, { @@ -4132,7 +4115,6 @@ const struct file_operations snd_pcm_f_ops[2] = { .unlocked_ioctl = snd_pcm_ioctl, .compat_ioctl = snd_pcm_ioctl_compat, .mmap = snd_pcm_mmap, - .fasync = snd_pcm_fasync, .get_unmapped_area = snd_pcm_get_unmapped_area, } };

-- 2.35.3

kernel test robot

16 Jul 16 Jul

7:02 p.m.

New subject: [PATCH 2/5] ALSA: pcm: Drop async signal support

Hi Takashi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on tiwai-sound/for-next] [also build test WARNING on linus/master v5.19-rc6 next-20220715] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Takashi-Iwai/ALSA-Drop-async-... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next config: i386-randconfig-a001 (https://download.01.org/0day-ci/archive/20220717/202207170009.xmmdeeBR-lkp@i...) compiler: gcc-11 (Debian 11.3.0-3) 11.3.0 reproduce (this is a W=1 build): # https://github.com/intel-lab-lkp/linux/commit/6705167eb6bf093c8a099fa16c0aeb... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Takashi-Iwai/ALSA-Drop-async-signal-support/20220716-211853 git checkout 6705167eb6bf093c8a099fa16c0aeb5f3da0dfba # save the config file mkdir build_dir && cp config build_dir/.config make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash sound/core/

If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot lkp@intel.com

All warnings (new ones prefixed by >>):

sound/core/pcm_lib.c: In function 'snd_pcm_period_elapsed_under_stream_lock':

...

...
sound/core/pcm_lib.c:1810:33: warning: variable 'runtime' set but not used [-Wunused-but-set-variable]

1810 | struct snd_pcm_runtime *runtime; | ^~~~~~~

vim +/runtime +1810 sound/core/pcm_lib.c

e88e8ae639a490 Takashi Iwai 2006-04-28 1780 ^1da177e4c3f41 Linus Torvalds 2005-04-16 1781 /** 47271b1b98c980 Takashi Sakamoto 2021-06-10 1782 * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period 47271b1b98c980 Takashi Sakamoto 2021-06-10 1783 * under acquired lock of PCM substream. 47271b1b98c980 Takashi Sakamoto 2021-06-10 1784 * @substream: the instance of pcm substream. 47271b1b98c980 Takashi Sakamoto 2021-06-10 1785 * 47271b1b98c980 Takashi Sakamoto 2021-06-10 1786 * This function is called when the batch of audio data frames as the same size as the period of 47271b1b98c980 Takashi Sakamoto 2021-06-10 1787 * buffer is already processed in audio data transmission. 47271b1b98c980 Takashi Sakamoto 2021-06-10 1788 * 47271b1b98c980 Takashi Sakamoto 2021-06-10 1789 * The call of function updates the status of runtime with the latest position of audio data 47271b1b98c980 Takashi Sakamoto 2021-06-10 1790 * transmission, checks overrun and underrun over buffer, awaken user processes from waiting for 47271b1b98c980 Takashi Sakamoto 2021-06-10 1791 * available audio data frames, sampling audio timestamp, and performs stop or drain the PCM 47271b1b98c980 Takashi Sakamoto 2021-06-10 1792 * substream according to configured threshold. 47271b1b98c980 Takashi Sakamoto 2021-06-10 1793 * 47271b1b98c980 Takashi Sakamoto 2021-06-10 1794 * The function is intended to use for the case that PCM driver operates audio data frames under 47271b1b98c980 Takashi Sakamoto 2021-06-10 1795 * acquired lock of PCM substream; e.g. in callback of any operation of &snd_pcm_ops in process 47271b1b98c980 Takashi Sakamoto 2021-06-10 1796 * context. In any interrupt context, it's preferrable to use ``snd_pcm_period_elapsed()`` instead 47271b1b98c980 Takashi Sakamoto 2021-06-10 1797 * since lock of PCM substream should be acquired in advance. ^1da177e4c3f41 Linus Torvalds 2005-04-16 1798 * 47271b1b98c980 Takashi Sakamoto 2021-06-10 1799 * Developer should pay enough attention that some callbacks in &snd_pcm_ops are done by the call of 47271b1b98c980 Takashi Sakamoto 2021-06-10 1800 * function: ^1da177e4c3f41 Linus Torvalds 2005-04-16 1801 * 47271b1b98c980 Takashi Sakamoto 2021-06-10 1802 * - .pointer - to retrieve current position of audio data transmission by frame count or XRUN state. 47271b1b98c980 Takashi Sakamoto 2021-06-10 1803 * - .trigger - with SNDRV_PCM_TRIGGER_STOP at XRUN or DRAINING state. 47271b1b98c980 Takashi Sakamoto 2021-06-10 1804 * - .get_time_info - to retrieve audio time stamp if needed. 47271b1b98c980 Takashi Sakamoto 2021-06-10 1805 * 47271b1b98c980 Takashi Sakamoto 2021-06-10 1806 * Even if more than one periods have elapsed since the last call, you have to call this only once. ^1da177e4c3f41 Linus Torvalds 2005-04-16 1807 */ 47271b1b98c980 Takashi Sakamoto 2021-06-10 1808 void snd_pcm_period_elapsed_under_stream_lock(struct snd_pcm_substream *substream) ^1da177e4c3f41 Linus Torvalds 2005-04-16 1809 { 877211f5e1b119 Takashi Iwai 2005-11-17 @1810 struct snd_pcm_runtime *runtime; ^1da177e4c3f41 Linus Torvalds 2005-04-16 1811 f5cdc9d4003a2f paulhsia 2019-11-13 1812 if (PCM_RUNTIME_CHECK(substream)) 47271b1b98c980 Takashi Sakamoto 2021-06-10 1813 return; f5cdc9d4003a2f paulhsia 2019-11-13 1814 runtime = substream->runtime; f5cdc9d4003a2f paulhsia 2019-11-13 1815 ^1da177e4c3f41 Linus Torvalds 2005-04-16 1816 if (!snd_pcm_running(substream) || f240406babfe15 Jaroslav Kysela 2010-01-05 1817 snd_pcm_update_hw_ptr0(substream, 1) < 0) 6705167eb6bf09 Takashi Iwai 2022-07-15 1818 return; ^1da177e4c3f41 Linus Torvalds 2005-04-16 1819

-- 0-DAY CI Kernel Test Service https://01.org/lkp

kernel test robot

7:53 p.m.

New subject: [PATCH 2/5] ALSA: pcm: Drop async signal support

Hi Takashi,

I love your patch! Perhaps something to improve:

url: https://github.com/intel-lab-lkp/linux/commits/Takashi-Iwai/ALSA-Drop-async-... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next config: i386-randconfig-a011 (https://download.01.org/0day-ci/archive/20220717/202207170141.kyNwoI54-lkp@i...) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 07022e6cf9b5b3baa642be53d0b3c3f1c403dbfd) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/6705167eb6bf093c8a099fa16c0aeb... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Takashi-Iwai/ALSA-Drop-async-signal-support/20220716-211853 git checkout 6705167eb6bf093c8a099fa16c0aeb5f3da0dfba # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash sound/core/

If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot lkp@intel.com

All warnings (new ones prefixed by >>):

...

...
sound/core/pcm_lib.c:1810:26: warning: variable 'runtime' set but not used [-Wunused-but-set-variable]

struct snd_pcm_runtime *runtime; ^ 1 warning generated.

vim +/runtime +1810 sound/core/pcm_lib.c

-- 0-DAY CI Kernel Test Service https://01.org/lkp

Takashi Iwai

15 Jul 15 Jul

12:29 p.m.

New subject: [PATCH 3/5] ALSA: control: Drop async signal support

The async signal (SIGIO) support for ALSA control API has been never used by real applications, but yet it can be a cause of various potential deadlocks (although there hasn't been a bug report in this code path unlike timer and PCM). Let's drop the feature as the simplest solution to align with other APIs.

Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/control.h | 1 - sound/core/control.c | 11 ----------- 2 files changed, 12 deletions(-)

diff --git a/include/sound/control.h b/include/sound/control.h index fcd3cce673ec..0390d4952dac 100644 --- a/include/sound/control.h +++ b/include/sound/control.h @@ -109,7 +109,6 @@ 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; 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..3d3b8bf93f80 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -181,7 +181,6 @@ 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); } read_unlock_irqrestore(&card->ctl_files_rwlock, flags); } @@ -2129,14 +2128,6 @@ int snd_ctl_unregister_ioctl_compat(snd_kctl_ioctl_func_t fcn) EXPORT_SYMBOL(snd_ctl_unregister_ioctl_compat); #endif

-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 the preferred subdevice number if already assigned; * otherwise return -1 */ @@ -2264,7 +2255,6 @@ static const struct file_operations snd_ctl_f_ops = .poll = snd_ctl_poll, .unlocked_ioctl = snd_ctl_ioctl, .compat_ioctl = snd_ctl_ioctl_compat, - .fasync = snd_ctl_fasync, };

/* @@ -2302,7 +2292,6 @@ 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); } read_unlock_irqrestore(&card->ctl_files_rwlock, flags);

-- 2.35.3

Takashi Iwai

12:29 p.m.

New subject: [PATCH 4/5] ALSA: core: Drop async signal support

Now fasync support is dropped from all ALSA interfaces, and we can drop the fasync workaround at the card disconnection code, too.

Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/init.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/sound/core/init.c b/sound/core/init.c index 3ac95c66a4b5..899bfe838a91 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -432,11 +432,8 @@ static int snd_disconnect_release(struct inode *inode, struct file *file) } spin_unlock(&shutdown_lock);

- if (likely(df)) { - if ((file->f_flags & FASYNC) && df->disconnected_f_op->fasync) - df->disconnected_f_op->fasync(-1, file, 0); + if (likely(df)) return df->disconnected_f_op->release(inode, file); - }

panic("%s(%p, %p) failed!", __func__, inode, file); } @@ -457,11 +454,6 @@ static int snd_disconnect_mmap(struct file *file, struct vm_area_struct *vma) return -ENODEV; }

-static int snd_disconnect_fasync(int fd, struct file *file, int on) -{ - return -ENODEV; -} - static const struct file_operations snd_shutdown_f_ops = { .owner = THIS_MODULE, @@ -475,7 +467,6 @@ static const struct file_operations snd_shutdown_f_ops = .compat_ioctl = snd_disconnect_ioctl, #endif .mmap = snd_disconnect_mmap, - .fasync = snd_disconnect_fasync };

/**

-- 2.35.3

Takashi Iwai

12:29 p.m.

New subject: [PATCH 5/5] ALSA: doc: Drop stale fasync entry

The fasync entry has been dropped recently. Update the documentation as well.

Signed-off-by: Takashi Iwai tiwai@suse.de --- Documentation/sound/kernel-api/writing-an-alsa-driver.rst | 1 - 1 file changed, 1 deletion(-)

diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst index 176b73583b7a..84b4ecae1485 100644 --- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst +++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst @@ -1597,7 +1597,6 @@ are the contents of this file: spinlock_t lock; wait_queue_head_t sleep; struct timer_list tick_timer; - struct fasync_struct *fasync;

/* -- private section -- */ void *private_data;

-- 2.35.3

936

Age (days ago)

937

Last active (days ago)

List overview

Download

7 comments

2 participants

tags (0)

participants (2)

kernel test robot
Takashi Iwai