[ovs-dev] [PATCH v2] datapath: compat: tunnel: Check if device is UP.

Joe Stringer joe at ovn.org
Fri Oct 21 18:37:16 UTC 2016


On 21 October 2016 at 10:55, Pravin B Shelar <pshelar at 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 at huawei.com>
> Signed-off-by: Pravin B Shelar <pshelar at ovn.org>

I'm not 100% convinced that this solves the issue (or whether this
affects upstream as well), but it seems like this at least makes it
significantly less likely to crash - If I understand correctly,
setting the vxlan port down currently guarantees a crash, whereas this
patch would mean it either doesn't crash, or it only crashes based on
a race condition. Specifically, one where the reconfiguration gets
past the synchronize_net() into ndo_stop() (before clearing IFF_UP)
while OVS manages to execute all of a flow up until output and
proceeds past this check before the reconfiguration thread clears it.

Probably the type of test to check this would be running heavy traffic
through a vxlan port, then toggle the port up and down.

Will you also check upstream?

Anyway, this LGTM so:
Acked-by: Joe Stringer <joe at ovn.org>

> ---
> v1-v2:
> Use dev-kfree-skb()
> ---
>  datapath/linux/compat/geneve.c                  |  3 +++
>  datapath/linux/compat/include/linux/netdevice.h | 12 ++++++++++++
>  datapath/linux/compat/ip_gre.c                  |  3 +++
>  datapath/linux/compat/lisp.c                    |  3 +++
>  datapath/linux/compat/stt.c                     |  3 +++
>  datapath/linux/compat/vxlan.c                   |  3 +++
>  6 files changed, 27 insertions(+)
>
> diff --git a/datapath/linux/compat/geneve.c b/datapath/linux/compat/geneve.c
> index 0c5b58a..a5d69b4 100644
> --- a/datapath/linux/compat/geneve.c
> +++ b/datapath/linux/compat/geneve.c
> @@ -1118,6 +1118,9 @@ 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(netdev_down_check(dev, skb)))
> +               return NETDEV_TX_OK;
> +
>         if (geneve->collect_md)
>                 info = skb_tunnel_info(skb);
>
> diff --git a/datapath/linux/compat/include/linux/netdevice.h b/datapath/linux/compat/include/linux/netdevice.h
> index dd028f6..2a768a9 100644
> --- a/datapath/linux/compat/include/linux/netdevice.h
> +++ b/datapath/linux/compat/include/linux/netdevice.h
> @@ -239,6 +239,18 @@ do {                                                               \
>  #ifndef USE_UPSTREAM_TUNNEL
>  #define dev_fill_metadata_dst ovs_dev_fill_metadata_dst
>  int ovs_dev_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb);
> +
> +static inline bool netdev_down_check(struct net_device *dev, struct sk_buff *skb)
> +{
> +       if (unlikely(!(dev->flags & IFF_UP))) {
> +               dev->stats.tx_dropped++;
> +               dev_kfree_skb(skb);
> +               return true;
> +       }
> +       return false;
> +}
> +#else
> +#define netdev_down_check(dev, skb)    false
>  #endif
>
>  #ifndef NETDEV_OFFLOAD_PUSH_VXLAN
> diff --git a/datapath/linux/compat/ip_gre.c b/datapath/linux/compat/ip_gre.c
> index 03c5435..6da6636 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(netdev_down_check(dev, skb)))
> +               return NETDEV_TX_OK;
> +
>         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..8ab8646 100644
> --- a/datapath/linux/compat/lisp.c
> +++ b/datapath/linux/compat/lisp.c
> @@ -325,6 +325,9 @@ netdev_tx_t rpl_lisp_xmit(struct sk_buff *skb)
>         __be16 df;
>         int err;
>
> +       if (unlikely(netdev_down_check(dev, skb)))
> +               return NETDEV_TX_OK;
> +
>         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..5ffe565 100644
> --- a/datapath/linux/compat/stt.c
> +++ b/datapath/linux/compat/stt.c
> @@ -1009,6 +1009,9 @@ netdev_tx_t ovs_stt_xmit(struct sk_buff *skb)
>         __be16 df;
>         int err;
>
> +       if (unlikely(netdev_down_check(dev, skb)))
> +               return NETDEV_TX_OK;
> +
>         tun_info = skb_tunnel_info(skb);
>         if (unlikely(!tun_info)) {
>                 err = -EINVAL;
> diff --git a/datapath/linux/compat/vxlan.c b/datapath/linux/compat/vxlan.c
> index 47a5a68..64cc1a8 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(netdev_down_check(dev, skb)))
> +               return NETDEV_TX_OK;
> +
>         info = skb_tunnel_info(skb);
>         skb_reset_mac_header(skb);
>         if (vxlan->flags & VXLAN_F_COLLECT_METADATA) {
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list