Re: [PATCH net] vxlan: synchronously and race-free destruction of vxlan sockets

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

Hi Hannes,

On Thu, Apr 07, 2016 at 04:57:40PM +0200, Hannes Frederic Sowa wrote:
> Due to the fact that the udp socket is destructed asynchronously in a
> work queue, we have some nondeterministic behavior during shutdown of
> vxlan tunnels and creating new ones. Fix this by keeping the destruction
> process synchronous in regards to the user space process so IFF_UP can
> be reliably set.
> 
> udp_tunnel_sock_release destroys vs->sock->sk if reference counter
> indicates so. We expect to have the same lifetime of vxlan_sock and
> vxlan_sock->sock->sk even in fast paths with only rcu locks held. So
> only destruct the whole socket after we can be sure it cannot be found
> by searching vxlan_net->sock_list.
> 
> Cc: Jiri Benc <jbenc@redhat.com>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
>  drivers/net/vxlan.c | 20 +++-----------------
>  include/net/vxlan.h |  2 --
>  2 files changed, 3 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 1c0fa364323e28..487e48b7a53090 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -98,7 +98,6 @@ struct vxlan_fdb {
>  
>  /* salt for hash table */
>  static u32 vxlan_salt __read_mostly;
> -static struct workqueue_struct *vxlan_wq;
>  
>  static inline bool vxlan_collect_metadata(struct vxlan_sock *vs)
>  {
> @@ -1065,7 +1064,9 @@ static void __vxlan_sock_release(struct vxlan_sock *vs)
>  	vxlan_notify_del_rx_port(vs);
>  	spin_unlock(&vn->sock_lock);
>  
> -	queue_work(vxlan_wq, &vs->del_work);
> +	synchronize_rcu();

__vxlan_sock_release is called by vxlan_sock_release which is called by
vxlan_open/stop. Do we really want to have synchronize_rcu() while
holding rtnl?

> +	udp_tunnel_sock_release(vs->sock);
> +	kfree(vs);
>  }
>  
>  static void vxlan_sock_release(struct vxlan_dev *vxlan)
> @@ -2574,13 +2575,6 @@ static const struct ethtool_ops vxlan_ethtool_ops = {
>  	.get_link	= ethtool_op_get_link,
>  };
>  
> -static void vxlan_del_work(struct work_struct *work)
> -{
> -	struct vxlan_sock *vs = container_of(work, struct vxlan_sock, del_work);
> -	udp_tunnel_sock_release(vs->sock);
> -	kfree_rcu(vs, rcu);
> -}
> -
>  static struct socket *vxlan_create_sock(struct net *net, bool ipv6,
>  					__be16 port, u32 flags)
>  {
> @@ -2626,8 +2620,6 @@ static struct vxlan_sock *vxlan_socket_create(struct net *net, bool ipv6,
>  	for (h = 0; h < VNI_HASH_SIZE; ++h)
>  		INIT_HLIST_HEAD(&vs->vni_list[h]);
>  
> -	INIT_WORK(&vs->del_work, vxlan_del_work);
> -
>  	sock = vxlan_create_sock(net, ipv6, port, flags);
>  	if (IS_ERR(sock)) {
>  		pr_info("Cannot bind port %d, err=%ld\n", ntohs(port),
> @@ -3218,10 +3210,6 @@ static int __init vxlan_init_module(void)
>  {
>  	int rc;
>  
> -	vxlan_wq = alloc_workqueue("vxlan", 0, 0);
> -	if (!vxlan_wq)
> -		return -ENOMEM;
> -
>  	get_random_bytes(&vxlan_salt, sizeof(vxlan_salt));
>  
>  	rc = register_pernet_subsys(&vxlan_net_ops);
> @@ -3242,7 +3230,6 @@ out3:
>  out2:
>  	unregister_pernet_subsys(&vxlan_net_ops);
>  out1:
> -	destroy_workqueue(vxlan_wq);
>  	return rc;
>  }
>  late_initcall(vxlan_init_module);
> @@ -3251,7 +3238,6 @@ static void __exit vxlan_cleanup_module(void)
>  {
>  	rtnl_link_unregister(&vxlan_link_ops);
>  	unregister_netdevice_notifier(&vxlan_notifier_block);
> -	destroy_workqueue(vxlan_wq);
>  	unregister_pernet_subsys(&vxlan_net_ops);
>  	/* rcu_barrier() is called by netns */
>  }
> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index 73ed2e951c020d..2113f808e905a4 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -126,9 +126,7 @@ struct vxlan_metadata {
>  /* per UDP socket information */
>  struct vxlan_sock {
>  	struct hlist_node hlist;
> -	struct work_struct del_work;
>  	struct socket	 *sock;
> -	struct rcu_head	  rcu;
>  	struct hlist_head vni_list[VNI_HASH_SIZE];
>  	atomic_t	  refcnt;
>  	struct udp_offload udp_offloads;
> -- 
> 2.5.5
>