| Message ID | s5hwprdv2sp.wl-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 13, 2016 at 5:53 PM, Takashi Iwai <tiwai@suse.de> wrote: > This and your other relevant reports seem pointing the race of timer > ioctls. Although snd_timer_close() itself calls snd_timer_stop(), > there is no other protection against the concurrent execution. > > If my guess is correct, a simplistic fix like below should work. It > basically serializes the timer ioctl by using a new mutex (and > replacing the old tread_sem mutex). They are no longtime blocking > calls, so this shouldn't be a big problem. But certainly there can be > a less intrusive way to paper over this if this really matters. > > In this case for timer.c, I'd leave the final decision rather to > Jaroslav. Jaroslav, what do you think? After applying this patch I still see the following WARNINGS: ------------[ cut here ]------------ WARNING: CPU: 2 PID: 30398 at lib/list_debug.c:53 __list_del_entry+0x10b/0x1e0() list_del corruption, ffff880032d933b0->next is LIST_POISON1 (dead000000000100) Modules linked in: CPU: 2 PID: 30398 Comm: syz-executor Not tainted 4.4.0+ #241 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 00000000ffffffff ffff8800627778d8 ffffffff82926eed ffff880062777948 ffff880061c2af80 ffffffff8660b640 ffff880062777918 ffffffff81350c89 ffffffff8298e77b ffffed000c4eef25 ffffffff8660b640 0000000000000035 Call Trace: [< inline >] __dump_stack lib/dump_stack.c:15 [<ffffffff82926eed>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50 [<ffffffff81350c89>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:483 [<ffffffff81350d99>] warn_slowpath_fmt+0xa9/0xd0 kernel/panic.c:495 [<ffffffff8298e77b>] __list_del_entry+0x10b/0x1e0 lib/list_debug.c:51 [< inline >] list_del_init include/linux/list.h:145 [<ffffffff84ebd199>] _snd_timer_stop+0x119/0x450 sound/core/timer.c:501 [<ffffffff84ebd4f4>] snd_timer_stop+0x24/0x140 sound/core/timer.c:535 [<ffffffff84ebd648>] snd_timer_close+0x38/0x5f0 sound/core/timer.c:317 [< inline >] snd_timer_user_tselect sound/core/timer.c:1518 [< inline >] __snd_timer_user_ioctl sound/core/timer.c:1803 [<ffffffff84ec4362>] snd_timer_user_ioctl+0x7b2/0x25c0 sound/core/timer.c:1833 [< inline >] vfs_ioctl fs/ioctl.c:43 [<ffffffff817cbd3c>] do_vfs_ioctl+0x18c/0xfa0 fs/ioctl.c:674 [< inline >] SYSC_ioctl fs/ioctl.c:689 [<ffffffff817ccbdf>] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:680 [<ffffffff86273076>] entry_SYSCALL_64_fastpath+0x16/0x7a arch/x86/entry/entry_64.S:185 ---[ end trace bfebf27b922184a1 ]---
diff --git a/sound/core/timer.c b/sound/core/timer.c index 31f40f03e5b7..b03a9e489286 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -73,7 +73,7 @@ struct snd_timer_user { struct timespec tstamp; /* trigger tstamp */ wait_queue_head_t qchange_sleep; struct fasync_struct *fasync; - struct mutex tread_sem; + struct mutex ioctl_lock; }; /* list of timers */ @@ -1253,7 +1253,7 @@ static int snd_timer_user_open(struct inode *inode, struct file *file) return -ENOMEM; spin_lock_init(&tu->qlock); init_waitqueue_head(&tu->qchange_sleep); - mutex_init(&tu->tread_sem); + mutex_init(&tu->ioctl_lock); tu->ticks = 1; tu->queue_size = 128; tu->queue = kmalloc(tu->queue_size * sizeof(struct snd_timer_read), @@ -1273,8 +1273,10 @@ static int snd_timer_user_release(struct inode *inode, struct file *file) if (file->private_data) { tu = file->private_data; file->private_data = NULL; + mutex_lock(&tu->ioctl_lock); if (tu->timeri) snd_timer_close(tu->timeri); + mutex_unlock(&tu->ioctl_lock); kfree(tu->queue); kfree(tu->tqueue); kfree(tu); @@ -1512,7 +1514,6 @@ static int snd_timer_user_tselect(struct file *file, int err = 0; tu = file->private_data; - mutex_lock(&tu->tread_sem); if (tu->timeri) { snd_timer_close(tu->timeri); tu->timeri = NULL; @@ -1556,7 +1557,6 @@ static int snd_timer_user_tselect(struct file *file, } __err: - mutex_unlock(&tu->tread_sem); return err; } @@ -1769,7 +1769,7 @@ enum { SNDRV_TIMER_IOCTL_PAUSE_OLD = _IO('T', 0x23), }; -static long snd_timer_user_ioctl(struct file *file, unsigned int cmd, +static long __snd_timer_user_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { struct snd_timer_user *tu; @@ -1786,17 +1786,11 @@ static long snd_timer_user_ioctl(struct file *file, unsigned int cmd, { int xarg; - mutex_lock(&tu->tread_sem); - if (tu->timeri) { /* too late */ - mutex_unlock(&tu->tread_sem); + if (tu->timeri) /* too late */ return -EBUSY; - } - if (get_user(xarg, p)) { - mutex_unlock(&tu->tread_sem); + if (get_user(xarg, p)) return -EFAULT; - } tu->tread = xarg ? 1 : 0; - mutex_unlock(&tu->tread_sem); return 0; } case SNDRV_TIMER_IOCTL_GINFO: @@ -1829,6 +1823,18 @@ static long snd_timer_user_ioctl(struct file *file, unsigned int cmd, return -ENOTTY; } +static long snd_timer_user_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) +{ + struct snd_timer_user *tu = file->private_data; + long ret; + + mutex_lock(&tu->ioctl_lock); + ret = __snd_timer_user_ioctl(file, cmd, arg); + mutex_unlock(&tu->ioctl_lock); + return ret; +} + static int snd_timer_user_fasync(int fd, struct file * file, int on) { struct snd_timer_user *tu;