[PATCH net-next 0/6] rtnetlink: fix initial rtnl pushdown fallout

[PATCH net-next 0/6] rtnetlink: fix initial rtnl pushdown fallout

[Florian Westphal]

This series fixes various bugs and splats reported since the
allow-handler-to-run-with-no-rtnl series went in.

Last patch adds a script that can be used to add further
tests in case more bugs are reported.
In case you prefer reverting the original series instead of
fixing fallout I can resend this patch on its own.

[PATCH net-next 1/6] rtnetlink: use rcu_dereference_raw to silence rcu splat

[Florian Westphal]

Ido reports a rcu splat in __rtnl_register.
The splat is correct; as rtnl_register doesn't grab any logs
and doesn't use rcu locks either.  It has always been like this.
handler families are not registered in parallel so there are no
races wrt. the kmalloc ordering.

The only reason to use rcu_dereference in the first place was to
avoid sparse from complaining about this.

Thus this switches to _raw() to not have rcu checks here.

The alternative is to add rtnl locking to register/unregister,
however, I don't see a compelling reason to do so as this has been
lockless for the past twenty years or so.

Fixes: 6853dd4881 ("rtnetlink: protect handler table with rcu")
Reported-by: Ido Schimmel <idosch@idosch.org>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/core/rtnetlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index dd4e50dfa248..a5bc5bd0dc12 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -172,7 +172,7 @@ int __rtnl_register(int protocol, int msgtype,
 	BUG_ON(protocol < 0 || protocol > RTNL_FAMILY_MAX);
 	msgindex = rtm_msgindex(msgtype);
 
-	tab = rcu_dereference(rtnl_msg_handlers[protocol]);
+	tab = rcu_dereference_raw(rtnl_msg_handlers[protocol]);
 	if (tab == NULL) {
 		tab = kcalloc(RTM_NR_MSGTYPES, sizeof(*tab), GFP_KERNEL);
 		if (tab == NULL)
-- 
2.13.0

Re: [PATCH net-next 1/6] rtnetlink: use rcu_dereference_raw to silence rcu splat

[Ido Schimmel]

On Thu, Aug 10, 2017 at 04:52:57PM +0200, Florian Westphal wrote:
> Ido reports a rcu splat in __rtnl_register.
> The splat is correct; as rtnl_register doesn't grab any logs
> and doesn't use rcu locks either.  It has always been like this.
> handler families are not registered in parallel so there are no
> races wrt. the kmalloc ordering.
> 
> The only reason to use rcu_dereference in the first place was to
> avoid sparse from complaining about this.
> 
> Thus this switches to _raw() to not have rcu checks here.
> 
> The alternative is to add rtnl locking to register/unregister,
> however, I don't see a compelling reason to do so as this has been
> lockless for the past twenty years or so.
> 
> Fixes: 6853dd4881 ("rtnetlink: protect handler table with rcu")
> Reported-by: Ido Schimmel <idosch@idosch.org>
> Signed-off-by: Florian Westphal <fw@strlen.de>

Tested-by: Ido Schimmel <idosch@mellanox.com>

Thanks!

[PATCH net-next 2/6] rtnetlink: do not use RTM_GETLINK directly

[Florian Westphal]

Userspace sends RTM_GETLINK type, but the kernel substracts
RTM_BASE from this, i.e. 'type' doesn't contain RTM_GETLINK
anymore but instead RTM_GETLINK - RTM_BASE.

This caused the calcit callback to not be invoked when it
should have been (and vice versa).

While at it, also fix a off-by one when checking family index. vs
handler array size.

Fixes: e1fa6d216dd ("rtnetlink: call rtnl_calcit directly")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/core/rtnetlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a5bc5bd0dc12..a9b5ebc1af21 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4167,7 +4167,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (kind != 2 && !netlink_net_capable(skb, CAP_NET_ADMIN))
 		return -EPERM;
 
-	if (family > ARRAY_SIZE(rtnl_msg_handlers))
+	if (family >= ARRAY_SIZE(rtnl_msg_handlers))
 		family = PF_UNSPEC;
 
 	rcu_read_lock();
@@ -4196,7 +4196,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 		refcount_inc(&rtnl_msg_handlers_ref[family]);
 
-		if (type == RTM_GETLINK)
+		if (type == RTM_GETLINK - RTM_BASE)
 			min_dump_alloc = rtnl_calcit(skb, nlh);
 
 		rcu_read_unlock();
-- 
2.13.0

[PATCH net-next 3/6] rtnetlink: switch rtnl_link_get_slave_info_data_size to rcu

[Florian Westphal]

David Ahern reports following splat:
 RTNL: assertion failed at net/core/dev.c (5717)
 netdev_master_upper_dev_get+0x5f/0x70
 if_nlmsg_size+0x158/0x240
 rtnl_calcit.isra.26+0xa3/0xf0

rtnl_link_get_slave_info_data_size currently assumes RTNL protection, but
there appears to be no hard requirement for this, so use rcu instead.

At the time of this writing, there are three 'get_slave_size' callbacks
(now invoked under rcu): bond_get_slave_size, vrf_get_slave_size and
br_port_get_slave_size, all return constant only (i.e. they don't sleep).

Fixes: 6853dd488119 ("rtnetlink: protect handler table with rcu")
Reported-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/core/rtnetlink.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a9b5ebc1af21..087f2434813a 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -402,16 +402,24 @@ static size_t rtnl_link_get_slave_info_data_size(const struct net_device *dev)
 {
 	struct net_device *master_dev;
 	const struct rtnl_link_ops *ops;
+	size_t size = 0;
 
-	master_dev = netdev_master_upper_dev_get((struct net_device *) dev);
+	rcu_read_lock();
+
+	master_dev = netdev_master_upper_dev_get_rcu((struct net_device *)dev);
 	if (!master_dev)
-		return 0;
+		goto out;
+
 	ops = master_dev->rtnl_link_ops;
 	if (!ops || !ops->get_slave_size)
-		return 0;
+		goto out;
 	/* IFLA_INFO_SLAVE_DATA + nested data */
-	return nla_total_size(sizeof(struct nlattr)) +
+	size = nla_total_size(sizeof(struct nlattr)) +
 	       ops->get_slave_size(master_dev, dev);
+
+out:
+	rcu_read_unlock();
+	return size;
 }
 
 static size_t rtnl_link_get_size(const struct net_device *dev)
-- 
2.13.0

Re: [PATCH net-next 3/6] rtnetlink: switch rtnl_link_get_slave_info_data_size to rcu

[David Ahern]

On 8/10/17 8:52 AM, Florian Westphal wrote:
> David Ahern reports following splat:
>  RTNL: assertion failed at net/core/dev.c (5717)
>  netdev_master_upper_dev_get+0x5f/0x70
>  if_nlmsg_size+0x158/0x240
>  rtnl_calcit.isra.26+0xa3/0xf0
> 
> rtnl_link_get_slave_info_data_size currently assumes RTNL protection, but
> there appears to be no hard requirement for this, so use rcu instead.
> 
> At the time of this writing, there are three 'get_slave_size' callbacks
> (now invoked under rcu): bond_get_slave_size, vrf_get_slave_size and
> br_port_get_slave_size, all return constant only (i.e. they don't sleep).
> 
> Fixes: 6853dd488119 ("rtnetlink: protect handler table with rcu")
> Reported-by: David Ahern <dsahern@gmail.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  net/core/rtnetlink.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)

Acked-by: David Ahern <dsahern@gmail.com>

[PATCH net-next 4/6] rtnetlink: init handler refcounts to 1

[Florian Westphal]

If using CONFIG_REFCOUNT_FULL=y we get following splat:
 refcount_t: increment on 0; use-after-free.
WARNING: CPU: 0 PID: 304 at lib/refcount.c:152 refcount_inc+0x47/0x50
Call Trace:
 rtnetlink_rcv_msg+0x191/0x260
 ...

This warning is harmless (0 is "no callback running", not "memory
was freed").

Use '1' as the new 'no handler is running' base instead of 0 to avoid
this.

Fixes: 019a316992ee ("rtnetlink: add reference counting to prevent module unload while dump is in progress")
Reported-by: Sabrina Dubroca <sdubroca@redhat.com>
Reported-by: kernel test robot <fengguang.wu@intel.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/core/rtnetlink.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 087f2434813a..59eda6952bc9 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -262,7 +262,7 @@ void rtnl_unregister_all(int protocol)
 
 	synchronize_net();
 
-	while (refcount_read(&rtnl_msg_handlers_ref[protocol]) > 0)
+	while (refcount_read(&rtnl_msg_handlers_ref[protocol]) > 1)
 		schedule();
 	kfree(handlers);
 }
@@ -4324,6 +4324,11 @@ static struct pernet_operations rtnetlink_net_ops = {
 
 void __init rtnetlink_init(void)
 {
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(rtnl_msg_handlers_ref); i++)
+		refcount_set(&rtnl_msg_handlers_ref[i], 1);
+
 	if (register_pernet_subsys(&rtnetlink_net_ops))
 		panic("rtnetlink_init: cannot initialize rtnetlink\n");
 
-- 
2.13.0

[PATCH net-next 5/6] rtnetlink: fallback to UNSPEC if current family has no doit callback

[Florian Westphal]

We need to use PF_UNSPEC in case the requested family has no doit
callback, otherwise this now fails with EOPNOTSUPP instead of running the
unspec doit callback, as before.

Fixes: 6853dd488119 ("rtnetlink: protect handler table with rcu")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/core/rtnetlink.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 59eda6952bc9..9e9f1419be60 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4221,6 +4221,12 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return err;
 	}
 
+	doit = READ_ONCE(handlers[type].doit);
+	if (!doit) {
+		family = PF_UNSPEC;
+		handlers = rcu_dereference(rtnl_msg_handlers[family]);
+	}
+
 	flags = READ_ONCE(handlers[type].flags);
 	if (flags & RTNL_FLAG_DOIT_UNLOCKED) {
 		refcount_inc(&rtnl_msg_handlers_ref[family]);
-- 
2.13.0

[PATCH net-next 6/6] selftests: add rtnetlink test script

[Florian Westphal]

add a simple script to exercise some rtnetlink call paths, so KASAN,
lockdep etc. can yell at developer before patches are sent upstream.

This can be extended to also cover bond, team, vrf and the like.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
  This test crashes the kernel, fix is already queued here:
  https://patchwork.ozlabs.org/patch/800144/

 tools/testing/selftests/net/Makefile     |   2 +-
 tools/testing/selftests/net/rtnetlink.sh | 199 +++++++++++++++++++++++++++++++
 2 files changed, 200 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/net/rtnetlink.sh

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 6135a8448900..de1f5772b878 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -3,7 +3,7 @@
 CFLAGS =  -Wall -Wl,--no-as-needed -O2 -g
 CFLAGS += -I../../../../usr/include/
 
-TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh
+TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh rtnetlink.sh
 TEST_GEN_FILES =  socket
 TEST_GEN_FILES += psock_fanout psock_tpacket
 TEST_GEN_FILES += reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh
new file mode 100755
index 000000000000..5b04ad912525
--- /dev/null
+++ b/tools/testing/selftests/net/rtnetlink.sh
@@ -0,0 +1,199 @@
+#!/bin/sh
+#
+# This test is for checking rtnetlink callpaths, and get as much coverage as possible.
+#
+# set -e
+
+devdummy="test-dummy0"
+ret=0
+
+# set global exit status, but never reset nonzero one.
+check_err()
+{
+	if [ $ret -eq 0 ]; then
+		ret=$1
+	fi
+}
+
+kci_add_dummy()
+{
+	ip link add name "$devdummy" type dummy
+	check_err $?
+	ip link set "$devdummy" up
+	check_err $?
+}
+
+kci_del_dummy()
+{
+	ip link del dev "$devdummy"
+	check_err $?
+}
+
+# add a bridge with vlans on top
+kci_test_bridge()
+{
+	devbr="test-br0"
+	vlandev="testbr-vlan1"
+
+	ret=0
+	ip link add name "$devbr" type bridge
+	check_err $?
+
+	ip link set dev "$devdummy" master "$devbr"
+	check_err $?
+
+	ip link set "$devbr" up
+	check_err $?
+
+	ip link add link "$devbr" name "$vlandev" type vlan id 1
+	check_err $?
+	ip addr add dev "$vlandev" 10.200.7.23/30
+	check_err $?
+	ip -6 addr add dev "$vlandev" dead:42::1234/64
+	check_err $?
+	ip -d link > /dev/null
+	check_err $?
+	ip r s t all > /dev/null
+	check_err $?
+	ip -6 addr del dev "$vlandev" dead:42::1234/64
+	check_err $?
+
+	ip link del dev "$vlandev"
+	check_err $?
+	ip link del dev "$devbr"
+	check_err $?
+
+	if [ $ret -ne 0 ];then
+		echo "FAIL: bridge setup"
+		return 1
+	fi
+	echo "PASS: bridge setup"
+
+}
+
+kci_test_gre()
+{
+	gredev=neta
+	rem=10.42.42.1
+	loc=10.0.0.1
+
+	ret=0
+	ip tunnel add $gredev mode gre remote $rem local $loc ttl 1
+	check_err $?
+	ip link set $gredev up
+	check_err $?
+	ip addr add 10.23.7.10 dev $gredev
+	check_err $?
+	ip route add 10.23.8.0/30 dev $gredev
+	check_err $?
+	ip addr add dev "$devdummy" 10.23.7.11/24
+	check_err $?
+	ip link > /dev/null
+	check_err $?
+	ip addr > /dev/null
+	check_err $?
+	ip addr del dev "$devdummy" 10.23.7.11/24
+	check_err $?
+
+	ip link del $gredev
+	check_err $?
+
+	if [ $ret -ne 0 ];then
+		echo "FAIL: gre tunnel endpoint"
+		return 1
+	fi
+	echo "PASS: gre tunnel endpoint"
+}
+
+# tc uses rtnetlink too, for full tc testing
+# please see tools/testing/selftests/tc-testing.
+kci_test_tc()
+{
+	dev=lo
+	ret=0
+
+	tc qdisc add dev "$dev" root handle 1: htb
+	check_err $?
+	tc class add dev "$dev" parent 1: classid 1:10 htb rate 1mbit
+	check_err $?
+	tc filter add dev "$dev" parent 1:0 prio 5 handle ffe: protocol ip u32 divisor 256
+	check_err $?
+	tc filter add dev "$dev" parent 1:0 prio 5 handle ffd: protocol ip u32 divisor 256
+	check_err $?
+	tc filter add dev "$dev" parent 1:0 prio 5 handle ffc: protocol ip u32 divisor 256
+	check_err $?
+	tc filter add dev "$dev" protocol ip parent 1: prio 5 handle ffe:2:3 u32 ht ffe:2: match ip src 10.0.0.3 flowid 1:10
+	check_err $?
+	tc filter add dev "$dev" protocol ip parent 1: prio 5 handle ffe:2:2 u32 ht ffe:2: match ip src 10.0.0.2 flowid 1:10
+	check_err $?
+	tc filter show dev "$dev" parent  1:0 > /dev/null
+	check_err $?
+	tc filter del dev "$dev" protocol ip parent 1: prio 5 handle ffe:2:3 u32
+	check_err $?
+	tc filter show dev "$dev" parent  1:0 > /dev/null
+	check_err $?
+	tc qdisc del dev "$dev" root handle 1: htb
+	check_err $?
+
+	if [ $ret -ne 0 ];then
+		echo "FAIL: tc htb hierarchy"
+		return 1
+	fi
+	echo "PASS: tc htb hierarchy"
+
+}
+
+kci_test_polrouting()
+{
+	ret=0
+	ip rule add fwmark 1 lookup 100
+	check_err $?
+	ip route add local 0.0.0.0/0 dev lo table 100
+	check_err $?
+	ip r s t all > /dev/null
+	check_err $?
+	ip rule del fwmark 1 lookup 100
+	check_err $?
+	ip route del local 0.0.0.0/0 dev lo table 100
+	check_err $?
+
+	if [ $ret -ne 0 ];then
+		echo "FAIL: policy route test"
+		return 1
+	fi
+	echo "PASS: policy routing"
+}
+
+kci_test_rtnl()
+{
+	kci_add_dummy
+	if [ $ret -ne 0 ];then
+		echo "FAIL: cannot add dummy interface"
+		return 1
+	fi
+
+	kci_test_polrouting
+	kci_test_tc
+	kci_test_gre
+	kci_test_bridge
+
+	kci_del_dummy
+}
+
+#check for needed privileges
+if [ "$(id -u)" -ne 0 ];then
+	echo "SKIP: Need root privileges"
+	exit 0
+fi
+
+for x in ip tc;do
+	$x -Version 2>/dev/null >/dev/null
+	if [ $? -ne 0 ];then
+		echo "SKIP: Could not run test without the $x tool"
+		exit 0
+	fi
+done
+
+kci_test_rtnl
+
+exit $ret
-- 
2.13.0

Re: [PATCH net-next 0/6] rtnetlink: fix initial rtnl pushdown fallout

[David Miller]

From: Florian Westphal <fw@strlen.de>
Date: Thu, 10 Aug 2017 16:52:56 +0200

> This series fixes various bugs and splats reported since the
> allow-handler-to-run-with-no-rtnl series went in.
> 
> Last patch adds a script that can be used to add further
> tests in case more bugs are reported.
> In case you prefer reverting the original series instead of
> fixing fallout I can resend this patch on its own.

Series applied, thanks Florian.