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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YaYbusXHbVQUXpmB@shredder>
Date:   Tue, 30 Nov 2021 14:40:26 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     Nikolay Aleksandrov <razor@...ckwall.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        dsahern@...il.com, Nikolay Aleksandrov <nikolay@...dia.com>
Subject: Re: [RFC PATCH net] net: ipv6: make fib6_nh_init properly clean
 after itself on error

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

> 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

> @@ -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?

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