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

Submitted by Konstantin Khorenko on Dec. 27, 2018, 12:35 p.m.

Details

Message ID 20181227123515.20985-2-khorenko@virtuozzo.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Konstantin Khorenko Dec. 27, 2018, 12:35 p.m.
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@virtuozzo.com>
---
 drivers/net/venetdev.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

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);
 		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);
 

Comments

Kirill Tkhai Dec. 27, 2018, 3:23 p.m.
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@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@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);
>  
>