| Message ID | 1476901636-3910-1-git-send-email-pshelar@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
On 19 October 2016 at 11:27, Pravin B Shelar <pshelar@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.huang@huawei.com> > Signed-off-by: Pravin B Shelar <pshelar@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
On Thu, Oct 20, 2016 at 6:49 PM, Joe Stringer <joe@ovn.org> wrote: > On 19 October 2016 at 11:27, Pravin B Shelar <pshelar@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.huang@huawei.com> >> Signed-off-by: Pravin B Shelar <pshelar@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. > This works on close but during __dev_open() the flag __LINK_STATE_START is set before calling ndo_open(). That does not work for vxlan device. I am trying to mimic upstream dev-queue-xmit behavior here. Therefore we should check for IFF_UP flag. >> --- >> 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()? > Thats true. I will post update patch.
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; + } + 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;
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.huang@huawei.com> Signed-off-by: Pravin B Shelar <pshelar@ovn.org> --- 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(-)