From c08c464d6f4136d9e48ffa23c0bcd93442343b2a Mon Sep 17 00:00:00 2001 From: Al Viro Date: Mon, 15 Apr 2013 16:31:13 -0400 Subject: mISDN: fix the races with timers going off just as they are deleted timer callback in timerdev.c both accesses struct mISDNtimer it's called for *and* moves it to dev->expired. We need del_timer_sync(), or we risk kfree() freeing it right under dev_expire_timer() *and* dev->expired getting corrupted. Signed-off-by: Al Viro --- drivers/isdn/mISDN/timerdev.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) (limited to 'drivers/isdn/mISDN') diff --git a/drivers/isdn/mISDN/timerdev.c b/drivers/isdn/mISDN/timerdev.c index 1094667d8f3..5a1a5cadc76 100644 --- a/drivers/isdn/mISDN/timerdev.c +++ b/drivers/isdn/mISDN/timerdev.c @@ -72,14 +72,24 @@ static int mISDN_close(struct inode *ino, struct file *filep) { struct mISDNtimerdev *dev = filep->private_data; + struct list_head *list = &dev->pending; struct mISDNtimer *timer, *next; if (*debug & DEBUG_TIMER) printk(KERN_DEBUG "%s(%p,%p)\n", __func__, ino, filep); - list_for_each_entry_safe(timer, next, &dev->pending, list) { - del_timer(&timer->tl); + + spin_lock_irq(&dev->lock); + while (!list_empty(list)) { + timer = list_first_entry(list, struct mISDNtimer, list); + spin_unlock_irq(&dev->lock); + del_timer_sync(&timer->tl); + spin_lock_irq(&dev->lock); + /* it might have been moved to ->expired */ + list_del(&timer->list); kfree(timer); } + spin_unlock_irq(&dev->lock); + list_for_each_entry_safe(timer, next, &dev->expired, list) { kfree(timer); } -- cgit v1.2.3-70-g09d2 From 1b1089561ce596a4032ba1039365090304db1cfd Mon Sep 17 00:00:00 2001 From: Al Viro Date: Mon, 15 Apr 2013 16:55:41 -0400 Subject: mISDN: fix races between misdn_del_timer() and timer callback mark the victim with negative ->id if misdn_del_timer() finds it on the list, have timer callback *not* move ones so marked to dev->expired Signed-off-by: Al Viro --- drivers/isdn/mISDN/timerdev.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) (limited to 'drivers/isdn/mISDN') diff --git a/drivers/isdn/mISDN/timerdev.c b/drivers/isdn/mISDN/timerdev.c index 5a1a5cadc76..c00546f830d 100644 --- a/drivers/isdn/mISDN/timerdev.c +++ b/drivers/isdn/mISDN/timerdev.c @@ -163,7 +163,8 @@ dev_expire_timer(unsigned long data) u_long flags; spin_lock_irqsave(&timer->dev->lock, flags); - list_move_tail(&timer->list, &timer->dev->expired); + if (timer->id >= 0) + list_move_tail(&timer->list, &timer->dev->expired); spin_unlock_irqrestore(&timer->dev->lock, flags); wake_up_interruptible(&timer->dev->wait); } @@ -203,26 +204,21 @@ misdn_add_timer(struct mISDNtimerdev *dev, int timeout) static int misdn_del_timer(struct mISDNtimerdev *dev, int id) { - u_long flags; struct mISDNtimer *timer; - int ret = 0; - spin_lock_irqsave(&dev->lock, flags); + spin_lock_irq(&dev->lock); list_for_each_entry(timer, &dev->pending, list) { if (timer->id == id) { list_del_init(&timer->list); - /* RED-PEN AK: race -- timer can be still running on - * other CPU. Needs reference count I think - */ - del_timer(&timer->tl); - ret = timer->id; + timer->id = -1; + spin_unlock_irq(&dev->lock); + del_timer_sync(&timer->tl); kfree(timer); - goto unlock; + return id; } } -unlock: - spin_unlock_irqrestore(&dev->lock, flags); - return ret; + spin_unlock_irq(&dev->lock); + return 0; } static long -- cgit v1.2.3-70-g09d2 From 1678ec00a632f8b9204e28e5c506128881171604 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Mon, 15 Apr 2013 17:04:04 -0400 Subject: mISDN: fix misdn_add_timer()/misdn_del_timer() race do add_timer() *before* unlocking dev->lock, or unpleasant things can happen if misdn_del_timer() on another CPU finds the sucker, calls del_timer_sync() (which does nothing, since we hadn't started the timer yet) and frees it, just as we get around to add_timer()... Signed-off-by: Al Viro --- drivers/isdn/mISDN/timerdev.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) (limited to 'drivers/isdn/mISDN') diff --git a/drivers/isdn/mISDN/timerdev.c b/drivers/isdn/mISDN/timerdev.c index c00546f830d..ddb8adcd5fb 100644 --- a/drivers/isdn/mISDN/timerdev.c +++ b/drivers/isdn/mISDN/timerdev.c @@ -173,7 +173,6 @@ static int misdn_add_timer(struct mISDNtimerdev *dev, int timeout) { int id; - u_long flags; struct mISDNtimer *timer; if (!timeout) { @@ -184,19 +183,16 @@ misdn_add_timer(struct mISDNtimerdev *dev, int timeout) timer = kzalloc(sizeof(struct mISDNtimer), GFP_KERNEL); if (!timer) return -ENOMEM; - spin_lock_irqsave(&dev->lock, flags); - timer->id = dev->next_id++; + timer->dev = dev; + setup_timer(&timer->tl, dev_expire_timer, (long)timer); + spin_lock_irq(&dev->lock); + id = timer->id = dev->next_id++; if (dev->next_id < 0) dev->next_id = 1; list_add_tail(&timer->list, &dev->pending); - spin_unlock_irqrestore(&dev->lock, flags); - timer->dev = dev; - timer->tl.data = (long)timer; - timer->tl.function = dev_expire_timer; - init_timer(&timer->tl); timer->tl.expires = jiffies + ((HZ * (u_long)timeout) / 1000); add_timer(&timer->tl); - id = timer->id; + spin_unlock_irq(&dev->lock); } return id; } -- cgit v1.2.3-70-g09d2 From ebb06be16bc9a1e66a010ca50c75c5128bafb4b1 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Mon, 15 Apr 2013 17:18:17 -0400 Subject: mISDN: fix mISDN_read()/mISDN_read() race Signed-off-by: Al Viro --- drivers/isdn/mISDN/timerdev.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) (limited to 'drivers/isdn/mISDN') diff --git a/drivers/isdn/mISDN/timerdev.c b/drivers/isdn/mISDN/timerdev.c index ddb8adcd5fb..da2aa376a3a 100644 --- a/drivers/isdn/mISDN/timerdev.c +++ b/drivers/isdn/mISDN/timerdev.c @@ -102,36 +102,41 @@ static ssize_t mISDN_read(struct file *filep, char __user *buf, size_t count, loff_t *off) { struct mISDNtimerdev *dev = filep->private_data; + struct list_head *list = &dev->expired; struct mISDNtimer *timer; - u_long flags; int ret = 0; if (*debug & DEBUG_TIMER) printk(KERN_DEBUG "%s(%p, %p, %d, %p)\n", __func__, filep, buf, (int)count, off); - if (list_empty(&dev->expired) && (dev->work == 0)) { + if (count < sizeof(int)) + return -ENOSPC; + + spin_lock_irq(&dev->lock); + while (list_empty(list) && (dev->work == 0)) { + spin_unlock_irq(&dev->lock); if (filep->f_flags & O_NONBLOCK) return -EAGAIN; wait_event_interruptible(dev->wait, (dev->work || - !list_empty(&dev->expired))); + !list_empty(list))); if (signal_pending(current)) return -ERESTARTSYS; + spin_lock_irq(&dev->lock); } - if (count < sizeof(int)) - return -ENOSPC; if (dev->work) dev->work = 0; - if (!list_empty(&dev->expired)) { - spin_lock_irqsave(&dev->lock, flags); - timer = (struct mISDNtimer *)dev->expired.next; + if (!list_empty(list)) { + timer = list_first_entry(list, struct mISDNtimer, list); list_del(&timer->list); - spin_unlock_irqrestore(&dev->lock, flags); + spin_unlock_irq(&dev->lock); if (put_user(timer->id, (int __user *)buf)) ret = -EFAULT; else ret = sizeof(int); kfree(timer); + } else { + spin_unlock_irq(&dev->lock); } return ret; } -- cgit v1.2.3-70-g09d2 From 89b107adce32a52920b36787b60c8f24c986c526 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Mon, 15 Apr 2013 17:27:11 -0400 Subject: mISDN: grabbing/dropping reference to THIS_MODULE in open/release is racy ... when you have no ->owner set. Signed-off-by: Al Viro --- drivers/isdn/mISDN/timerdev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'drivers/isdn/mISDN') diff --git a/drivers/isdn/mISDN/timerdev.c b/drivers/isdn/mISDN/timerdev.c index da2aa376a3a..9438d7ec330 100644 --- a/drivers/isdn/mISDN/timerdev.c +++ b/drivers/isdn/mISDN/timerdev.c @@ -64,7 +64,6 @@ mISDN_open(struct inode *ino, struct file *filep) dev->work = 0; init_waitqueue_head(&dev->wait); filep->private_data = dev; - __module_get(THIS_MODULE); return nonseekable_open(ino, filep); } @@ -94,7 +93,6 @@ mISDN_close(struct inode *ino, struct file *filep) kfree(timer); } kfree(dev); - module_put(THIS_MODULE); return 0; } @@ -269,6 +267,7 @@ mISDN_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) } static const struct file_operations mISDN_fops = { + .owner = THIS_MODULE, .read = mISDN_read, .poll = mISDN_poll, .unlocked_ioctl = mISDN_ioctl, -- cgit v1.2.3-70-g09d2