summaryrefslogtreecommitdiff
blob: 2038b2f0fdca5aaddd443808da339c21d29c968b (plain)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
From 3c701b60dc169a24cf52c0397560125ddce3d54c Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 7 Sep 2016 23:47:14 +0200
Subject: [PATCH 1/2] device: workaround driver issue with delayed change of
 MAC address

brcmfmac and possibly other drivers don't change the MAC address
right away, but instead the result is delayed. That is problematic
because we cannot continue activation before the MAC address is
settled.

Add a hack to workaround the issue by waiting until the MAC address
changed.

The previous attempt to workaround this was less intrusive: we would
just refresh the link once and check the result. But that turns out
not to be sufficent for all cases. Now, wait and poll.

https://bugzilla.gnome.org/show_bug.cgi?id=770456
https://bugzilla.redhat.com/show_bug.cgi?id=1374023
(cherry picked from commit 1a85103765d4eaa0acab6b03658a4f9cfe684a64)
(cherry picked from commit 8d575403685208aad75f918484ae7adbc1a46085)
---
 src/devices/nm-device.c | 85 ++++++++++++++++++++++++++++++++++---------------
 src/devices/nm-device.h |  2 +-
 2 files changed, 61 insertions(+), 26 deletions(-)

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 674563f..3e549c5 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -11549,16 +11549,17 @@ nm_device_get_hw_address (NMDevice *self)
 	return priv->hw_addr;
 }
 
-void
+gboolean
 nm_device_update_hw_address (NMDevice *self)
 {
 	NMDevicePrivate *priv;
 	const guint8 *hwaddr;
 	gsize hwaddrlen = 0;
+	gboolean changed = FALSE;
 
 	priv = NM_DEVICE_GET_PRIVATE (self);
 	if (priv->ifindex <= 0)
-		return;
+		return FALSE;
 
 	hwaddr = nm_platform_link_get_address (NM_PLATFORM_GET, priv->ifindex, &hwaddrlen);
 
@@ -11585,6 +11586,7 @@ nm_device_update_hw_address (NMDevice *self)
 				 * update our inital hw-address as well. */
 				nm_device_update_initial_hw_address (self);
 			}
+			changed = TRUE;
 		}
 	} else {
 		/* Invalid or no hardware address */
@@ -11597,6 +11599,7 @@ nm_device_update_hw_address (NMDevice *self)
 			       "hw-addr: failed reading current MAC address");
 		}
 	}
+	return changed;
 }
 
 void
@@ -11756,6 +11759,15 @@ nm_device_hw_addr_is_explict (NMDevice *self)
 }
 
 static gboolean
+_hw_addr_matches (NMDevice *self, const char *addr)
+{
+	const char *cur_addr;
+
+	cur_addr = nm_device_get_hw_address (self);
+	return cur_addr && nm_utils_hwaddr_matches (cur_addr, -1, addr, -1);
+}
+
+static gboolean
 _hw_addr_set (NMDevice *self,
               const char *addr,
               const char *operation,
@@ -11765,7 +11777,6 @@ _hw_addr_set (NMDevice *self,
 	gboolean success = FALSE;
 	gboolean needs_refresh = FALSE;
 	NMPlatformError plerr;
-	const char *cur_addr;
 	guint8 addr_bytes[NM_UTILS_HWADDR_LEN_MAX];
 	guint hw_addr_len;
 	gboolean was_up;
@@ -11776,11 +11787,9 @@ _hw_addr_set (NMDevice *self,
 
 	priv = NM_DEVICE_GET_PRIVATE (self);
 
-	cur_addr = nm_device_get_hw_address (self);
-
 	/* Do nothing if current MAC is same */
-	if (cur_addr && nm_utils_hwaddr_matches (cur_addr, -1, addr, -1)) {
-		_LOGT (LOGD_DEVICE, "set-hw-addr: no MAC address change needed (%s)", cur_addr);
+	if (_hw_addr_matches (self, addr)) {
+		_LOGT (LOGD_DEVICE, "set-hw-addr: no MAC address change needed (%s)", addr);
 		return TRUE;
 	}
 
@@ -11804,8 +11813,7 @@ _hw_addr_set (NMDevice *self,
 	if (success) {
 		/* MAC address succesfully changed; update the current MAC to match */
 		nm_device_update_hw_address (self);
-		cur_addr = nm_device_get_hw_address (self);
-		if (cur_addr && nm_utils_hwaddr_matches (cur_addr, -1, addr, -1)) {
+		if (_hw_addr_matches (self, addr)) {
 			_LOGI (LOGD_DEVICE, "set-hw-addr: %s MAC address to %s (%s)",
 			       operation, addr, detail);
 		} else {
@@ -11827,24 +11835,51 @@ _hw_addr_set (NMDevice *self,
 	}
 
 	if (needs_refresh) {
-		/* The platform call indicated success, however the address is not
-		 * as expected. May be a kernel issue and the MAC address takes
-		 * a moment to change (bgo#770456).
-		 *
-		 * Try to reload the link and check again. */
-		nm_platform_link_refresh (NM_PLATFORM_GET, nm_device_get_ip_ifindex (self));
-
-		nm_device_update_hw_address (self);
-		cur_addr = nm_device_get_hw_address (self);
-		if (cur_addr && nm_utils_hwaddr_matches (cur_addr, -1, addr, -1)) {
-			_LOGI (LOGD_DEVICE, "set-hw-addr: %s MAC address to %s (%s)",
-			       operation, addr, detail);
+		if (_hw_addr_matches (self, addr)) {
+			/* the MAC address already changed during nm_device_bring_up() above. */
 		} else {
-			_LOGW (LOGD_DEVICE,
-			       "set-hw-addr: new MAC address %s not successfully %s (%s)",
-			       addr, operation, detail);
-			return FALSE;
+			gint64 poll_end, now;
+
+			/* The platform call indicated success, however the address is not
+			 * as expected. That is either due to a driver issue (brcmfmac, bgo#770456,
+			 * rh#1374023) or a race where externally the MAC address was reset.
+			 * The race is rather unlikely.
+			 *
+			 * The alternative would be to postpone the activation in case the
+			 * MAC address is not yet ready and poll without blocking. However,
+			 * that is rather complicated and it is not expected that this case
+			 * happens for regular drivers.
+			 * Note that brcmfmac can block NetworkManager for 500 msec while
+			 * taking down the device. Let's add annother 100 msec to that.
+			 *
+			 * wait/poll up to 100 msec until it changes. */
+
+			poll_end = nm_utils_get_monotonic_timestamp_us () + (100 * 1000);
+			for (;;) {
+				if (!nm_platform_link_refresh (NM_PLATFORM_GET, nm_device_get_ip_ifindex (self)))
+					goto handle_fail;
+				if (!nm_device_update_hw_address (self))
+					goto handle_wait;
+				if (!_hw_addr_matches (self, addr))
+					goto handle_fail;
+
+				break;
+handle_wait:
+				now = nm_utils_get_monotonic_timestamp_us ();
+				if (now < poll_end) {
+					g_usleep (NM_MIN (poll_end - now, 500));
+					continue;
+				}
+handle_fail:
+				_LOGW (LOGD_DEVICE,
+				       "set-hw-addr: new MAC address %s not successfully %s (%s)",
+				       addr, operation, detail);
+				return FALSE;
+			}
 		}
+
+		_LOGI (LOGD_DEVICE, "set-hw-addr: %s MAC address to %s (%s)",
+		       operation, addr, detail);
 	}
 
 	return success;
diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h
index be12ce7..a757a37 100644
--- a/src/devices/nm-device.h
+++ b/src/devices/nm-device.h
@@ -588,7 +588,7 @@ void nm_device_reactivate_ip6_config (NMDevice *device,
                                       NMSettingIPConfig *s_ip6_old,
                                       NMSettingIPConfig *s_ip6_new);
 
-void nm_device_update_hw_address (NMDevice *self);
+gboolean nm_device_update_hw_address (NMDevice *self);
 void nm_device_update_initial_hw_address (NMDevice *self);
 void nm_device_update_permanent_hw_address (NMDevice *self);
 void nm_device_update_dynamic_ip_setup (NMDevice *self);
-- 
2.7.4


From d99c3b63e81347fb861e1306eb0b603cfa15223e Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Sun, 11 Sep 2016 09:48:56 +0200
Subject: [PATCH 2/2] device: wait for MAC address change to complete before
 setting interface up

Some drivers (brcmfmac) don't change the MAC address right away.
NetworkManager works around that by waiting synchronously until
the address changes (commit 1a85103765d4eaa0acab6b03658a4f9cfe684a64).

wpa_supplicant on the other hand, only re-reads the MAC address
when changing state from DISABLED to ENABLED, which happens when
the interface comes up.

That is a bug in wpa_supplicant and the driver, but we can work-around by
waiting until the MAC address actually changed before setting the interface
IFF_UP. Also note, that there is still a race in wpa_supplicant which might
miss a change to DISABLED state altogether.

https://bugzilla.gnome.org/show_bug.cgi?id=770504
https://bugzilla.redhat.com/show_bug.cgi?id=1374023
(cherry picked from commit 32f7c1d4b9aba597a99128631f07c2985149f303)
(cherry picked from commit cd8f2ecc617a896d8007e6fe825c676a626a3b8d)
---
 src/devices/nm-device.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 3e549c5..47e858b 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -11829,12 +11829,8 @@ _hw_addr_set (NMDevice *self,
 		        nm_platform_error_to_string (plerr));
 	}
 
-	if (was_up) {
-		if (!nm_device_bring_up (self, TRUE, NULL))
-			return FALSE;
-	}
-
 	if (needs_refresh) {
+		success = TRUE;
 		if (_hw_addr_matches (self, addr)) {
 			/* the MAC address already changed during nm_device_bring_up() above. */
 		} else {
@@ -11871,15 +11867,24 @@ handle_wait:
 					continue;
 				}
 handle_fail:
-				_LOGW (LOGD_DEVICE,
-				       "set-hw-addr: new MAC address %s not successfully %s (%s)",
-				       addr, operation, detail);
-				return FALSE;
+				success = FALSE;
+				break;
 			}
 		}
 
-		_LOGI (LOGD_DEVICE, "set-hw-addr: %s MAC address to %s (%s)",
-		       operation, addr, detail);
+		if (success) {
+			_LOGI (LOGD_DEVICE, "set-hw-addr: %s MAC address to %s (%s)",
+			       operation, addr, detail);
+		} else {
+			_LOGW (LOGD_DEVICE,
+			       "set-hw-addr: new MAC address %s not successfully %s (%s)",
+			       addr, operation, detail);
+		}
+	}
+
+	if (was_up) {
+		if (!nm_device_bring_up (self, TRUE, NULL))
+			return FALSE;
 	}
 
 	return success;
-- 
2.7.4