netdev - [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-next>] [day] [month] [year] [list]

Date:   Mon, 29 Nov 2021 16:11:51 +0200
From:   Nikolay Aleksandrov <razor@...ckwall.org>
To:     netdev@...r.kernel.org
Cc:     davem@...emloft.net, kuba@...nel.org, dsahern@...il.com,
        idosch@...sch.org, Nikolay Aleksandrov <nikolay@...dia.com>
Subject: [RFC PATCH net] net: ipv6: make fib6_nh_init properly clean after itself on error

From: Nikolay Aleksandrov <nikolay@...dia.com>

Currently we have different cleanup expectations by different users of
fib6_nh_init:
 1. nh_create_ipv6
 - calls fib6_nh_release manually which does full cleanup

 2. ip6_route_info_create
 - calls fib6_info_release to drop refs to 0 and schedules rcu call
   for fib6_info_destroy_rcu() which also does full cleanup

 3. fib_check_nh_v6_gw
 - doesn't do any cleanup on error, expects fib6_nh_init to clean up
   after itself fully (nhc_pcpu_rth_output per-cpu memory leak on error)

We can alter fib6_nh_init to properly cleanup after itself so
expectations would be the same for everyone and noone would have to do
anything in such case. It is safe because the route is not inserted yet
so the fib6_nh should not be visible at fib6_nh_init point, thus it
should be possible to free up all resources in its error path. The
problems (and leaks) are because it doesn't free all resources in its
error path, the nhc_pcpu_rth_output per-cpu allocation done by
fib_nh_common_init is not cleaned up and it expects its callers to clean
up if an error occurred after it, e.g. the dst per-cpu allocation
might fail. This change allows us to fix the memory leak at 3. and also
to simplify nh_create_ipv6 and remove the special error handling.

Fixes: 717a8f5b2923 ("ipv4: Add fib_check_nh_v6_gw")
Signed-off-by: Nikolay Aleksandrov <nikolay@...dia.com>
---
Sending as RFC to see what people think. I've tested this patch under
heavy load with replacing/traffic forwarding/nexthop add/del etc.
I've also tested error paths by adding artificial ENOMEM errors in
different fib6_nh_init stages while running kmemleak.

 net/ipv4/nexthop.c |  8 +-------
 net/ipv6/route.c   | 15 +++++++++------
 2 files changed, 10 insertions(+), 13 deletions(-)

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;
 }
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);
 	}
@@ -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);
 	kfree(rt);
+out:
 	return ERR_PTR(err);
 }
 
-- 
2.31.1