[Devel] [PATCH vz7 2/2] venetdev: fix race between veip shutdown and add veip entry

[Kirill Tkhai]

On 27.12.2018 15:35, Konstantin Khorenko wrote:
> If veip entry is added via cgroup ve::ip_allow interface
> after veip_shutdown() is called, veip structure leaks.
> Moreover, you won't be able to assign same IP address to
> a Container (same after restart or another one) because
> veip entry exists, but veip->veid mismatches.
> 
> This happens because veip_entry_add() creates new veip struct
> if ve->veip == NULL. The intention was to handle the case when
> we load vznetdev module after a Container start.
> 
> In fact this code does not work now: venet device is create by
> venet_newlink() which does it only in case env->veip == NULL,
> so venet device will be never created in this case.
> 
> Let's make veip_entry_add() to fail in case ve->veip == NULL
> instead.
> 
> Let's also add a warning on veip struct leak. Just in case.
> 
> https://jira.sw.ru/browse/PSBM-90395
> 
> Signed-off-by: Konstantin Khorenko <khorenko at virtuozzo.com>
> ---
>  drivers/net/venetdev.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/venetdev.c b/drivers/net/venetdev.c
> index 429a74c77a4b..fb02660035b3 100644
> --- a/drivers/net/venetdev.c
> +++ b/drivers/net/venetdev.c
> @@ -87,8 +87,10 @@ static void veip_free(struct rcu_head *rcu)
>  
>  int veip_put(struct veip_struct *veip)
>  {
> -	if (!list_empty(&veip->ip_lh))
> +	if (!list_empty(&veip->ip_lh)) {
> +		WARN_ONCE(1, "Leaking veip structure 0x%p", veip);

We come here from two paths:

1)__veip_stop()->veip_release(), where ip_lh is definitely empty,
2)venet_exit()->veip_cleanup(), which is called on module unload,
  on module unload. Module may be unloaded only after all veip were
  destroyed, i.e., went thru (1) path.

So, this looks OK for me.

Reviewed-by: Kirill Tkhai <ktkhai at virtuozzo.com>


>  		return 0;
> +	}
>  
>  	list_del(&veip->list);
>  	call_rcu(&veip->rcu, veip_free);
> @@ -283,14 +285,12 @@ static int veip_entry_add(struct ve_struct *ve, struct ve_addr_struct *addr)
>  	if (entry == NULL)
>  		return -ENOMEM;
>  
> +	spin_lock(&veip_lock);
>  	if (ve->veip == NULL) {
> -		/* This can happen if we load venet AFTER ve was started */
> -	       	err = veip_start(ve);
> -		if (err < 0)
> -			goto out;
> +		err = -ENODEV;
> +		goto out_unlock;
>  	}
>  
> -	spin_lock(&veip_lock);
>  	found = venet_entry_lookup(addr);
>  	if (found != NULL) {
>  		err = veip_entry_conflict(found, ve);
> @@ -303,9 +303,9 @@ static int veip_entry_add(struct ve_struct *ve, struct ve_addr_struct *addr)
>  
>  	err = 0;
>  	entry = NULL;
> +
>  out_unlock:
>  	spin_unlock(&veip_lock);
> -out:
>  	if (entry != NULL)
>  		kfree(entry);
>  
>