| Message ID | 20191130142400.3930-1-ap420073@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] hsr: fix a NULL pointer dereference in hsr_dev_xmit() | expand |
From: Taehee Yoo <ap420073@gmail.com> Date: Sat, 30 Nov 2019 14:24:00 +0000 > @@ -226,9 +226,16 @@ static int hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev) > struct hsr_priv *hsr = netdev_priv(dev); > struct hsr_port *master; > > + rcu_read_lock(); > master = hsr_port_get_hsr(hsr, HSR_PT_MASTER); I don't want to distract from your bug fix but I had to audit and learn how this hsr->ports stuff works while reading your patch. hsr->ports has supposedly RCU protection... But add and delete opertions to the port list only occur by newlink netlink operations (the device isn't even visible yet at this point) and network device teardown (all packet processing paths will quiesce beforehand). Therefore, the port list never changes from it's effectively static configuration made at hsr_dev_finalize() time. The whole driver very inconsistently accesses the hsr->port list, and it all works only because of the above invariant. So let's not try to fix the RCU protection issues here ok? That should be handled separately, and there are no real problems caused by the lack of RCU protection here right now. Thank you.
On Sat, Nov 30, 2019 at 6:24 AM Taehee Yoo <ap420073@gmail.com> wrote: > > hsr_dev_xmit() calls hsr_port_get_hsr() to find master node and that would > return NULL if master node is not existing in the list. > But hsr_dev_xmit() doesn't check return pointer so a NULL dereference > could occur. If you look at the git history, I made a same patch but reverted it later. :) > > In the TX datapath, there is no rcu_read_lock() so this patch adds missing > rcu_read_lock() in the hsr_dev_xmit() too. This is wrong. > > Test commands: > ip netns add nst > ip link add v0 type veth peer name v1 > ip link add v2 type veth peer name v3 > ip link set v1 netns nst > ip link set v3 netns nst > ip link add hsr0 type hsr slave1 v0 slave2 v2 > ip a a 192.168.100.1/24 dev hsr0 > ip link set v0 up > ip link set v2 up > ip link set hsr0 up > ip netns exec nst ip link add hsr1 type hsr slave1 v1 slave2 v3 > ip netns exec nst ip a a 192.168.100.2/24 dev hsr1 > ip netns exec nst ip link set v1 up > ip netns exec nst ip link set v3 up > ip netns exec nst ip link set hsr1 up > hping3 192.168.100.2 -2 --flood & > modprobe -rv hsr Looks like the master port got deleted without respecting RCU readers, let me look into it. Thanks!
On Sat, Nov 30, 2019 at 10:35 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > Test commands: > > ip netns add nst > > ip link add v0 type veth peer name v1 > > ip link add v2 type veth peer name v3 > > ip link set v1 netns nst > > ip link set v3 netns nst > > ip link add hsr0 type hsr slave1 v0 slave2 v2 > > ip a a 192.168.100.1/24 dev hsr0 > > ip link set v0 up > > ip link set v2 up > > ip link set hsr0 up > > ip netns exec nst ip link add hsr1 type hsr slave1 v1 slave2 v3 > > ip netns exec nst ip a a 192.168.100.2/24 dev hsr1 > > ip netns exec nst ip link set v1 up > > ip netns exec nst ip link set v3 up > > ip netns exec nst ip link set hsr1 up > > hping3 192.168.100.2 -2 --flood & > > modprobe -rv hsr > > Looks like the master port got deleted without respecting RCU > readers, let me look into it. It seems hsr_del_port() gets called on module removal path too and we delete the master port before waiting for RCU readers there. Does the following patch help anything? It just moves the list_del_rcu() after synchronize_rcu() only for master port. diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c index ee561297d8a7..c9638ee00d20 100644 --- a/net/hsr/hsr_slave.c +++ b/net/hsr/hsr_slave.c @@ -174,9 +174,9 @@ void hsr_del_port(struct hsr_port *port) hsr = port->hsr; master = hsr_port_get_hsr(hsr, HSR_PT_MASTER); - list_del_rcu(&port->port_list); if (port != master) { + list_del_rcu(&port->port_list); if (master) { netdev_update_features(master->dev); dev_set_mtu(master->dev, hsr_get_max_mtu(hsr)); @@ -193,5 +193,7 @@ void hsr_del_port(struct hsr_port *port) if (port != master) dev_put(port->dev); + else + list_del_rcu(&port->port_list); kfree(port); }
On Sun, 1 Dec 2019 at 03:34, David Miller <davem@davemloft.net> wrote: > Hi David, Thank you for the review! > From: Taehee Yoo <ap420073@gmail.com> > Date: Sat, 30 Nov 2019 14:24:00 +0000 > > > @@ -226,9 +226,16 @@ static int hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev) > > struct hsr_priv *hsr = netdev_priv(dev); > > struct hsr_port *master; > > > > + rcu_read_lock(); > > master = hsr_port_get_hsr(hsr, HSR_PT_MASTER); > > I don't want to distract from your bug fix but I had to audit and learn > how this hsr->ports stuff works while reading your patch. > > hsr->ports has supposedly RCU protection... > > But add and delete opertions to the port list only occur by newlink > netlink operations (the device isn't even visible yet at this point) > and network device teardown (all packet processing paths will quiesce > beforehand). > > Therefore, the port list never changes from it's effectively static > configuration made at hsr_dev_finalize() time. > > The whole driver very inconsistently accesses the hsr->port list, > and it all works only because of the above invariant. > > So let's not try to fix the RCU protection issues here ok? That > should be handled separately, and there are no real problems caused by > the lack of RCU protection here right now. > > Thank you. Why I thought that rcu_read_lock() is needed in the hsr_dev_xmit() is that hsr_port_get_hsr() and hsr_forward_skb() should be called under rcu_read_lock() but I found there is no rcu_read_lock() in the TX datapath. Below is the test code. diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c index ddd9605bad04..8a62474eb1f6 100644 --- a/net/hsr/hsr_forward.c +++ b/net/hsr/hsr_forward.c @@ -349,6 +349,8 @@ void hsr_forward_skb(struct sk_buff *skb, struct hsr_port *port) { struct hsr_frame_info frame; + printk("[TEST]%s %u rcu_read_lock_held() = %d\n", + __func__, __LINE__, rcu_read_lock_held()); if (skb_mac_header(skb) != skb->data) { WARN_ONCE(1, "%s:%d: Malformed frame (port_src %s)\n", __FILE__, __LINE__, port->dev->name); [12990.180324][ C2] [TEST]hsr_forward_skb 353 rcu_read_lock_held() = 1 [12990.337812][ T1946] [TEST]hsr_forward_skb 353 rcu_read_lock_held() = 0 [12990.341608][ C2] [TEST]hsr_forward_skb 353 rcu_read_lock_held() = 1 $ cat /proc/1946/cmdlind ping192.168.100.2 we could see there is no rcu_read_lock() in the TX datapath. So I'm sure that rcu_read_lock() should be added to the hsr_dev_xmit(). If rcu_read_lock() is unnecessary in there, please let me know about the reason. Thank you! Taehee
On Sun, 1 Dec 2019 at 03:36, Cong Wang <xiyou.wangcong@gmail.com> wrote: > Hi Cong, Thank you for the review! > On Sat, Nov 30, 2019 at 6:24 AM Taehee Yoo <ap420073@gmail.com> wrote: > > > > hsr_dev_xmit() calls hsr_port_get_hsr() to find master node and that would > > return NULL if master node is not existing in the list. > > But hsr_dev_xmit() doesn't check return pointer so a NULL dereference > > could occur. > > If you look at the git history, I made a same patch but reverted it later. :) > Yes, you already made the same patch and reverted. > > > > > In the TX datapath, there is no rcu_read_lock() so this patch adds missing > > rcu_read_lock() in the hsr_dev_xmit() too. > > This is wrong. > I thought hsr_dev_xmit() needs rcu_read_lock() because hsr_port_get_hsr() and hsr_forward_skb() should be called under rcu_read_lock(). Could you let me know about the reason? > > > > > Test commands: > > ip netns add nst > > ip link add v0 type veth peer name v1 > > ip link add v2 type veth peer name v3 > > ip link set v1 netns nst > > ip link set v3 netns nst > > ip link add hsr0 type hsr slave1 v0 slave2 v2 > > ip a a 192.168.100.1/24 dev hsr0 > > ip link set v0 up > > ip link set v2 up > > ip link set hsr0 up > > ip netns exec nst ip link add hsr1 type hsr slave1 v1 slave2 v3 > > ip netns exec nst ip a a 192.168.100.2/24 dev hsr1 > > ip netns exec nst ip link set v1 up > > ip netns exec nst ip link set v3 up > > ip netns exec nst ip link set hsr1 up > > hping3 192.168.100.2 -2 --flood & > > modprobe -rv hsr > > Looks like the master port got deleted without respecting RCU > readers, let me look into it. > > Thanks! Thank you! Taehee
On Sun, 1 Dec 2019 at 06:04, Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Sat, Nov 30, 2019 at 10:35 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > Test commands: > > > ip netns add nst > > > ip link add v0 type veth peer name v1 > > > ip link add v2 type veth peer name v3 > > > ip link set v1 netns nst > > > ip link set v3 netns nst > > > ip link add hsr0 type hsr slave1 v0 slave2 v2 > > > ip a a 192.168.100.1/24 dev hsr0 > > > ip link set v0 up > > > ip link set v2 up > > > ip link set hsr0 up > > > ip netns exec nst ip link add hsr1 type hsr slave1 v1 slave2 v3 > > > ip netns exec nst ip a a 192.168.100.2/24 dev hsr1 > > > ip netns exec nst ip link set v1 up > > > ip netns exec nst ip link set v3 up > > > ip netns exec nst ip link set hsr1 up > > > hping3 192.168.100.2 -2 --flood & > > > modprobe -rv hsr > > > > Looks like the master port got deleted without respecting RCU > > readers, let me look into it. > > > It seems hsr_del_port() gets called on module removal path too > and we delete the master port before waiting for RCU readers > there. The path is this. hsr_exit() -> unregister_netdevice_notifier() -> hsr_netdev_notify() -> hsr_del_port(). > > Does the following patch help anything? It just moves the list_del_rcu() > after synchronize_rcu() only for master port. > Thank you so much for providing the testing code. I have tested this patch, but I could still see the same panic. > diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c > index ee561297d8a7..c9638ee00d20 100644 > --- a/net/hsr/hsr_slave.c > +++ b/net/hsr/hsr_slave.c > @@ -174,9 +174,9 @@ void hsr_del_port(struct hsr_port *port) > > hsr = port->hsr; > master = hsr_port_get_hsr(hsr, HSR_PT_MASTER); > - list_del_rcu(&port->port_list); > > if (port != master) { > + list_del_rcu(&port->port_list); > if (master) { > netdev_update_features(master->dev); > dev_set_mtu(master->dev, hsr_get_max_mtu(hsr)); > @@ -193,5 +193,7 @@ void hsr_del_port(struct hsr_port *port) > > if (port != master) > dev_put(port->dev); > + else > + list_del_rcu(&port->port_list); > kfree(port); > }
On Sun, Dec 1, 2019 at 8:08 AM Taehee Yoo <ap420073@gmail.com> wrote: > > > > Does the following patch help anything? It just moves the list_del_rcu() > > after synchronize_rcu() only for master port. > > > > Thank you so much for providing the testing code. > I have tested this patch, but I could still see the same panic. Yeah, I think the RCU rule requires the "unpublish" to be done before grace period. New RCU readers could still jump in after synchronize_rcu()... :-/ IOW, checking NULL is the only way to fix it. So your patch is fine, although the rcu_read_lock() is unnecessary, as the caller already acquires RCU read lock. But it doesn't harm anything either. Thanks!
On Sat, Nov 30, 2019 at 6:24 AM Taehee Yoo <ap420073@gmail.com> wrote: > > hsr_dev_xmit() calls hsr_port_get_hsr() to find master node and that would > return NULL if master node is not existing in the list. > But hsr_dev_xmit() doesn't check return pointer so a NULL dereference > could occur. > > In the TX datapath, there is no rcu_read_lock() so this patch adds missing > rcu_read_lock() in the hsr_dev_xmit() too. Just a correction: The TX path _has_ RCU read lock, but it harms nothing to take it again. [...] > > Fixes: 311633b60406 ("hsr: switch ->dellink() to ->ndo_uninit()") > Signed-off-by: Taehee Yoo <ap420073@gmail.com> This fix is correct. There is no other way to workaround this RCU rule, checking against NULL is the only way to fix RCU reader races, so: Acked-by: Cong Wang <xiyou.wangcong@gmail.com> Taehee, you might have to resend this with my Acked-by as David probably already drops it from patchwork. Thanks.
On Thu, 5 Dec 2019 at 02:30, Cong Wang <xiyou.wangcong@gmail.com> wrote: > Hi Cong, > On Sat, Nov 30, 2019 at 6:24 AM Taehee Yoo <ap420073@gmail.com> wrote: > > > > hsr_dev_xmit() calls hsr_port_get_hsr() to find master node and that would > > return NULL if master node is not existing in the list. > > But hsr_dev_xmit() doesn't check return pointer so a NULL dereference > > could occur. > > > > In the TX datapath, there is no rcu_read_lock() so this patch adds missing > > rcu_read_lock() in the hsr_dev_xmit() too. > > Just a correction: > The TX path _has_ RCU read lock, but it harms nothing to take it again. > > [...] > > > > Fixes: 311633b60406 ("hsr: switch ->dellink() to ->ndo_uninit()") > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > This fix is correct. There is no other way to workaround this RCU > rule, checking against NULL is the only way to fix RCU reader > races, so: > > Acked-by: Cong Wang <xiyou.wangcong@gmail.com> > > > Taehee, you might have to resend this with my Acked-by as David > probably already drops it from patchwork. > > Thanks. Thank you so much for your detailed reviews. I will send a v2 patch that will not include unnecessary rcu_read_lock(). Thank you! Taehee Yoo
diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c index f509b495451a..e3871491960c 100644 --- a/net/hsr/hsr_device.c +++ b/net/hsr/hsr_device.c @@ -226,9 +226,16 @@ static int hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev) struct hsr_priv *hsr = netdev_priv(dev); struct hsr_port *master; + rcu_read_lock(); master = hsr_port_get_hsr(hsr, HSR_PT_MASTER); - skb->dev = master->dev; - hsr_forward_skb(skb, master); + if (master) { + skb->dev = master->dev; + hsr_forward_skb(skb, master); + } else { + atomic_long_inc(&dev->tx_dropped); + dev_kfree_skb_any(skb); + } + rcu_read_unlock(); return NETDEV_TX_OK; }
hsr_dev_xmit() calls hsr_port_get_hsr() to find master node and that would return NULL if master node is not existing in the list. But hsr_dev_xmit() doesn't check return pointer so a NULL dereference could occur. In the TX datapath, there is no rcu_read_lock() so this patch adds missing rcu_read_lock() in the hsr_dev_xmit() too. Test commands: ip netns add nst ip link add v0 type veth peer name v1 ip link add v2 type veth peer name v3 ip link set v1 netns nst ip link set v3 netns nst ip link add hsr0 type hsr slave1 v0 slave2 v2 ip a a 192.168.100.1/24 dev hsr0 ip link set v0 up ip link set v2 up ip link set hsr0 up ip netns exec nst ip link add hsr1 type hsr slave1 v1 slave2 v3 ip netns exec nst ip a a 192.168.100.2/24 dev hsr1 ip netns exec nst ip link set v1 up ip netns exec nst ip link set v3 up ip netns exec nst ip link set hsr1 up hping3 192.168.100.2 -2 --flood & modprobe -rv hsr Splat looks like: [ 390.879740][ T1362] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN PTI [ 390.880789][ T1362] CPU: 3 PID: 1362 Comm: hping3 Not tainted 5.4.0+ #183 [ 390.881679][ T1362] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 [ 390.882804][ T1362] RIP: 0010:hsr_dev_xmit+0x34/0x90 [hsr] [ 390.883528][ T1362] Code: 48 8d be 00 0c 00 00 be 04 00 00 00 48 83 ec 08 e8 21 be ff ff 48 8d 78 10 48 ba 00 00 00 0b [ 390.887020][ T1362] RSP: 0018:ffff888045507058 EFLAGS: 00010202 [ 390.888067][ T1362] RAX: 0000000000000000 RBX: ffff88804a5d0cc0 RCX: 0000000000000002 [ 390.889390][ T1362] RDX: dffffc0000000000 RSI: 0000000000000004 RDI: 0000000000000010 [ 390.890525][ T1362] RBP: ffff88804a5d0cc0 R08: ffffed100d9c0d5d R09: 0000000000000001 [ 390.891527][ T1362] R10: 0000000000000001 R11: ffffed100d9c0d5c R12: ffff888063bac000 [ 390.893637][ T1362] R13: ffff888063bac088 R14: 0000000000000000 R15: ffff88806428fa00 [ 390.900829][ T1362] FS: 00007fa5a5f40740(0000) GS:ffff88806cc00000(0000) knlGS:0000000000000000 [ 390.908566][ T1362] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 390.909280][ T1362] CR2: 0000555eaf8cef58 CR3: 000000005c8ec002 CR4: 00000000000606e0 [ 390.910070][ T1362] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 390.910899][ T1362] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 390.911722][ T1362] Call Trace: [ 390.912105][ T1362] dev_hard_start_xmit+0x160/0x740 [ 390.912640][ T1362] __dev_queue_xmit+0x1961/0x2e10 [ 390.913148][ T1362] ? check_object+0xaf/0x260 [ 390.913630][ T1362] ? __alloc_skb+0xb9/0x500 [ 390.914088][ T1362] ? init_object+0x6b/0x80 [ 390.914558][ T1362] ? netdev_core_pick_tx+0x2e0/0x2e0 [ 390.915085][ T1362] ? __alloc_skb+0xb9/0x500 [ 390.915588][ T1362] ? rcu_read_lock_sched_held+0x90/0xc0 [ 390.916182][ T1362] ? rcu_read_lock_bh_held+0xa0/0xa0 [ 390.916742][ T1362] ? kasan_unpoison_shadow+0x30/0x40 [ 390.917276][ T1362] ? __kasan_kmalloc.constprop.4+0xa0/0xd0 [ 390.924192][ T1362] ? __kmalloc_node_track_caller+0x3a8/0x3f0 [ 390.924902][ T1362] ? __kasan_kmalloc.constprop.4+0xa0/0xd0 [ 390.925662][ T1362] ? __kmalloc_reserve.isra.46+0x2e/0xb0 [ 390.926398][ T1362] ? memset+0x1f/0x40 [ 390.926904][ T1362] ? __alloc_skb+0x317/0x500 [ 390.927492][ T1362] ? arp_xmit+0xca/0x2c0 [ 390.928050][ T1362] arp_xmit+0xca/0x2c0 [ 390.928576][ T1362] ? arp_error_report+0x150/0x150 [ 390.929209][ T1362] ? eth_header+0x1b5/0x200 [ 390.929781][ T1362] ? memset+0x1f/0x40 [ 390.930288][ T1362] ? arp_create+0x616/0x780 [ 390.930857][ T1362] arp_send_dst.part.16+0x124/0x180 [ 390.931524][ T1362] ? arp_xmit+0x2c0/0x2c0 [ 390.932088][ T1362] arp_solicit+0x8cf/0xfb0 [ 390.932654][ T1362] ? lock_downgrade+0x6e0/0x6e0 [ 390.933269][ T1362] ? arp_send+0x90/0x90 [ 390.933803][ T1362] neigh_probe+0xaf/0xf0 [ ... ] Fixes: 311633b60406 ("hsr: switch ->dellink() to ->ndo_uninit()") Signed-off-by: Taehee Yoo <ap420073@gmail.com> --- net/hsr/hsr_device.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)