[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0243bb47-4b5f-a1d7-ff63-adcb6504df8a@gmail.com>
Date: Tue, 30 Nov 2021 09:01:31 -0700
From: David Ahern <dsahern@...il.com>
To: Ido Schimmel <idosch@...sch.org>,
Nikolay Aleksandrov <razor@...ckwall.org>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
Nikolay Aleksandrov <nikolay@...dia.com>
Subject: Re: [RFC PATCH net] net: ipv6: make fib6_nh_init properly clean after
itself on error
On 11/30/21 5:40 AM, Ido Schimmel wrote:
> On Mon, Nov 29, 2021 at 04:11:51PM +0200, Nikolay Aleksandrov wrote:
>> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
>> index 5dbd4b5505eb..a7debafe8b90 100644
>> --- a/net/ipv4/nexthop.c
>> +++ b/net/ipv4/nexthop.c
>> @@ -2565,14 +2565,8 @@ static int nh_create_ipv6(struct net *net, struct nexthop *nh,
>> /* sets nh_dev if successful */
>> err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
>> extack);
>> - if (err) {
>> - /* IPv6 is not enabled, don't call fib6_nh_release */
>> - if (err == -EAFNOSUPPORT)
>> - goto out;
>> - ipv6_stub->fib6_nh_release(fib6_nh);
>> - } else {
>> + if (!err)
>> nh->nh_flags = fib6_nh->fib_nh_flags;
>> - }
>> out:
>> return err;
>> }
>
> This hunk looks good
agreed, but it should be a no-op now so this should be a net-next
cleanup patch.
>
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index 42d60c76d30a..2107b13cc9ab 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -3635,7 +3635,9 @@ int fib6_nh_init(struct net *net, struct fib6_nh *fib6_nh,
>> in6_dev_put(idev);
>>
>> if (err) {
>> - lwtstate_put(fib6_nh->fib_nh_lws);
>> + /* check if we failed after fib_nh_common_init() was called */
>> + if (fib6_nh->nh_common.nhc_pcpu_rth_output)
>> + fib_nh_common_release(&fib6_nh->nh_common);
>> fib6_nh->fib_nh_lws = NULL;
>> dev_put(dev);
>> }
>
> Likewise
this is a leak in the current code and should go through -net as a
separate patch.
>
>> @@ -3822,7 +3824,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>> } else {
>> err = fib6_nh_init(net, rt->fib6_nh, cfg, gfp_flags, extack);
>> if (err)
>> - goto out;
>> + goto out_free;
>>
>> fib6_nh = rt->fib6_nh;
>>
>> @@ -3841,7 +3843,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>> if (!ipv6_chk_addr(net, &cfg->fc_prefsrc, dev, 0)) {
>> NL_SET_ERR_MSG(extack, "Invalid source address");
>> err = -EINVAL;
>> - goto out;
>> + goto out_free;
>> }
>> rt->fib6_prefsrc.addr = cfg->fc_prefsrc;
>> rt->fib6_prefsrc.plen = 128;
>> @@ -3849,12 +3851,13 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>> rt->fib6_prefsrc.plen = 0;
>>
>> return rt;
>> -out:
>> - fib6_info_release(rt);
>> - return ERR_PTR(err);
>> +
>> out_free:
>> ip_fib_metrics_put(rt->fib6_metrics);
>> + if (rt->nh)
>> + nexthop_put(rt->nh);
>
> Shouldn't this be above ip_fib_metrics_put() given nexthop_get() is
> called after ip_fib_metrics_init() ?
>
> Also, shouldn't we call fib6_nh_release() if fib6_nh_init() succeeded
> and we failed later?
similarly I think this cleanup is a separate patch.
>
>> kfree(rt);
>> +out:
>> return ERR_PTR(err);
>> }
>>
>> --
>> 2.31.1
>>
Powered by blists - more mailing lists