diff mbox

net_namespace: fixed net_device reference leak

Message ID CAJD3hpUMdSt6UXEFjU9OnU4K7qiET50xo8AQ194+4vpX2CfOUA@mail.gmail.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Jojy Varghese Sept. 8, 2016, 11:16 p.m. UTC 
The dst's do disappear but the net device does not. A better solution
is to only hold reference to dst's net device ONLY if it is not a
loopback device. In the case of the loopback device, we want to do
only a "put" on it and skip the "hold".

Updating the patch below:

---
 net/core/dst.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

 }
--
1.8.3.1



On Thu, Sep 8, 2016 at 3:16 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2016-09-08 at 15:12 -0700, Jojy Varghese wrote:
>> Hi Eric/Lance
>>
>>  Thanks for taking time to look at the patch. The problem in dst.c's
>> "dst_ifdown" is that it will never do an actual "dev_put" on the
>> loopback net device. This results in the loopback net device to be
>> alive even when the dst goes away.
>
> Sure, but the dst themselves should disappear, and thus release their
> dst->dev reference.
>
> Something is missing.
>
> loopback device is only used when we remove one device from the system,
> but namespace (and loopback device) is still around.
>
>
>

Comments

Eric Dumazet Sept. 8, 2016, 11:37 p.m. UTC | #1 
On Thu, 2016-09-08 at 16:16 -0700, Jojy Varghese wrote:
> The dst's do disappear but the net device does not. A better solution
> is to only hold reference to dst's net device ONLY if it is not a
> loopback device. In the case of the loopback device, we want to do
> only a "put" on it and skip the "hold".
> 
> Updating the patch below:
> 
> ---
>  net/core/dst.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/net/core/dst.c b/net/core/dst.c
> index 7e45593..f63027e 100644
> --- a/net/core/dst.c
> +++ b/net/core/dst.c
> @@ -433,12 +433,11 @@ static void dst_ifdown(struct dst_entry *dst,
> struct net_device *dev,
>                 dst->input = dst_discard;
>                 dst->output = dst_discard_out;
>         } else {
> -               if (dst->dev == dev_net(dst->dev)->loopback_dev)

On which tree are you working exactly ?
Current code is not looking like that.

> -                       dst->dev = init_net.loopback_dev;
> -               else
> +               if (dst->dev != dev_net(dst->dev)->loopback_dev) {
>                         dst->dev = dev_net(dst->dev)->loopback_dev;
> +                       dev_hold(dst->dev);
> +               }
> 
> -               dev_hold(dst->dev);
>                 dev_put(dev);
>         }
>  }


I appreciate your desperate efforts, but I am telling you the bug is
elsewhere.

Do you have a reproducer of the bug, on latest David Miller tree
( https://git.kernel.org/cgit/linux/kernel/git/davem/net.git )

Thanks.
diff mbox

Patch

diff --git a/net/core/dst.c b/net/core/dst.c
index 7e45593..f63027e 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -433,12 +433,11 @@  static void dst_ifdown(struct dst_entry *dst,
struct net_device *dev,
                dst->input = dst_discard;
                dst->output = dst_discard_out;
        } else {
-               if (dst->dev == dev_net(dst->dev)->loopback_dev)
-                       dst->dev = init_net.loopback_dev;
-               else
+               if (dst->dev != dev_net(dst->dev)->loopback_dev) {
                        dst->dev = dev_net(dst->dev)->loopback_dev;
+                       dev_hold(dst->dev);
+               }

-               dev_hold(dst->dev);
                dev_put(dev);
        }