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

On 30/11/2021 19:18, David Ahern wrote:

On 11/30/21 9:45 AM, Nikolay Aleksandrov wrote:

quoted

On 30/11/2021 18:01, David Ahern wrote:

quoted

On 11/30/21 5:40 AM, Ido Schimmel wrote:

quoted

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.

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.

fib6_nh_init should do proper cleanup if it hits an error. Your bug fix
to get nhc_pcpu_rth_output freed should complete that. It can also set
the value to NULL to avoid double free on any code path.

Indeed, that's another way of achieving the same goal.

quoted

quoted

quoted

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.

Yep, this is the point of this patch. :)

quoted

quoted

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.

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

exactly. set it to NULL and make the -net patch as simple as possible

Sure, of course. I'd prefer to make the code consistent w.r.t expectations regardless of
the release cycle, but I don't have a strong preference. I'll post the fix with
NULLing the nh_common pointers.

Cheers,
 Nik