On 19 October 2016 at 11:27, Pravin B Shelar <pshe...@ovn.org> wrote: > On upstream kernel datapath OVS make use of networking devices > where networking stack does check if device is UP. following > patch adds same check in case of compat tunneling implementation. > This check also fixes kernel crash in case vxlan device was > brought down by user. > > CPU: 4 PID: 12988 Comm: handler903 Tainted: > RIP: 0010:[<ffffffffa05e5407>] vxlan_xmit_one.constprop.50+0x47/0x1210 > [openvswitch] > Call Trace: > [<ffffffffa05e6625>] rpl_vxlan_xmit+0x55/0x80 [openvswitch] > [<ffffffffa05d5ad4>] ovs_vport_send+0x44/0xb0 [openvswitch] > [<ffffffffa05c62a5>] do_output+0x65/0x180 [openvswitch] > [<ffffffffa05c70dc>] do_execute_actions+0x10c/0x860 [openvswitch] > [<ffffffffa05c7870>] ovs_execute_actions+0x40/0x130 [openvswitch] > [<ffffffffa05cbb59>] ovs_packet_cmd_execute+0x2c9/0x2f0 [openvswitch] > [<ffffffff8155f31d>] genl_family_rcv_msg+0x1cd/0x400 > [<ffffffff8155f5e1>] genl_rcv_msg+0x91/0xd0 > [<ffffffff8155d549>] netlink_rcv_skb+0xa9/0xc0 > [<ffffffff8155da78>] genl_rcv+0x28/0x40 > [<ffffffff8155ceba>] netlink_unicast+0x16a/0x210 > [<ffffffff8155d277>] netlink_sendmsg+0x317/0x430 > [<ffffffff81514fd0>] sock_sendmsg+0xb0/0xf0 > [<ffffffff81515409>] ___sys_sendmsg+0x3a9/0x3c0 > [<ffffffff815162f1>] __sys_sendmsg+0x51/0x90 > [<ffffffff81516342>] SyS_sendmsg+0x12/0x20 > [<ffffffff81649609>] system_call_fastpath+0x16/0x1b > > Reported-by: Huanglili (lee) <huanglili.hu...@huawei.com> > Signed-off-by: Pravin B Shelar <pshe...@ovn.org>
I think we need to use netif_running() for the check, not IFF_UP. in __dev_close_many(), it first clears the bit that is used by netif_running, then tries to deactive all of the tx queues, synchronizes with net if need be, calls ndo_stop(), then finally clears IFF_UP. > --- > datapath/linux/compat/geneve.c | 6 ++++++ > datapath/linux/compat/ip_gre.c | 3 +++ > datapath/linux/compat/lisp.c | 5 +++++ > datapath/linux/compat/stt.c | 8 +++++++- > datapath/linux/compat/vxlan.c | 5 ++++- > 5 files changed, 25 insertions(+), 2 deletions(-) > > diff --git a/datapath/linux/compat/geneve.c b/datapath/linux/compat/geneve.c > index 0c5b58a..1cd4f75 100644 > --- a/datapath/linux/compat/geneve.c > +++ b/datapath/linux/compat/geneve.c > @@ -1118,6 +1118,12 @@ netdev_tx_t rpl_geneve_xmit(struct sk_buff *skb) > struct geneve_dev *geneve = netdev_priv(dev); > struct ip_tunnel_info *info = NULL; > > + if (unlikely(!(dev->flags & IFF_UP))) { > + dev->stats.tx_dropped++; > + kfree_skb(skb); > + return NETDEV_TX_OK; > + } > + I guess this is more like an 'expected' type of error, so we should use dev_kfree_skb()? > if (geneve->collect_md) > info = skb_tunnel_info(skb); > > diff --git a/datapath/linux/compat/ip_gre.c b/datapath/linux/compat/ip_gre.c > index 03c5435..c77b834 100644 > --- a/datapath/linux/compat/ip_gre.c > +++ b/datapath/linux/compat/ip_gre.c > @@ -285,6 +285,9 @@ netdev_tx_t rpl_gre_fb_xmit(struct sk_buff *skb) > __be16 df, flags; > int err; > > + if (unlikely(!(dev->flags & IFF_UP))) > + goto err_free_skb; > + > tun_info = skb_tunnel_info(skb); > if (unlikely(!tun_info || !(tun_info->mode & IP_TUNNEL_INFO_TX) || > ip_tunnel_info_af(tun_info) != AF_INET)) > diff --git a/datapath/linux/compat/lisp.c b/datapath/linux/compat/lisp.c > index 3a4bebc..6aadfa7 100644 > --- a/datapath/linux/compat/lisp.c > +++ b/datapath/linux/compat/lisp.c > @@ -325,6 +325,11 @@ netdev_tx_t rpl_lisp_xmit(struct sk_buff *skb) > __be16 df; > int err; > > + if (unlikely(!(dev->flags & IFF_UP))) { > + dev->stats.tx_dropped++; > + goto error; > + } > + > info = skb_tunnel_info(skb); > if (unlikely(!info)) { > err = -EINVAL; > diff --git a/datapath/linux/compat/stt.c b/datapath/linux/compat/stt.c > index ca9f039..9e851c5 100644 > --- a/datapath/linux/compat/stt.c > +++ b/datapath/linux/compat/stt.c > @@ -1009,6 +1009,11 @@ netdev_tx_t ovs_stt_xmit(struct sk_buff *skb) > __be16 df; > int err; > > + if (unlikely(!(dev->flags & IFF_UP))) { > + dev->stats.tx_dropped++; > + goto free; > + } > + > tun_info = skb_tunnel_info(skb); > if (unlikely(!tun_info)) { > err = -EINVAL; > @@ -1032,8 +1037,9 @@ netdev_tx_t ovs_stt_xmit(struct sk_buff *skb) > df, sport, dport, tun_key->tun_id); > return NETDEV_TX_OK; > error: > - kfree_skb(skb); > dev->stats.tx_errors++; > +free: > + kfree_skb(skb); > return NETDEV_TX_OK; > } > EXPORT_SYMBOL(ovs_stt_xmit); > diff --git a/datapath/linux/compat/vxlan.c b/datapath/linux/compat/vxlan.c > index 47a5a68..211d0c3 100644 > --- a/datapath/linux/compat/vxlan.c > +++ b/datapath/linux/compat/vxlan.c > @@ -1231,6 +1231,9 @@ netdev_tx_t rpl_vxlan_xmit(struct sk_buff *skb) > struct vxlan_dev *vxlan = netdev_priv(dev); > const struct ip_tunnel_info *info; > > + if (unlikely(!(dev->flags & IFF_UP))) > + goto drop; > + > info = skb_tunnel_info(skb); > skb_reset_mac_header(skb); > if (vxlan->flags & VXLAN_F_COLLECT_METADATA) { > @@ -1239,7 +1242,7 @@ netdev_tx_t rpl_vxlan_xmit(struct sk_buff *skb) > return NETDEV_TX_OK; > } > } > - > +drop: > dev->stats.tx_dropped++; > kfree_skb(skb); > return NETDEV_TX_OK; > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev