[ovs-dev] [PATCH 1/3] datapath: backport: vxlan: avoid using stale vxlan socket.

Pravin Shelar pshelar at ovn.org
Mon Oct 31 21:24:18 UTC 2016


On Mon, Oct 31, 2016 at 2:05 PM, Joe Stringer <joe at ovn.org> wrote:
> On 29 October 2016 at 21:33, Pravin B Shelar <pshelar at ovn.org> wrote:
>> Upstream commit:
>>     commit c6fcc4fc5f8b592600c7409e769ab68da0fb1eca
>>     Author: pravin shelar <pshelar at ovn.org>
>>     Date:   Fri Oct 28 09:59:15 2016 -0700
>>
>>     vxlan: avoid using stale vxlan socket.
>>
>>     When vxlan device is closed vxlan socket is freed. This
>>     operation can race with vxlan-xmit function which
>>     dereferences vxlan socket. Following patch uses RCU
>>     mechanism to avoid this situation.
>>
>>     Signed-off-by: Pravin B Shelar <pshelar at ovn.org>
>>     Signed-off-by: David S. Miller <davem at davemloft.net>
>>
>> Signed-off-by: Pravin B Shelar <pshelar at ovn.org>
>
> There's another dereference of vxlan->vn*_sock in vxlan_dev_dst_port()
> in the compat code.. is this supposed to use rcu_dereference() now?
> (Upstream too)
>
right.
This function is not used even on upstream kernel. I will just remove it.

> <snip>
>
>> @@ -1434,9 +1449,10 @@ int ovs_vxlan_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb)
>>         dport = info->key.tp_dst ? : vxlan->cfg.dst_port;
>>
>>         if (ip_tunnel_info_af(info) == AF_INET) {
>> +               struct vxlan_sock *sock4 = rcu_dereference(vxlan->vn4_sock);
>>                 struct rtable *rt;
>>
>> -               if (!vxlan->vn4_sock)
>> +               if (!sock4)
>>                         return -EINVAL;
>>                 rt = vxlan_get_route(vxlan, skb, 0, info->key.tos,
>>                                      info->key.u.ipv4.dst,
>> @@ -1448,8 +1464,6 @@ int ovs_vxlan_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb)
>>  #if IS_ENABLED(CONFIG_IPV6)
>>                 struct dst_entry *ndst;
>>
>> -               if (!vxlan->vn6_sock)
>> -                       return -EINVAL;
>
> It's a bit odd that we hide this in vxlan6_get_route() for IPv6, but
> we do it differently in the v4 case above. But that's just cosmetic.
>
I did not wanted to check current structure of the code too much.

>>                 ndst = vxlan6_get_route(vxlan, skb, 0, info->key.tos,
>>                                         info->key.label, &info->key.u.ipv6.dst,
>>                                         &info->key.u.ipv6.src, NULL, info);
>
> <snip>
>
> Otherwise, LGTM thanks.

Thanks. I will post new version soon.



More information about the dev mailing list