diff mbox series

[net] hsr: switch ->dellink() to ->ndo_uninit()

Message ID 20190710062454.16386-1-xiyou.wangcong@gmail.com
State Accepted
Delegated to: David Miller
Headers show
Series [net] hsr: switch ->dellink() to ->ndo_uninit() | expand

Commit Message

Cong Wang July 10, 2019, 6:24 a.m. UTC 
Switching from ->priv_destructor to dellink() has an unexpected
consequence: existing RCU readers, that is, hsr_port_get_hsr()
callers, may still be able to read the port list.

Instead of checking the return value of each hsr_port_get_hsr(),
we can just move it to ->ndo_uninit() which is called after
device unregister and synchronize_net(), and we still have RTNL
lock there.

Fixes: b9a1e627405d ("hsr: implement dellink to clean up resources")
Fixes: edf070a0fb45 ("hsr: fix a NULL pointer deref in hsr_dev_xmit()")
Reported-by: syzbot+097ef84cdc95843fbaa8@syzkaller.appspotmail.com
Cc: Arvid Brodin <arvid.brodin@alten.se>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/hsr/hsr_device.c  | 18 ++++++++----------
 net/hsr/hsr_device.h  |  1 -
 net/hsr/hsr_netlink.c |  7 -------
 3 files changed, 8 insertions(+), 18 deletions(-)

Comments

David Miller July 11, 2019, 9:38 p.m. UTC | #1 
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue,  9 Jul 2019 23:24:54 -0700

> Switching from ->priv_destructor to dellink() has an unexpected
> consequence: existing RCU readers, that is, hsr_port_get_hsr()
> callers, may still be able to read the port list.
> 
> Instead of checking the return value of each hsr_port_get_hsr(),
> we can just move it to ->ndo_uninit() which is called after
> device unregister and synchronize_net(), and we still have RTNL
> lock there.
> 
> Fixes: b9a1e627405d ("hsr: implement dellink to clean up resources")
> Fixes: edf070a0fb45 ("hsr: fix a NULL pointer deref in hsr_dev_xmit()")
> Reported-by: syzbot+097ef84cdc95843fbaa8@syzkaller.appspotmail.com
> Cc: Arvid Brodin <arvid.brodin@alten.se>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied and queued up for -stable, thanks.
diff mbox series

Patch

diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index f0f9b493c47b..f509b495451a 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -227,13 +227,8 @@  static int hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct hsr_port *master;
 
 	master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
-	if (master) {
-		skb->dev = master->dev;
-		hsr_forward_skb(skb, master);
-	} else {
-		atomic_long_inc(&dev->tx_dropped);
-		dev_kfree_skb_any(skb);
-	}
+	skb->dev = master->dev;
+	hsr_forward_skb(skb, master);
 	return NETDEV_TX_OK;
 }
 
@@ -348,7 +343,11 @@  static void hsr_announce(struct timer_list *t)
 	rcu_read_unlock();
 }
 
-void hsr_dev_destroy(struct net_device *hsr_dev)
+/* This has to be called after all the readers are gone.
+ * Otherwise we would have to check the return value of
+ * hsr_port_get_hsr().
+ */
+static void hsr_dev_destroy(struct net_device *hsr_dev)
 {
 	struct hsr_priv *hsr;
 	struct hsr_port *port;
@@ -364,8 +363,6 @@  void hsr_dev_destroy(struct net_device *hsr_dev)
 	del_timer_sync(&hsr->prune_timer);
 	del_timer_sync(&hsr->announce_timer);
 
-	synchronize_rcu();
-
 	hsr_del_self_node(&hsr->self_node_db);
 	hsr_del_nodes(&hsr->node_db);
 }
@@ -376,6 +373,7 @@  static const struct net_device_ops hsr_device_ops = {
 	.ndo_stop = hsr_dev_close,
 	.ndo_start_xmit = hsr_dev_xmit,
 	.ndo_fix_features = hsr_fix_features,
+	.ndo_uninit = hsr_dev_destroy,
 };
 
 static struct device_type hsr_type = {
diff --git a/net/hsr/hsr_device.h b/net/hsr/hsr_device.h
index d0fa6b0696d2..6d7759c4f5f9 100644
--- a/net/hsr/hsr_device.h
+++ b/net/hsr/hsr_device.h
@@ -14,7 +14,6 @@ 
 void hsr_dev_setup(struct net_device *dev);
 int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
 		     unsigned char multicast_spec, u8 protocol_version);
-void hsr_dev_destroy(struct net_device *hsr_dev);
 void hsr_check_carrier_and_operstate(struct hsr_priv *hsr);
 bool is_hsr_master(struct net_device *dev);
 int hsr_get_max_mtu(struct hsr_priv *hsr);
diff --git a/net/hsr/hsr_netlink.c b/net/hsr/hsr_netlink.c
index 160edd24de4e..8f8337f893ba 100644
--- a/net/hsr/hsr_netlink.c
+++ b/net/hsr/hsr_netlink.c
@@ -69,12 +69,6 @@  static int hsr_newlink(struct net *src_net, struct net_device *dev,
 	return hsr_dev_finalize(dev, link, multicast_spec, hsr_version);
 }
 
-static void hsr_dellink(struct net_device *hsr_dev, struct list_head *head)
-{
-	hsr_dev_destroy(hsr_dev);
-	unregister_netdevice_queue(hsr_dev, head);
-}
-
 static int hsr_fill_info(struct sk_buff *skb, const struct net_device *dev)
 {
 	struct hsr_priv *hsr;
@@ -119,7 +113,6 @@  static struct rtnl_link_ops hsr_link_ops __read_mostly = {
 	.priv_size	= sizeof(struct hsr_priv),
 	.setup		= hsr_dev_setup,
 	.newlink	= hsr_newlink,
-	.dellink	= hsr_dellink,
 	.fill_info	= hsr_fill_info,
 };