diff mbox

UBUNTU: SAUCE: net/ipv4: Always flush route cache on unregister batch call

Message ID 1399040150-11863-2-git-send-email-chris.j.arges@canonical.com
State New
Headers show

Commit Message

Chris J Arges May 2, 2014, 2:15 p.m. UTC
From: Stefan Bader <stefan.bader@canonical.com>

When doing (for example) a connect to an unused port on localhost
in a new net namespace, it was observed that lo could not be
removed immediately because there were still references on it.
To release those references could take a very long time (up to
five minutes).

This was tracked down to an element in the route cache which was
supposed to be dropped by a notify call with sending a
NETDEVICE_UNREGISTER_BATCH message, but as in_dev is already unset
then, the case statement never was executed.

Move the handling of that notification up to be done regardless of
the state of in_dev.

BugLink: http://bugs.launchpad.net/bugs/1021471

Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
---
 net/ipv4/fib_frontend.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Tim Gardner May 2, 2014, 3:57 p.m. UTC | #1

Andy Whitcroft May 2, 2014, 4:25 p.m. UTC | #2
On Fri, May 02, 2014 at 09:15:50AM -0500, Chris J Arges wrote:
> From: Stefan Bader <stefan.bader@canonical.com>
> 
> When doing (for example) a connect to an unused port on localhost
> in a new net namespace, it was observed that lo could not be
> removed immediately because there were still references on it.
> To release those references could take a very long time (up to
> five minutes).
> 
> This was tracked down to an element in the route cache which was
> supposed to be dropped by a notify call with sending a
> NETDEVICE_UNREGISTER_BATCH message, but as in_dev is already unset
> then, the case statement never was executed.
> 
> Move the handling of that notification up to be done regardless of
> the state of in_dev.
> 
> BugLink: http://bugs.launchpad.net/bugs/1021471
> 
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
> ---
>  net/ipv4/fib_frontend.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> index 92fc5f6..5085d48 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -999,6 +999,14 @@ static int fib_netdev_event(struct notifier_block *this, unsigned long event, vo
>  		fib_disable_ip(dev, 2, -1);
>  		return NOTIFY_DONE;
>  	}
> +	if (event == NETDEV_UNREGISTER_BATCH) {
> +		/* The batch unregister is only called on the first
> +		 * device in the list of devices being unregistered.
> +		 * Therefore we should not pass dev_net(dev) in here.
> +		 */
> +		rt_cache_flush_batch(NULL);
> +		return NOTIFY_DONE;
> +	}
>  
>  	if (!in_dev)
>  		return NOTIFY_DONE;
> @@ -1021,13 +1029,6 @@ static int fib_netdev_event(struct notifier_block *this, unsigned long event, vo
>  	case NETDEV_CHANGE:
>  		rt_cache_flush(dev_net(dev), 0);
>  		break;
> -	case NETDEV_UNREGISTER_BATCH:
> -		/* The batch unregister is only called on the first
> -		 * device in the list of devices being unregistered.
> -		 * Therefore we should not pass dev_net(dev) in here.
> -		 */
> -		rt_cache_flush_batch(NULL);
> -		break;
>  	}
>  	return NOTIFY_DONE;
>  }

Looks to do what is claimed and has a reproducer to confirm.  I assume
this already on its way upstream.  On that basis:

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw
diff mbox

Patch

diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 92fc5f6..5085d48 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -999,6 +999,14 @@  static int fib_netdev_event(struct notifier_block *this, unsigned long event, vo
 		fib_disable_ip(dev, 2, -1);
 		return NOTIFY_DONE;
 	}
+	if (event == NETDEV_UNREGISTER_BATCH) {
+		/* The batch unregister is only called on the first
+		 * device in the list of devices being unregistered.
+		 * Therefore we should not pass dev_net(dev) in here.
+		 */
+		rt_cache_flush_batch(NULL);
+		return NOTIFY_DONE;
+	}
 
 	if (!in_dev)
 		return NOTIFY_DONE;
@@ -1021,13 +1029,6 @@  static int fib_netdev_event(struct notifier_block *this, unsigned long event, vo
 	case NETDEV_CHANGE:
 		rt_cache_flush(dev_net(dev), 0);
 		break;
-	case NETDEV_UNREGISTER_BATCH:
-		/* The batch unregister is only called on the first
-		 * device in the list of devices being unregistered.
-		 * Therefore we should not pass dev_net(dev) in here.
-		 */
-		rt_cache_flush_batch(NULL);
-		break;
 	}
 	return NOTIFY_DONE;
 }