[ 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 <piel@xxxxxxxxxx>
Signed-off-by: Ian Abbott <abbotti@xxxxxxxxx>
Cc: Éric Piel <piel@xxxxxxxxxx>
---
drivers/staging/comedi/drivers/comedi_test.c | 24 ++++++------------------
1 file changed, 6 insertions(+), 18 deletions(-)
---
Reposting, as I used as out-of-date email address for Sasha the first time.
Sorry! -- Ian
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.9.3
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html