]>
Commit | Line | Data |
---|---|---|
abfbd08a GKH |
1 | From a41de97eaa03dedbe170919b95ce161e0dce818d Mon Sep 17 00:00:00 2001 |
2 | From: "nikolay@redhat.com" <nikolay@redhat.com> | |
3 | Date: Thu, 29 Nov 2012 01:31:31 +0000 | |
4 | Subject: bonding: fix miimon and arp_interval delayed work race conditions | |
5 | ||
6 | ||
7 | From: "nikolay@redhat.com" <nikolay@redhat.com> | |
8 | ||
9 | [ Upstream commit fbb0c41b814d497c656fc7be9e35456f139cb2fb ] | |
10 | ||
11 | First I would give three observations which will be used later. | |
12 | Observation 1: if (delayed_work_pending(wq)) cancel_delayed_work(wq) | |
13 | This usage is wrong because the pending bit is cleared just before the | |
14 | work's fn is executed and if the function re-arms itself we might end up | |
15 | with the work still running. It's safe to call cancel_delayed_work_sync() | |
16 | even if the work is not queued at all. | |
17 | Observation 2: Use of INIT_DELAYED_WORK() | |
18 | Work needs to be initialized only once prior to (de/en)queueing. | |
19 | Observation 3: IFF_UP is set only after ndo_open is called | |
20 | ||
21 | Related race conditions: | |
22 | 1. Race between bonding_store_miimon() and bonding_store_arp_interval() | |
23 | Because of Obs.1 we can end up having both works enqueued. | |
24 | 2. Multiple races with INIT_DELAYED_WORK() | |
25 | Since the works are not protected by anything between INIT_DELAYED_WORK() | |
26 | and calls to (en/de)queue it is possible for races between the following | |
27 | functions: | |
28 | (races are also possible between the calls to INIT_DELAYED_WORK() | |
29 | and workqueue code) | |
30 | bonding_store_miimon() - bonding_store_arp_interval(), bond_close(), | |
31 | bond_open(), enqueued functions | |
32 | bonding_store_arp_interval() - bonding_store_miimon(), bond_close(), | |
33 | bond_open(), enqueued functions | |
34 | 3. By Obs.1 we need to change bond_cancel_all() | |
35 | ||
36 | Bugs 1 and 2 are fixed by moving all work initializations in bond_open | |
37 | which by Obs. 2 and Obs. 3 and the fact that we make sure that all works | |
38 | are cancelled in bond_close(), is guaranteed not to have any work | |
39 | enqueued. | |
40 | Also RTNL lock is now acquired in bonding_store_miimon/arp_interval so | |
41 | they can't race with bond_close and bond_open. The opposing work is | |
42 | cancelled only if the IFF_UP flag is set and it is cancelled | |
43 | unconditionally. The opposing work is already cancelled if the interface | |
44 | is down so no need to cancel it again. This way we don't need new | |
45 | synchronizations for the bonding workqueue. These bugs (and fixes) are | |
46 | tied together and belong in the same patch. | |
47 | Note: I have left 1 line intentionally over 80 characters (84) because I | |
48 | didn't like how it looks broken down. If you'd prefer it otherwise, | |
49 | then simply break it. | |
50 | ||
51 | v2: Make description text < 75 columns | |
52 | ||
53 | Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com> | |
54 | Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> | |
55 | Signed-off-by: David S. Miller <davem@davemloft.net> | |
56 | Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> | |
57 | --- | |
58 | drivers/net/bonding/bond_main.c | 88 +++++++++++---------------------------- | |
59 | drivers/net/bonding/bond_sysfs.c | 34 ++++----------- | |
60 | 2 files changed, 36 insertions(+), 86 deletions(-) | |
61 | ||
62 | --- a/drivers/net/bonding/bond_main.c | |
63 | +++ b/drivers/net/bonding/bond_main.c | |
64 | @@ -3398,6 +3398,28 @@ static int bond_xmit_hash_policy_l2(stru | |
65 | ||
66 | /*-------------------------- Device entry points ----------------------------*/ | |
67 | ||
68 | +static void bond_work_init_all(struct bonding *bond) | |
69 | +{ | |
70 | + INIT_DELAYED_WORK(&bond->mcast_work, | |
71 | + bond_resend_igmp_join_requests_delayed); | |
72 | + INIT_DELAYED_WORK(&bond->alb_work, bond_alb_monitor); | |
73 | + INIT_DELAYED_WORK(&bond->mii_work, bond_mii_monitor); | |
74 | + if (bond->params.mode == BOND_MODE_ACTIVEBACKUP) | |
75 | + INIT_DELAYED_WORK(&bond->arp_work, bond_activebackup_arp_mon); | |
76 | + else | |
77 | + INIT_DELAYED_WORK(&bond->arp_work, bond_loadbalance_arp_mon); | |
78 | + INIT_DELAYED_WORK(&bond->ad_work, bond_3ad_state_machine_handler); | |
79 | +} | |
80 | + | |
81 | +static void bond_work_cancel_all(struct bonding *bond) | |
82 | +{ | |
83 | + cancel_delayed_work_sync(&bond->mii_work); | |
84 | + cancel_delayed_work_sync(&bond->arp_work); | |
85 | + cancel_delayed_work_sync(&bond->alb_work); | |
86 | + cancel_delayed_work_sync(&bond->ad_work); | |
87 | + cancel_delayed_work_sync(&bond->mcast_work); | |
88 | +} | |
89 | + | |
90 | static int bond_open(struct net_device *bond_dev) | |
91 | { | |
92 | struct bonding *bond = netdev_priv(bond_dev); | |
93 | @@ -3420,41 +3442,27 @@ static int bond_open(struct net_device * | |
94 | } | |
95 | read_unlock(&bond->lock); | |
96 | ||
97 | - INIT_DELAYED_WORK(&bond->mcast_work, bond_resend_igmp_join_requests_delayed); | |
98 | + bond_work_init_all(bond); | |
99 | ||
100 | if (bond_is_lb(bond)) { | |
101 | /* bond_alb_initialize must be called before the timer | |
102 | * is started. | |
103 | */ | |
104 | - if (bond_alb_initialize(bond, (bond->params.mode == BOND_MODE_ALB))) { | |
105 | - /* something went wrong - fail the open operation */ | |
106 | + if (bond_alb_initialize(bond, (bond->params.mode == BOND_MODE_ALB))) | |
107 | return -ENOMEM; | |
108 | - } | |
109 | - | |
110 | - INIT_DELAYED_WORK(&bond->alb_work, bond_alb_monitor); | |
111 | queue_delayed_work(bond->wq, &bond->alb_work, 0); | |
112 | } | |
113 | ||
114 | - if (bond->params.miimon) { /* link check interval, in milliseconds. */ | |
115 | - INIT_DELAYED_WORK(&bond->mii_work, bond_mii_monitor); | |
116 | + if (bond->params.miimon) /* link check interval, in milliseconds. */ | |
117 | queue_delayed_work(bond->wq, &bond->mii_work, 0); | |
118 | - } | |
119 | ||
120 | if (bond->params.arp_interval) { /* arp interval, in milliseconds. */ | |
121 | - if (bond->params.mode == BOND_MODE_ACTIVEBACKUP) | |
122 | - INIT_DELAYED_WORK(&bond->arp_work, | |
123 | - bond_activebackup_arp_mon); | |
124 | - else | |
125 | - INIT_DELAYED_WORK(&bond->arp_work, | |
126 | - bond_loadbalance_arp_mon); | |
127 | - | |
128 | queue_delayed_work(bond->wq, &bond->arp_work, 0); | |
129 | if (bond->params.arp_validate) | |
130 | bond->recv_probe = bond_arp_rcv; | |
131 | } | |
132 | ||
133 | if (bond->params.mode == BOND_MODE_8023AD) { | |
134 | - INIT_DELAYED_WORK(&bond->ad_work, bond_3ad_state_machine_handler); | |
135 | queue_delayed_work(bond->wq, &bond->ad_work, 0); | |
136 | /* register to receive LACPDUs */ | |
137 | bond->recv_probe = bond_3ad_lacpdu_recv; | |
138 | @@ -3469,34 +3477,10 @@ static int bond_close(struct net_device | |
139 | struct bonding *bond = netdev_priv(bond_dev); | |
140 | ||
141 | write_lock_bh(&bond->lock); | |
142 | - | |
143 | bond->send_peer_notif = 0; | |
144 | - | |
145 | write_unlock_bh(&bond->lock); | |
146 | ||
147 | - if (bond->params.miimon) { /* link check interval, in milliseconds. */ | |
148 | - cancel_delayed_work_sync(&bond->mii_work); | |
149 | - } | |
150 | - | |
151 | - if (bond->params.arp_interval) { /* arp interval, in milliseconds. */ | |
152 | - cancel_delayed_work_sync(&bond->arp_work); | |
153 | - } | |
154 | - | |
155 | - switch (bond->params.mode) { | |
156 | - case BOND_MODE_8023AD: | |
157 | - cancel_delayed_work_sync(&bond->ad_work); | |
158 | - break; | |
159 | - case BOND_MODE_TLB: | |
160 | - case BOND_MODE_ALB: | |
161 | - cancel_delayed_work_sync(&bond->alb_work); | |
162 | - break; | |
163 | - default: | |
164 | - break; | |
165 | - } | |
166 | - | |
167 | - if (delayed_work_pending(&bond->mcast_work)) | |
168 | - cancel_delayed_work_sync(&bond->mcast_work); | |
169 | - | |
170 | + bond_work_cancel_all(bond); | |
171 | if (bond_is_lb(bond)) { | |
172 | /* Must be called only after all | |
173 | * slaves have been released | |
174 | @@ -4375,26 +4359,6 @@ static void bond_setup(struct net_device | |
175 | bond_dev->features |= bond_dev->hw_features; | |
176 | } | |
177 | ||
178 | -static void bond_work_cancel_all(struct bonding *bond) | |
179 | -{ | |
180 | - if (bond->params.miimon && delayed_work_pending(&bond->mii_work)) | |
181 | - cancel_delayed_work_sync(&bond->mii_work); | |
182 | - | |
183 | - if (bond->params.arp_interval && delayed_work_pending(&bond->arp_work)) | |
184 | - cancel_delayed_work_sync(&bond->arp_work); | |
185 | - | |
186 | - if (bond->params.mode == BOND_MODE_ALB && | |
187 | - delayed_work_pending(&bond->alb_work)) | |
188 | - cancel_delayed_work_sync(&bond->alb_work); | |
189 | - | |
190 | - if (bond->params.mode == BOND_MODE_8023AD && | |
191 | - delayed_work_pending(&bond->ad_work)) | |
192 | - cancel_delayed_work_sync(&bond->ad_work); | |
193 | - | |
194 | - if (delayed_work_pending(&bond->mcast_work)) | |
195 | - cancel_delayed_work_sync(&bond->mcast_work); | |
196 | -} | |
197 | - | |
198 | /* | |
199 | * Destroy a bonding device. | |
200 | * Must be under rtnl_lock when this function is called. | |
201 | --- a/drivers/net/bonding/bond_sysfs.c | |
202 | +++ b/drivers/net/bonding/bond_sysfs.c | |
203 | @@ -518,6 +518,8 @@ static ssize_t bonding_store_arp_interva | |
204 | int new_value, ret = count; | |
205 | struct bonding *bond = to_bond(d); | |
206 | ||
207 | + if (!rtnl_trylock()) | |
208 | + return restart_syscall(); | |
209 | if (sscanf(buf, "%d", &new_value) != 1) { | |
210 | pr_err("%s: no arp_interval value specified.\n", | |
211 | bond->dev->name); | |
212 | @@ -544,10 +546,6 @@ static ssize_t bonding_store_arp_interva | |
213 | pr_info("%s: ARP monitoring cannot be used with MII monitoring. %s Disabling MII monitoring.\n", | |
214 | bond->dev->name, bond->dev->name); | |
215 | bond->params.miimon = 0; | |
216 | - if (delayed_work_pending(&bond->mii_work)) { | |
217 | - cancel_delayed_work(&bond->mii_work); | |
218 | - flush_workqueue(bond->wq); | |
219 | - } | |
220 | } | |
221 | if (!bond->params.arp_targets[0]) { | |
222 | pr_info("%s: ARP monitoring has been set up, but no ARP targets have been specified.\n", | |
223 | @@ -559,19 +557,12 @@ static ssize_t bonding_store_arp_interva | |
224 | * timer will get fired off when the open function | |
225 | * is called. | |
226 | */ | |
227 | - if (!delayed_work_pending(&bond->arp_work)) { | |
228 | - if (bond->params.mode == BOND_MODE_ACTIVEBACKUP) | |
229 | - INIT_DELAYED_WORK(&bond->arp_work, | |
230 | - bond_activebackup_arp_mon); | |
231 | - else | |
232 | - INIT_DELAYED_WORK(&bond->arp_work, | |
233 | - bond_loadbalance_arp_mon); | |
234 | - | |
235 | - queue_delayed_work(bond->wq, &bond->arp_work, 0); | |
236 | - } | |
237 | + cancel_delayed_work_sync(&bond->mii_work); | |
238 | + queue_delayed_work(bond->wq, &bond->arp_work, 0); | |
239 | } | |
240 | ||
241 | out: | |
242 | + rtnl_unlock(); | |
243 | return ret; | |
244 | } | |
245 | static DEVICE_ATTR(arp_interval, S_IRUGO | S_IWUSR, | |
246 | @@ -967,6 +958,8 @@ static ssize_t bonding_store_miimon(stru | |
247 | int new_value, ret = count; | |
248 | struct bonding *bond = to_bond(d); | |
249 | ||
250 | + if (!rtnl_trylock()) | |
251 | + return restart_syscall(); | |
252 | if (sscanf(buf, "%d", &new_value) != 1) { | |
253 | pr_err("%s: no miimon value specified.\n", | |
254 | bond->dev->name); | |
255 | @@ -998,10 +991,6 @@ static ssize_t bonding_store_miimon(stru | |
256 | bond->params.arp_validate = | |
257 | BOND_ARP_VALIDATE_NONE; | |
258 | } | |
259 | - if (delayed_work_pending(&bond->arp_work)) { | |
260 | - cancel_delayed_work(&bond->arp_work); | |
261 | - flush_workqueue(bond->wq); | |
262 | - } | |
263 | } | |
264 | ||
265 | if (bond->dev->flags & IFF_UP) { | |
266 | @@ -1010,15 +999,12 @@ static ssize_t bonding_store_miimon(stru | |
267 | * timer will get fired off when the open function | |
268 | * is called. | |
269 | */ | |
270 | - if (!delayed_work_pending(&bond->mii_work)) { | |
271 | - INIT_DELAYED_WORK(&bond->mii_work, | |
272 | - bond_mii_monitor); | |
273 | - queue_delayed_work(bond->wq, | |
274 | - &bond->mii_work, 0); | |
275 | - } | |
276 | + cancel_delayed_work_sync(&bond->arp_work); | |
277 | + queue_delayed_work(bond->wq, &bond->mii_work, 0); | |
278 | } | |
279 | } | |
280 | out: | |
281 | + rtnl_unlock(); | |
282 | return ret; | |
283 | } | |
284 | static DEVICE_ATTR(miimon, S_IRUGO | S_IWUSR, |