| Message ID | 20170222203031.45513e4c@cakuba.netronome.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 22 Feb 2017 20:30:31 -0800, Jakub Kicinski wrote: > On Wed, 22 Feb 2017 14:27:45 -0800, Jakub Kicinski wrote: > > [ 1571.067134] =============================== > > [ 1571.071842] [ ERR: suspicious RCU usage. ] > > [ 1571.076546] 4.10.0-debug-03232-g12d656af4e3d #1 Tainted: G W O > > [ 1571.084166] ------------------------------- > > [ 1571.088867] ../drivers/net/vxlan.c:2111 suspicious rcu_dereference_check() usage! > > [ 1571.097286] > > [ 1571.097286] other info that might help us debug this: > > [ 1571.097286] > > [ 1571.106305] > > [ 1571.106305] rcu_scheduler_active = 2, debug_locks = 1 > > [ 1571.113654] 3 locks held by ping/13826: > > [ 1571.117968] #0: (sk_lock-AF_INET){+.+.+.}, at: [<ffffffffa1cd4972>] raw_sendmsg+0x14e2/0x2e40 > > [ 1571.127758] #1: (rcu_read_lock_bh){......}, at: [<ffffffffa1be9594>] ip_finish_output2+0x274/0x1390 > > [ 1571.138135] #2: (rcu_read_lock_bh){......}, at: [<ffffffffa1a9b63c>] __dev_queue_xmit+0x1ec/0x2750 > .... > > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c > index 4e27c5b09600..8aa3e837cd6c 100644 > --- a/drivers/net/vxlan.c > +++ b/drivers/net/vxlan.c > @@ -2109,7 +2109,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev, > vxlan->cfg.port_max, true); > > if (dst->sa.sa_family == AF_INET) { > - struct vxlan_sock *sock4 = rcu_dereference(vxlan->vn4_sock); > + struct vxlan_sock *sock4 = rcu_dereference_bh(vxlan->vn4_sock); > struct rtable *rt; > __be16 df = 0; > > @@ -2148,7 +2148,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev, > src_port, dst_port, xnet, !udp_sum); > #if IS_ENABLED(CONFIG_IPV6) > } else { > - struct vxlan_sock *sock6 = rcu_dereference(vxlan->vn6_sock); > + struct vxlan_sock *sock6 = rcu_dereference_bh(vxlan->vn6_sock); > > ndst = vxlan6_get_route(vxlan, dev, sock6, skb, > rdst ? rdst->remote_ifindex : 0, tos, > Ugh. Looks like this may not work even if it makes the splat go away. synchronize_net() doesn't seem to wait for the _bh() flavor of RCU, so we need to add syncronize_rcu_bh() call before freeing the socket or do a normal rcu_read_lock()/unlock() on the fast path. Any RCU experts want to comment? :) FWIW geneve will need similar fix, I presume.
On Thu, Feb 23, 2017 at 12:20 AM, Jakub Kicinski <kubakici@wp.pl> wrote: > On Wed, 22 Feb 2017 20:30:31 -0800, Jakub Kicinski wrote: >> On Wed, 22 Feb 2017 14:27:45 -0800, Jakub Kicinski wrote: >> > [ 1571.067134] =============================== >> > [ 1571.071842] [ ERR: suspicious RCU usage. ] >> > [ 1571.076546] 4.10.0-debug-03232-g12d656af4e3d #1 Tainted: G W O >> > [ 1571.084166] ------------------------------- >> > [ 1571.088867] ../drivers/net/vxlan.c:2111 suspicious rcu_dereference_check() usage! >> > [ 1571.097286] >> > [ 1571.097286] other info that might help us debug this: >> > [ 1571.097286] >> > [ 1571.106305] >> > [ 1571.106305] rcu_scheduler_active = 2, debug_locks = 1 >> > [ 1571.113654] 3 locks held by ping/13826: >> > [ 1571.117968] #0: (sk_lock-AF_INET){+.+.+.}, at: [<ffffffffa1cd4972>] raw_sendmsg+0x14e2/0x2e40 >> > [ 1571.127758] #1: (rcu_read_lock_bh){......}, at: [<ffffffffa1be9594>] ip_finish_output2+0x274/0x1390 >> > [ 1571.138135] #2: (rcu_read_lock_bh){......}, at: [<ffffffffa1a9b63c>] __dev_queue_xmit+0x1ec/0x2750 >> .... >> >> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c >> index 4e27c5b09600..8aa3e837cd6c 100644 >> --- a/drivers/net/vxlan.c >> +++ b/drivers/net/vxlan.c >> @@ -2109,7 +2109,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev, >> vxlan->cfg.port_max, true); >> >> if (dst->sa.sa_family == AF_INET) { >> - struct vxlan_sock *sock4 = rcu_dereference(vxlan->vn4_sock); >> + struct vxlan_sock *sock4 = rcu_dereference_bh(vxlan->vn4_sock); >> struct rtable *rt; >> __be16 df = 0; >> >> @@ -2148,7 +2148,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev, >> src_port, dst_port, xnet, !udp_sum); >> #if IS_ENABLED(CONFIG_IPV6) >> } else { >> - struct vxlan_sock *sock6 = rcu_dereference(vxlan->vn6_sock); >> + struct vxlan_sock *sock6 = rcu_dereference_bh(vxlan->vn6_sock); >> >> ndst = vxlan6_get_route(vxlan, dev, sock6, skb, >> rdst ? rdst->remote_ifindex : 0, tos, >> > > Ugh. Looks like this may not work even if it makes the splat go away. > synchronize_net() doesn't seem to wait for the _bh() flavor of RCU, so > we need to add syncronize_rcu_bh() call before freeing the socket or do > a normal rcu_read_lock()/unlock() on the fast path. Any RCU experts > want to comment? :) > I think both solutions would work. I prefer using rcu_read-lock in fast path. We already execute vxlan rcv path in rcu read lock so it would make it consistent with xmit path. > FWIW geneve will need similar fix, I presume. I agree.
On Thu, Feb 23, 2017 at 12:20 AM, Jakub Kicinski <kubakici@wp.pl> wrote: > > Ugh. Looks like this may not work even if it makes the splat go away. > synchronize_net() doesn't seem to wait for the _bh() flavor of RCU, so > we need to add syncronize_rcu_bh() call before freeing the socket or do > a normal rcu_read_lock()/unlock() on the fast path. Any RCU experts > want to comment? :) But rcu_read_lock_bh() is enforced by upper layer, __dev_queue_xmit(), I am afraid you can't change it. Does changing these kfree() to kfree_rcu() work too since there is no kfree_rcu_bh()?
On Fri, 24 Feb 2017 11:30:05 -0800, Cong Wang wrote: > On Thu, Feb 23, 2017 at 12:20 AM, Jakub Kicinski <kubakici@wp.pl> wrote: > > > > Ugh. Looks like this may not work even if it makes the splat go away. > > synchronize_net() doesn't seem to wait for the _bh() flavor of RCU, so > > we need to add syncronize_rcu_bh() call before freeing the socket or do > > a normal rcu_read_lock()/unlock() on the fast path. Any RCU experts > > want to comment? :) > > But rcu_read_lock_bh() is enforced by upper layer, __dev_queue_xmit(), > I am afraid you can't change it. > > Does changing these kfree() to kfree_rcu() work too since there is no > kfree_rcu_bh()? If I read the code correctly the VXLAN/GENEVE does standard RCU dereferencing (i.e. non-_bh()) but as you point out __dev_queue_xmit() takes rcu_read_lock_bh() and not rcu_read_lock() so we either have to make VXLAN code take the correct lock itself or make VXLAN reconfiguration code do synchronize_rcu_bh() as well as synchronize_net() (most likely with synchronize_rcu_mult() but then we will have to open code expediting under rtnl...) I was waiting for some internal reviews but let me just post the code I have now implementing the first option.
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c index 4e27c5b09600..8aa3e837cd6c 100644 --- a/drivers/net/vxlan.c +++ b/drivers/net/vxlan.c @@ -2109,7 +2109,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev, vxlan->cfg.port_max, true); if (dst->sa.sa_family == AF_INET) { - struct vxlan_sock *sock4 = rcu_dereference(vxlan->vn4_sock); + struct vxlan_sock *sock4 = rcu_dereference_bh(vxlan->vn4_sock); struct rtable *rt; __be16 df = 0; @@ -2148,7 +2148,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev, src_port, dst_port, xnet, !udp_sum); #if IS_ENABLED(CONFIG_IPV6) } else { - struct vxlan_sock *sock6 = rcu_dereference(vxlan->vn6_sock); + struct vxlan_sock *sock6 = rcu_dereference_bh(vxlan->vn6_sock); ndst = vxlan6_get_route(vxlan, dev, sock6, skb, rdst ? rdst->remote_ifindex : 0, tos,