From 871499e342fc0fe42e7dbef00fb680bef217c1c5 Mon Sep 17 00:00:00 2001 From: Ian Abbott Date: Wed, 7 Sep 2016 11:35:32 -0400 Subject: [PATCH] staging: comedi: comedi_test: fix timer race conditions MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit [ commit 403fe7f34e3327ddac2e06a15e76a293d613381e upstream. Patch is cut down a bit from upstream patch, as upstream patch fixed parts that don't exist in this version. Also, some identifiers were renamed, so patch description has been edited accordingly -- Ian Abbott ] Commit 73e0e4dfed4c ("staging: comedi: comedi_test: fix timer lock-up") fixed a lock-up in the timer routine `waveform_ai_interrupt()` caused by commit 240512474424 ("staging: comedi: comedi_test: use comedi_handle_events()"). However, it introduced a race condition that can result in the timer routine misbehaving, such as accessing freed memory or dereferencing a NULL pointer. 73e0... changed the timer routine to do nothing unless a `WAVEFORM_AI_RUNNING` flag was set, and changed `waveform_ai_cancel()` to clear the flag and replace a call to `del_timer_sync()` with a call to `del_timer()`. `waveform_ai_cancel()` may be called from the timer routine itself (via `comedi_handle_events()`), or from `do_cancel()`. (`do_cancel()` is called as a result of a file operation (usually a `COMEDI_CANCEL` ioctl command, or a release), or during device removal.) When called from `do_cancel()`, the call to `waveform_ai_cancel()` is followed by a call to `do_become_nonbusy()`, which frees up stuff for the current asynchronous command under the assumption that it is now safe to do so. The race condition occurs when the timer routine `waveform_ai_interrupt()` checks the `WAVEFORM_AI_RUNNING` flag just before it is cleared by `waveform_ai_cancel()`, and is still running during the call to `do_become_nonbusy()`. In particular, it can lead to a NULL pointer dereference because `do_become_nonbusy()` frees `async->cmd.chanlist` and sets it to `NULL`, but `waveform_ai_interrupt()` dereferences it. Fix the race by calling `del_timer_sync()` instead of `del_timer()` in `waveform_ai_cancel()` when not in an interrupt context. The only time `waveform_ai_cancel()` is called in an interrupt context is when it is called from the timer routine itself, via `comedi_handle_events()`. There is no longer any need for the `WAVEFORM_AI_RUNNING` flag, so get rid of it. Fixes: 73e0e4dfed4c ("staging: comedi: comedi_test: fix timer lock-up") Reported-by: Éric Piel Signed-off-by: Ian Abbott Cc: Éric Piel Signed-off-by: Sasha Levin --- drivers/staging/comedi/drivers/comedi_test.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c index 80d613c..6fd2c17 100644 --- a/drivers/staging/comedi/drivers/comedi_test.c +++ b/drivers/staging/comedi/drivers/comedi_test.c @@ -55,10 +55,6 @@ zero volts). #define N_CHANS 8 -enum waveform_state_bits { - WAVEFORM_AI_RUNNING = 0 -}; - /* Data unique to this driver */ struct waveform_private { struct timer_list timer; @@ -67,7 +63,6 @@ struct waveform_private { unsigned long usec_period; /* waveform period in microseconds */ unsigned long usec_current; /* current time (mod waveform period) */ unsigned long usec_remainder; /* usec since last scan */ - unsigned long state_bits; unsigned int scan_period; /* scan period in usec */ unsigned int convert_period; /* conversion period in usec */ unsigned int ao_loopbacks[N_CHANS]; @@ -177,10 +172,6 @@ static void waveform_ai_interrupt(unsigned long arg) unsigned int num_scans; ktime_t now; - /* check command is still active */ - if (!test_bit(WAVEFORM_AI_RUNNING, &devpriv->state_bits)) - return; - now = ktime_get(); elapsed_time = ktime_to_us(ktime_sub(now, devpriv->last)); @@ -322,10 +313,6 @@ static int waveform_ai_cmd(struct comedi_device *dev, devpriv->usec_remainder = 0; devpriv->timer.expires = jiffies + 1; - /* mark command as active */ - smp_mb__before_atomic(); - set_bit(WAVEFORM_AI_RUNNING, &devpriv->state_bits); - smp_mb__after_atomic(); add_timer(&devpriv->timer); return 0; } @@ -335,11 +322,12 @@ static int waveform_ai_cancel(struct comedi_device *dev, { struct waveform_private *devpriv = dev->private; - /* mark command as no longer active */ - clear_bit(WAVEFORM_AI_RUNNING, &devpriv->state_bits); - smp_mb__after_atomic(); - /* cannot call del_timer_sync() as may be called from timer routine */ - del_timer(&devpriv->timer); + if (in_softirq()) { + /* Assume we were called from the timer routine itself. */ + del_timer(&devpriv->timer); + } else { + del_timer_sync(&devpriv->timer); + } return 0; } -- 2.7.4