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:

quoted

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.

quoted

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.

quoted

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

quoted

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