Re: [PATCH net-next 0/5] net: Consolidate metrics handling for ipv4 and ipv6

Re: [PATCH net-next 0/5] net: Consolidate metrics handling for ipv4 and ipv6

[David Miller]

From: David Ahern <dsahern@kernel.org>
Date: Thu,  4 Oct 2018 20:07:50 -0700

> From: David Ahern <dsahern@gmail.com>
> 
> As part of the IPv6 fib info refactoring, the intent was to make metrics
> handling for ipv6 identical to ipv4. One oversight in ip6_dst_destroy
> led to confusion and a couple of incomplete attempts at finding and
> fixing the resulting memory leak which was ultimately resolved by
> ce7ea4af0838 ("ipv6: fix memory leak on dst->_metrics").
> 
> Refactor metrics hanlding make the code really identical for v4 and v6,
> and add a few test cases.

Looks nice, series applied, thanks David.

Re: [PATCH net-next 0/5] net: Consolidate metrics handling for ipv4 and ipv6

[Eric Dumazet]

On 10/04/2018 09:55 PM, David Miller wrote:
> From: David Ahern <dsahern@kernel.org>
> Date: Thu,  4 Oct 2018 20:07:50 -0700
> 
>> From: David Ahern <dsahern@gmail.com>
>>
>> As part of the IPv6 fib info refactoring, the intent was to make metrics
>> handling for ipv6 identical to ipv4. One oversight in ip6_dst_destroy
>> led to confusion and a couple of incomplete attempts at finding and
>> fixing the resulting memory leak which was ultimately resolved by
>> ce7ea4af0838 ("ipv6: fix memory leak on dst->_metrics").
>>
>> Refactor metrics hanlding make the code really identical for v4 and v6,
>> and add a few test cases.
> 
> Looks nice, series applied, thanks David.
> 

Does not look well tested and reviewed to me.

Re: [PATCH net-next 0/5] net: Consolidate metrics handling for ipv4 and ipv6

[Eric Dumazet]

On 10/05/2018 05:17 AM, Eric Dumazet wrote:
> 
> 
> On 10/04/2018 09:55 PM, David Miller wrote:
>> From: David Ahern <dsahern@kernel.org>
>> Date: Thu,  4 Oct 2018 20:07:50 -0700
>>
>>> From: David Ahern <dsahern@gmail.com>
>>>
>>> As part of the IPv6 fib info refactoring, the intent was to make metrics
>>> handling for ipv6 identical to ipv4. One oversight in ip6_dst_destroy
>>> led to confusion and a couple of incomplete attempts at finding and
>>> fixing the resulting memory leak which was ultimately resolved by
>>> ce7ea4af0838 ("ipv6: fix memory leak on dst->_metrics").
>>>
>>> Refactor metrics hanlding make the code really identical for v4 and v6,
>>> and add a few test cases.
>>
>> Looks nice, series applied, thanks David.
>>
> 
> Does not look well tested and reviewed to me.

For some reason I have not received the patch series in my inbox, I only got
your "series applied" message.

Commit 767a2217533fed6 ("net: common metrics init helper for FIB entries")
is not correct because we need to better deal with error paths.

I will submit this more formally when I can reach my workstation in a few minutes :

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 6c1d817151cae45421dc976c5ea082b4115650be..74d97addf1af20dda0c2b6a2018e88696f9f7d5a 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2976,6 +2976,8 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
        rt->fib6_metrics = ip_fib_metrics_init(net, cfg->fc_mx, cfg->fc_mx_len);
        if (IS_ERR(rt->fib6_metrics)) {
                err = PTR_ERR(rt->fib6_metrics);
+               /* Do not leave garbage there. */
+               rt->fib6_metrics = (struct dst_metrics *)&dst_default_metrics;
                goto out;
        }
 

Re: [PATCH net-next 0/5] net: Consolidate metrics handling for ipv4 and ipv6

[David Ahern]

On 10/5/18 7:08 AM, Eric Dumazet wrote:
> Commit 767a2217533fed6 ("net: common metrics init helper for FIB entries")
> is not correct because we need to better deal with error paths.
> 
> I will submit this more formally when I can reach my workstation in a few minutes :
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 6c1d817151cae45421dc976c5ea082b4115650be..74d97addf1af20dda0c2b6a2018e88696f9f7d5a 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -2976,6 +2976,8 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>         rt->fib6_metrics = ip_fib_metrics_init(net, cfg->fc_mx, cfg->fc_mx_len);
>         if (IS_ERR(rt->fib6_metrics)) {
>                 err = PTR_ERR(rt->fib6_metrics);
> +               /* Do not leave garbage there. */
> +               rt->fib6_metrics = (struct dst_metrics *)&dst_default_metrics;
>                 goto out;
>         }
>  
> 

Yes, I was focused on the memory leaks and cleanup and forgot to
purposely fail the alloc for the metrics. The above is one way to fix
it. Thanks for spotting it.

Re: [PATCH net-next 0/5] net: Consolidate metrics handling for ipv4 and ipv6

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 5 Oct 2018 06:08:57 -0700

> For some reason I have not received the patch series in my inbox, I
> only got your "series applied" message.

I see what happened.

I did something stupid on vger.kernel.org yesterday which ran a
partition out of disk space, and some postings got lost as a result.

Sorry about that.

> Commit 767a2217533fed6 ("net: common metrics init helper for FIB entries")
> is not correct because we need to better deal with error paths.
> 
> I will submit this more formally when I can reach my workstation in a few minutes :

Thanks Eric.