]>
Commit | Line | Data |
---|---|---|
ad834691 GKH |
1 | From 403fe7f34e3327ddac2e06a15e76a293d613381e Mon Sep 17 00:00:00 2001 |
2 | From: Ian Abbott <abbotti@mev.co.uk> | |
3 | Date: Thu, 30 Jun 2016 19:58:32 +0100 | |
4 | Subject: staging: comedi: comedi_test: fix timer race conditions | |
5 | MIME-Version: 1.0 | |
6 | Content-Type: text/plain; charset=UTF-8 | |
7 | Content-Transfer-Encoding: 8bit | |
8 | ||
9 | From: Ian Abbott <abbotti@mev.co.uk> | |
10 | ||
11 | commit 403fe7f34e3327ddac2e06a15e76a293d613381e upstream. | |
12 | ||
13 | Commit 73e0e4dfed4c ("staging: comedi: comedi_test: fix timer lock-up") | |
14 | fixed a lock-up in the timer routine `waveform_ai_timer()` (which was | |
15 | called `waveform_ai_interrupt()` at the time) caused by | |
16 | commit 240512474424 ("staging: comedi: comedi_test: use | |
17 | comedi_handle_events()"). However, it introduced a race condition that | |
18 | can result in the timer routine misbehaving, such as accessing freed | |
19 | memory or dereferencing a NULL pointer. | |
20 | ||
21 | 73e0... changed the timer routine to do nothing unless a | |
22 | `WAVEFORM_AI_RUNNING` flag was set, and changed `waveform_ai_cancel()` | |
23 | to clear the flag and replace a call to `del_timer_sync()` with a call | |
24 | to `del_timer()`. `waveform_ai_cancel()` may be called from the timer | |
25 | routine itself (via `comedi_handle_events()`), or from `do_cancel()`. | |
26 | (`do_cancel()` is called as a result of a file operation (usually a | |
27 | `COMEDI_CANCEL` ioctl command, or a release), or during device removal.) | |
28 | When called from `do_cancel()`, the call to `waveform_ai_cancel()` is | |
29 | followed by a call to `do_become_nonbusy()`, which frees up stuff for | |
30 | the current asynchronous command under the assumption that it is now | |
31 | safe to do so. The race condition occurs when the timer routine | |
32 | `waveform_ai_timer()` checks the `WAVEFORM_AI_RUNNING` flag just before | |
33 | it is cleared by `waveform_ai_cancel()`, and is still running during the | |
34 | call to `do_become_nonbusy()`. In particular, it can lead to a NULL | |
35 | pointer dereference: | |
36 | ||
37 | BUG: unable to handle kernel NULL pointer dereference at (null) | |
38 | IP: [<ffffffffc0c63add>] waveform_ai_timer+0x17d/0x290 [comedi_test] | |
39 | ||
40 | That corresponds to this line in `waveform_ai_timer()`: | |
41 | ||
42 | unsigned int chanspec = cmd->chanlist[async->cur_chan]; | |
43 | ||
44 | but `do_become_nonbusy()` frees `cmd->chanlist` and sets it to `NULL`. | |
45 | ||
46 | Fix the race by calling `del_timer_sync()` instead of `del_timer()` in | |
47 | `waveform_ai_cancel()` when not in an interrupt context. The only time | |
48 | `waveform_ai_cancel()` is called in an interrupt context is when it is | |
49 | called from the timer routine itself, via `comedi_handle_events()`. | |
50 | ||
51 | There is no longer any need for the `WAVEFORM_AI_RUNNING` flag, so get | |
52 | rid of it. | |
53 | ||
54 | The bug was copied from the AI subdevice to the AO when support for | |
55 | commands on the AO subdevice was added by commit 0cf55bbef2f9 ("staging: | |
56 | comedi: comedi_test: implement commands on AO subdevice"). That | |
57 | involves the timer routine `waveform_ao_timer()`, the comedi "cancel" | |
58 | routine `waveform_ao_cancel()`, and the flag `WAVEFORM_AO_RUNNING`. Fix | |
59 | it in the same way as for the AI subdevice. | |
60 | ||
61 | Fixes: 73e0e4dfed4c ("staging: comedi: comedi_test: fix timer lock-up") | |
62 | Fixes: 0cf55bbef2f9 ("staging: comedi: comedi_test: implement commands | |
63 | on AO subdevice") | |
64 | Reported-by: Éric Piel <piel@delmic.com> | |
65 | Signed-off-by: Ian Abbott <abbotti@mev.co.uk> | |
66 | Cc: Éric Piel <piel@delmic.com> | |
67 | Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> | |
68 | ||
69 | --- | |
70 | drivers/staging/comedi/drivers/comedi_test.c | 46 +++++++-------------------- | |
71 | 1 file changed, 12 insertions(+), 34 deletions(-) | |
72 | ||
73 | --- a/drivers/staging/comedi/drivers/comedi_test.c | |
74 | +++ b/drivers/staging/comedi/drivers/comedi_test.c | |
75 | @@ -56,11 +56,6 @@ | |
76 | ||
77 | #define N_CHANS 8 | |
78 | ||
79 | -enum waveform_state_bits { | |
80 | - WAVEFORM_AI_RUNNING, | |
81 | - WAVEFORM_AO_RUNNING | |
82 | -}; | |
83 | - | |
84 | /* Data unique to this driver */ | |
85 | struct waveform_private { | |
86 | struct timer_list ai_timer; /* timer for AI commands */ | |
87 | @@ -68,7 +63,6 @@ struct waveform_private { | |
88 | unsigned int wf_amplitude; /* waveform amplitude in microvolts */ | |
89 | unsigned int wf_period; /* waveform period in microseconds */ | |
90 | unsigned int wf_current; /* current time in waveform period */ | |
91 | - unsigned long state_bits; | |
92 | unsigned int ai_scan_period; /* AI scan period in usec */ | |
93 | unsigned int ai_convert_period; /* AI conversion period in usec */ | |
94 | struct timer_list ao_timer; /* timer for AO commands */ | |
95 | @@ -191,10 +185,6 @@ static void waveform_ai_timer(unsigned l | |
96 | unsigned int nsamples; | |
97 | unsigned int time_increment; | |
98 | ||
99 | - /* check command is still active */ | |
100 | - if (!test_bit(WAVEFORM_AI_RUNNING, &devpriv->state_bits)) | |
101 | - return; | |
102 | - | |
103 | now = ktime_to_us(ktime_get()); | |
104 | nsamples = comedi_nsamples_left(s, UINT_MAX); | |
105 | ||
106 | @@ -386,11 +376,6 @@ static int waveform_ai_cmd(struct comedi | |
107 | */ | |
108 | devpriv->ai_timer.expires = | |
109 | jiffies + usecs_to_jiffies(devpriv->ai_convert_period) + 1; | |
110 | - | |
111 | - /* mark command as active */ | |
112 | - smp_mb__before_atomic(); | |
113 | - set_bit(WAVEFORM_AI_RUNNING, &devpriv->state_bits); | |
114 | - smp_mb__after_atomic(); | |
115 | add_timer(&devpriv->ai_timer); | |
116 | return 0; | |
117 | } | |
118 | @@ -400,11 +385,12 @@ static int waveform_ai_cancel(struct com | |
119 | { | |
120 | struct waveform_private *devpriv = dev->private; | |
121 | ||
122 | - /* mark command as no longer active */ | |
123 | - clear_bit(WAVEFORM_AI_RUNNING, &devpriv->state_bits); | |
124 | - smp_mb__after_atomic(); | |
125 | - /* cannot call del_timer_sync() as may be called from timer routine */ | |
126 | - del_timer(&devpriv->ai_timer); | |
127 | + if (in_softirq()) { | |
128 | + /* Assume we were called from the timer routine itself. */ | |
129 | + del_timer(&devpriv->ai_timer); | |
130 | + } else { | |
131 | + del_timer_sync(&devpriv->ai_timer); | |
132 | + } | |
133 | return 0; | |
134 | } | |
135 | ||
136 | @@ -436,10 +422,6 @@ static void waveform_ao_timer(unsigned l | |
137 | u64 scans_since; | |
138 | unsigned int scans_avail = 0; | |
139 | ||
140 | - /* check command is still active */ | |
141 | - if (!test_bit(WAVEFORM_AO_RUNNING, &devpriv->state_bits)) | |
142 | - return; | |
143 | - | |
144 | /* determine number of scan periods since last time */ | |
145 | now = ktime_to_us(ktime_get()); | |
146 | scans_since = now - devpriv->ao_last_scan_time; | |
147 | @@ -518,11 +500,6 @@ static int waveform_ao_inttrig_start(str | |
148 | devpriv->ao_last_scan_time = ktime_to_us(ktime_get()); | |
149 | devpriv->ao_timer.expires = | |
150 | jiffies + usecs_to_jiffies(devpriv->ao_scan_period); | |
151 | - | |
152 | - /* mark command as active */ | |
153 | - smp_mb__before_atomic(); | |
154 | - set_bit(WAVEFORM_AO_RUNNING, &devpriv->state_bits); | |
155 | - smp_mb__after_atomic(); | |
156 | add_timer(&devpriv->ao_timer); | |
157 | ||
158 | return 1; | |
159 | @@ -608,11 +585,12 @@ static int waveform_ao_cancel(struct com | |
160 | struct waveform_private *devpriv = dev->private; | |
161 | ||
162 | s->async->inttrig = NULL; | |
163 | - /* mark command as no longer active */ | |
164 | - clear_bit(WAVEFORM_AO_RUNNING, &devpriv->state_bits); | |
165 | - smp_mb__after_atomic(); | |
166 | - /* cannot call del_timer_sync() as may be called from timer routine */ | |
167 | - del_timer(&devpriv->ao_timer); | |
168 | + if (in_softirq()) { | |
169 | + /* Assume we were called from the timer routine itself. */ | |
170 | + del_timer(&devpriv->ao_timer); | |
171 | + } else { | |
172 | + del_timer_sync(&devpriv->ao_timer); | |
173 | + } | |
174 | return 0; | |
175 | } | |
176 |