wext: fix message delay/ordering

Commit Message

Johannes Berg Jan. 27, 2016, 12:14 p.m. UTC

From: Johannes Berg <[e-mail]>

Beniamino reported that he was getting an RTM_NEWLINK message for a
given interface, after the RTM_DELLINK for it. It turns out that the
message is a wireless extensions message, which was sent because the
interface had been connected and disconnection while it was deleted
caused a wext message.

It's strange that wext uses RTM_NEWLINK, and the message is somewhat
malformed without any proper attributes, causing "ip monitor link"
to print just rudimentary information:

5: wlan1: <BROADCAST,MULTICAST> mtu 1500 qdisc mq state DOWN group default
    link/ether 02:00:00:00:01:00 brd ff:ff:ff:ff:ff:ff
Deleted 5: wlan1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
    link/ether 02:00:00:00:01:00 brd ff:ff:ff:ff:ff:ff
5: wlan1: <BROADCAST,MULTICAST,UP>
    link/ether
(from my hwsim reproduction)

The reason for this is that wext schedules a worker to send out the
messages, and the scheduling delay can cause the messages to get out
to userspace after the RTM_DELLINK.

To fix this, have wext register a netdevice notifier and flush out
any pending messages when a netdevice is unregistered.

Cc: [e-mail]
Reported-by: Beniamino Galvani <[e-mail]>
Signed-off-by: Johannes Berg <[e-mail]>
---
 net/wireless/wext-core.c | 48 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 10 deletions(-)

Comments

Johannes Berg Jan. 27, 2016, 12:17 p.m. UTC | #1

On Wed, 2016-01-27 at 13:14 +0100, Johannes Berg wrote:
> 
> +static int wext_netdev_notifier_call(struct notifier_block *nb,
> +				     unsigned long state, void *ptr)
> +{
> +	/*
> +	 * When a netdev is unregistered, flush all pending messages
> +	 * to avoid them going out after the RTM_DELLINK, which can
> +	 * happen due to the schedule_work().
> +	 */
> +	if (state == NETDEV_UNREGISTER)
> +		wireless_nlevent_flush();
> 
It could be argued that the state check isn't really necessary and
should be removed to avoid ordering issues with up/down vs. wext, but
this fixes the really strange issue where you get an RTM_NEWLINK after
RTM_DELLINK (with the same ifidx), and I don't see how any software
would care much about the ordering otherwise.

johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Johannes Berg Jan. 27, 2016, 12:18 p.m. UTC | #2

> > +	if (state == NETDEV_UNREGISTER)
> > +		wireless_nlevent_flush();
> > 
> It could be argued that the state check isn't really necessary and
> should be removed to avoid ordering issues with up/down vs. wext, but
> this fixes the really strange issue where you get an RTM_NEWLINK
> after
> RTM_DELLINK (with the same ifidx), and I don't see how any software
> would care much about the ordering otherwise.
> 

Actually though, with the fix I still get:

5: wlan1: <BROADCAST,MULTICAST> mtu 1500 qdisc mq state DOWN group default 
    link/ether 02:00:00:00:01:00 brd ff:ff:ff:ff:ff:ff
5: wlan1: <BROADCAST,MULTICAST,UP> 
    link/ether 
Deleted 5: wlan1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default 
    link/ether 02:00:00:00:01:00 brd ff:ff:ff:ff:ff:ff

which is clearly odd (see the UP flag), so I'll change it.

johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Johannes Berg Jan. 27, 2016, 12:28 p.m. UTC | #3

On Wed, 2016-01-27 at 13:18 +0100, Johannes Berg wrote:
> > > +	if (state == NETDEV_UNREGISTER)
> > > +		wireless_nlevent_flush();
> > > 
> > It could be argued that the state check isn't really necessary and
> > should be removed to avoid ordering issues with up/down vs. wext,
> > but
> > this fixes the really strange issue where you get an RTM_NEWLINK
> > after
> > RTM_DELLINK (with the same ifidx), and I don't see how any software
> > would care much about the ordering otherwise.
> > 
> 
> Actually though, with the fix I still get:
> 
> 5: wlan1: <BROADCAST,MULTICAST> mtu 1500 qdisc mq state DOWN group
> default 
>     link/ether 02:00:00:00:01:00 brd ff:ff:ff:ff:ff:ff
> 5: wlan1: <BROADCAST,MULTICAST,UP> 
>     link/ether 
> Deleted 5: wlan1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state
> DOWN group default 
>     link/ether 02:00:00:00:01:00 brd ff:ff:ff:ff:ff:ff
> 
> which is clearly odd (see the UP flag), so I'll change it.
> 

Doesn't help, since the wext netdev notifier is registered earlier and
thus runs earlier than the cfg80211 one that triggers the action ...

I'll fix that differently then.

johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
index c8717c1d082e..4e4e1c5236cc 100644
--- a/net/wireless/wext-core.c
+++ b/net/wireless/wext-core.c
@@ -342,6 +342,38 @@  static const int compat_event_type_size[] = {
 
 /* IW event code */
 
+static void wireless_nlevent_flush(void)
+{
+	struct sk_buff *skb;
+	struct net *net;
+
+	ASSERT_RTNL();
+
+	for_each_net(net) {
+		while ((skb = skb_dequeue(&net->wext_nlevents)))
+			rtnl_notify(skb, net, 0, RTNLGRP_LINK, NULL,
+				    GFP_KERNEL);
+	}
+}
+
+static int wext_netdev_notifier_call(struct notifier_block *nb,
+				     unsigned long state, void *ptr)
+{
+	/*
+	 * When a netdev is unregistered, flush all pending messages
+	 * to avoid them going out after the RTM_DELLINK, which can
+	 * happen due to the schedule_work().
+	 */
+	if (state == NETDEV_UNREGISTER)
+		wireless_nlevent_flush();
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block wext_netdev_notifier = {
+	.notifier_call = wext_netdev_notifier_call,
+};
+
 static int __net_init wext_pernet_init(struct net *net)
 {
 	skb_queue_head_init(&net->wext_nlevents);
@@ -360,6 +392,11 @@  static struct pernet_operations wext_pernet_ops = {
 
 static int __init wireless_nlevent_init(void)
 {
+	int err = register_netdevice_notifier(&wext_netdev_notifier);
+
+	if (err)
+		return err;
+
 	return register_pernet_subsys(&wext_pernet_ops);
 }
 
@@ -368,17 +405,8 @@  subsys_initcall(wireless_nlevent_init);
 /* Process events generated by the wireless layer or the driver. */
 static void wireless_nlevent_process(struct work_struct *work)
 {
-	struct sk_buff *skb;
-	struct net *net;
-
 	rtnl_lock();
-
-	for_each_net(net) {
-		while ((skb = skb_dequeue(&net->wext_nlevents)))
-			rtnl_notify(skb, net, 0, RTNLGRP_LINK, NULL,
-				    GFP_KERNEL);
-	}
-
+	wireless_nlevent_flush();
 	rtnl_unlock();
 }