Re: [RFC PATCH net] net: ipv6: make fib6_nh_init properly clean after itself on error

[Nikolay Aleksandrov <nikolay@nvidia.com>]

On 30/11/2021 18:01, David Ahern wrote:
> 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.
> 

Actually it is needed, it's not a cleanup or noop. If fib6_nh_init fails after fib_nh_common_init
in the per-cpu allocation then fib6_nh->nh_common's pointers will still be there but
freed, so it will lead to double free. We have to NULL them when freeing if we want to avoid that.

>>
>>> 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.
> 

Yep, this is the point of this 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.
> 

Same thing, fib6_info_destroy_rcu -> fib6_nh_release would double-free the nh_common parts
if fib6_nh_init fails in the per-cpu allocation after fib_nh_common_init.
It is not a cleanup, but a result of the fix. If we want to keep it, we'll have to NULL
the nh_common parts when freeing them in fib_nh_common_release().

> 
>>
>>>  	kfree(rt);
>>> +out:
>>>  	return ERR_PTR(err);
>>>  }
>>>  
>>> -- 
>>> 2.31.1
>>>
>