[PATCH net-next v2 0/6] rtnetlink: preparation patches for further rtnl lock pushdown/removal

[PATCH net-next v2 0/6] rtnetlink: preparation patches for further rtnl lock pushdown/removal

[Florian Westphal]

First patch adds a rudimentary vrf test case.

Remaining patches split large rtnl_fill_ifinfo into smaller chunks
to better see which parts

1. require rtnl
2. do not require it at all
3. rely on rtnl locking now but could be converted

changes since v2:
- fix subject lines of patches 2 and 5
- drop pure ASSERT_RTNL annotation patch, its not all that useful
- remove two extra ASSERT_RTNL in last patch

[PATCH net-next v2 1/6] selftests: rtnetlink.sh: add rudimentary vrf test

[Florian Westphal]

Cc: David Ahern <dsahern@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tools/testing/selftests/net/rtnetlink.sh | 42 ++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh
index 4b48de565cae..6ee2bbaebcd4 100755
--- a/tools/testing/selftests/net/rtnetlink.sh
+++ b/tools/testing/selftests/net/rtnetlink.sh
@@ -291,6 +291,47 @@ kci_test_ifalias()
 	echo "PASS: set ifalias $namewant for $devdummy"
 }
 
+kci_test_vrf()
+{
+	vrfname="test-vrf"
+	ret=0
+
+	ip link show type vrf 2>/dev/null
+	if [ $? -ne 0 ]; then
+		echo "SKIP: vrf: iproute2 too old"
+		return 0
+	fi
+
+	ip link add "$vrfname" type vrf table 10
+	check_err $?
+	if [ $ret -ne 0 ];then
+		echo "FAIL: can't add vrf interface, skipping test"
+		return 0
+	fi
+
+	ip -br link show type vrf | grep -q "$vrfname"
+	check_err $?
+	if [ $ret -ne 0 ];then
+		echo "FAIL: created vrf device not found"
+		return 1
+	fi
+
+        ip link set dev "$vrfname" up
+	check_err $?
+
+	ip link set dev "$devdummy" master "$vrfname"
+	check_err $?
+	ip link del dev "$vrfname"
+	check_err $?
+
+	if [ $ret -ne 0 ];then
+		echo "FAIL: vrf"
+		return 1
+	fi
+
+	echo "PASS: vrf"
+}
+
 kci_test_rtnl()
 {
 	kci_add_dummy
@@ -306,6 +347,7 @@ kci_test_rtnl()
 	kci_test_bridge
 	kci_test_addrlabel
 	kci_test_ifalias
+	kci_test_vrf
 
 	kci_del_dummy
 }
-- 
2.13.5

[PATCH net-next v2 2/6] rtnetlink: add helper to put master ifindex

[Florian Westphal]

rtnl_fill_ifinfo currently requires caller to hold the rtnl mutex.
Unfortunately the function is quite large which makes it harder to see
which spots need the lock, which spots assume it and which ones could do
without.

Add helpers to factor out the ifindex dumping.

One helper can use rcu to remove rtnl dependency.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/core/rtnetlink.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a78fd61da0ec..c801212ee40e 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1307,6 +1307,31 @@ static u32 rtnl_get_event(unsigned long event)
 	return rtnl_event_type;
 }
 
+static int put_master_ifindex(struct sk_buff *skb, struct net_device *dev)
+{
+	const struct net_device *upper_dev;
+	int ret = 0;
+
+	rcu_read_lock();
+
+	upper_dev = netdev_master_upper_dev_get_rcu(dev);
+	if (upper_dev)
+		ret = nla_put_u32(skb, IFLA_MASTER, upper_dev->ifindex);
+
+	rcu_read_unlock();
+	return ret;
+}
+
+static int nla_put_iflink(struct sk_buff *skb, const struct net_device *dev)
+{
+	int ifindex = dev_get_iflink(dev);
+
+	if (dev->ifindex == ifindex)
+		return 0;
+
+	return nla_put_u32(skb, IFLA_LINK, ifindex);
+}
+
 static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			    int type, u32 pid, u32 seq, u32 change,
 			    unsigned int flags, u32 ext_filter_mask,
@@ -1316,7 +1341,6 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 	struct nlmsghdr *nlh;
 	struct nlattr *af_spec;
 	struct rtnl_af_ops *af_ops;
-	struct net_device *upper_dev = netdev_master_upper_dev_get(dev);
 
 	ASSERT_RTNL();
 	nlh = nlmsg_put(skb, pid, seq, type, sizeof(*ifm), flags);
@@ -1345,10 +1369,8 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 #ifdef CONFIG_RPS
 	    nla_put_u32(skb, IFLA_NUM_RX_QUEUES, dev->num_rx_queues) ||
 #endif
-	    (dev->ifindex != dev_get_iflink(dev) &&
-	     nla_put_u32(skb, IFLA_LINK, dev_get_iflink(dev))) ||
-	    (upper_dev &&
-	     nla_put_u32(skb, IFLA_MASTER, upper_dev->ifindex)) ||
+	    nla_put_iflink(skb, dev) ||
+	    put_master_ifindex(skb, dev) ||
 	    nla_put_u8(skb, IFLA_CARRIER, netif_carrier_ok(dev)) ||
 	    (dev->qdisc &&
 	     nla_put_string(skb, IFLA_QDISC, dev->qdisc->ops->id)) ||
-- 
2.13.5

[PATCH net-next v2 3/6] rtnetlink: add helper to dump qdisc name

[Florian Westphal]

We can use rcu here to make this safe even if we would not hold rtnl:
qdisc_destroy uses call_rcu to free the Qdisc struct.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/core/rtnetlink.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index c801212ee40e..ad3f27da37a8 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1332,6 +1332,19 @@ static int nla_put_iflink(struct sk_buff *skb, const struct net_device *dev)
 	return nla_put_u32(skb, IFLA_LINK, ifindex);
 }
 
+static int nla_put_qdisc(struct sk_buff *skb, struct net_device *dev)
+{
+	struct Qdisc *q;
+	int ret = 0;
+
+	rcu_read_lock();
+	q = READ_ONCE(dev->qdisc);
+	if (q)
+		ret = nla_put_string(skb, IFLA_QDISC, q->ops->id);
+	rcu_read_unlock();
+	return ret;
+}
+
 static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			    int type, u32 pid, u32 seq, u32 change,
 			    unsigned int flags, u32 ext_filter_mask,
@@ -1372,8 +1385,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 	    nla_put_iflink(skb, dev) ||
 	    put_master_ifindex(skb, dev) ||
 	    nla_put_u8(skb, IFLA_CARRIER, netif_carrier_ok(dev)) ||
-	    (dev->qdisc &&
-	     nla_put_string(skb, IFLA_QDISC, dev->qdisc->ops->id)) ||
+	    nla_put_qdisc(skb, dev) ||
 	    (dev->ifalias &&
 	     nla_put_string(skb, IFLA_IFALIAS, dev->ifalias)) ||
 	    nla_put_u32(skb, IFLA_CARRIER_CHANGES,
-- 
2.13.5

[PATCH net-next v2 4/6] rtnetlink: add helper to dump ifalias

[Florian Westphal]

ifalias is currently protected by rtnl mutex, add assertion
as a reminder.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/core/rtnetlink.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index ad3f27da37a8..42ff582a010e 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1345,6 +1345,16 @@ static int nla_put_qdisc(struct sk_buff *skb, struct net_device *dev)
 	return ret;
 }
 
+static noinline int nla_put_ifalias(struct sk_buff *skb, struct net_device *dev)
+{
+	ASSERT_RTNL();
+
+	if (dev->ifalias)
+		return nla_put_string(skb, IFLA_IFALIAS, dev->ifalias);
+
+	return 0;
+}
+
 static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			    int type, u32 pid, u32 seq, u32 change,
 			    unsigned int flags, u32 ext_filter_mask,
@@ -1386,8 +1396,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 	    put_master_ifindex(skb, dev) ||
 	    nla_put_u8(skb, IFLA_CARRIER, netif_carrier_ok(dev)) ||
 	    nla_put_qdisc(skb, dev) ||
-	    (dev->ifalias &&
-	     nla_put_string(skb, IFLA_IFALIAS, dev->ifalias)) ||
+	    nla_put_ifalias(skb, dev) ||
 	    nla_put_u32(skb, IFLA_CARRIER_CHANGES,
 			atomic_read(&dev->carrier_changes)) ||
 	    nla_put_u8(skb, IFLA_PROTO_DOWN, dev->proto_down))
-- 
2.13.5

[PATCH net-next v2 5/6] rtnetlink: add helpers to dump vf and netnsid information

[Florian Westphal]

similar to earlier patches, split out more parts of this function to
better see what is happening and where we assume rtnl is locked.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/core/rtnetlink.c | 80 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 30 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 42ff582a010e..590823f70cc3 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1211,6 +1211,36 @@ static noinline_for_stack int rtnl_fill_vfinfo(struct sk_buff *skb,
 	return -EMSGSIZE;
 }
 
+static noinline_for_stack int rtnl_fill_vf(struct sk_buff *skb,
+					   struct net_device *dev,
+					   u32 ext_filter_mask)
+{
+	struct nlattr *vfinfo;
+	int i, num_vfs;
+
+	if (!dev->dev.parent || ((ext_filter_mask & RTEXT_FILTER_VF) == 0))
+		return 0;
+
+	num_vfs = dev_num_vf(dev->dev.parent);
+	if (nla_put_u32(skb, IFLA_NUM_VF, num_vfs))
+		return -EMSGSIZE;
+
+	if (!dev->netdev_ops->ndo_get_vf_config)
+		return 0;
+
+	vfinfo = nla_nest_start(skb, IFLA_VFINFO_LIST);
+	if (!vfinfo)
+		return -EMSGSIZE;
+
+	for (i = 0; i < num_vfs; i++) {
+		if (rtnl_fill_vfinfo(skb, dev, i, vfinfo))
+			return -EMSGSIZE;
+	}
+
+	nla_nest_end(skb, vfinfo);
+	return 0;
+}
+
 static int rtnl_fill_link_ifmap(struct sk_buff *skb, struct net_device *dev)
 {
 	struct rtnl_link_ifmap map;
@@ -1355,6 +1385,23 @@ static noinline int nla_put_ifalias(struct sk_buff *skb, struct net_device *dev)
 	return 0;
 }
 
+static noinline int rtnl_fill_link_netnsid(struct sk_buff *skb,
+					   const struct net_device *dev)
+{
+	if (dev->rtnl_link_ops && dev->rtnl_link_ops->get_link_net) {
+		struct net *link_net = dev->rtnl_link_ops->get_link_net(dev);
+
+		if (!net_eq(dev_net(dev), link_net)) {
+			int id = peernet2id_alloc(dev_net(dev), link_net);
+
+			if (nla_put_s32(skb, IFLA_LINK_NETNSID, id))
+				return -EMSGSIZE;
+		}
+	}
+
+	return 0;
+}
+
 static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			    int type, u32 pid, u32 seq, u32 change,
 			    unsigned int flags, u32 ext_filter_mask,
@@ -1428,27 +1475,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 	if (rtnl_fill_stats(skb, dev))
 		goto nla_put_failure;
 
-	if (dev->dev.parent && (ext_filter_mask & RTEXT_FILTER_VF) &&
-	    nla_put_u32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent)))
+	if (rtnl_fill_vf(skb, dev, ext_filter_mask))
 		goto nla_put_failure;
 
-	if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent &&
-	    ext_filter_mask & RTEXT_FILTER_VF) {
-		int i;
-		struct nlattr *vfinfo;
-		int num_vfs = dev_num_vf(dev->dev.parent);
-
-		vfinfo = nla_nest_start(skb, IFLA_VFINFO_LIST);
-		if (!vfinfo)
-			goto nla_put_failure;
-		for (i = 0; i < num_vfs; i++) {
-			if (rtnl_fill_vfinfo(skb, dev, i, vfinfo))
-				goto nla_put_failure;
-		}
-
-		nla_nest_end(skb, vfinfo);
-	}
-
 	if (rtnl_port_fill(skb, dev, ext_filter_mask))
 		goto nla_put_failure;
 
@@ -1460,17 +1489,8 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			goto nla_put_failure;
 	}
 
-	if (dev->rtnl_link_ops &&
-	    dev->rtnl_link_ops->get_link_net) {
-		struct net *link_net = dev->rtnl_link_ops->get_link_net(dev);
-
-		if (!net_eq(dev_net(dev), link_net)) {
-			int id = peernet2id_alloc(dev_net(dev), link_net);
-
-			if (nla_put_s32(skb, IFLA_LINK_NETNSID, id))
-				goto nla_put_failure;
-		}
-	}
+	if (rtnl_fill_link_netnsid(skb, dev))
+		goto nla_put_failure;
 
 	if (!(af_spec = nla_nest_start(skb, IFLA_AF_SPEC)))
 		goto nla_put_failure;
-- 
2.13.5

[PATCH net-next v2 6/6] rtnetlink: rtnl_have_link_slave_info doesn't need rtnl

[Florian Westphal]

Switch it to rcu.

rtnl_link_slave_info_fill on to other hand does need rtnl mutex for now so
add an annotation to its caller as a reminder.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Change since v1:
  - don't add ASSERT_RTNL to rtnl_link_slave_info_fill and
  rtnl_link_info_fill they are only called via rtnl_link_fill.

 net/core/rtnetlink.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 590823f70cc3..e6f9e36d9d5a 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -522,11 +522,15 @@ static size_t rtnl_link_get_af_size(const struct net_device *dev,
 static bool rtnl_have_link_slave_info(const struct net_device *dev)
 {
 	struct net_device *master_dev;
+	bool ret = false;
 
-	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 && master_dev->rtnl_link_ops)
-		return true;
-	return false;
+		ret = true;
+	rcu_read_unlock();
+	return ret;
 }
 
 static int rtnl_link_slave_info_fill(struct sk_buff *skb,
@@ -598,6 +602,8 @@ static int rtnl_link_fill(struct sk_buff *skb, const struct net_device *dev)
 	struct nlattr *linkinfo;
 	int err = -EMSGSIZE;
 
+	ASSERT_RTNL();
+
 	linkinfo = nla_nest_start(skb, IFLA_LINKINFO);
 	if (linkinfo == NULL)
 		goto out;
-- 
2.13.5

Re: [PATCH net-next v2 1/6] selftests: rtnetlink.sh: add rudimentary vrf test

[David Ahern]

On 9/22/17 12:10 AM, Florian Westphal wrote:
> Cc: David Ahern <dsahern@gmail.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  tools/testing/selftests/net/rtnetlink.sh | 42 ++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)

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

Re: [PATCH net-next v2 2/6] rtnetlink: add helper to put master ifindex

[David Ahern]

On 9/22/17 12:10 AM, Florian Westphal wrote:
> rtnl_fill_ifinfo currently requires caller to hold the rtnl mutex.
> Unfortunately the function is quite large which makes it harder to see
> which spots need the lock, which spots assume it and which ones could do
> without.
> 
> Add helpers to factor out the ifindex dumping.
> 
> One helper can use rcu to remove rtnl dependency.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  net/core/rtnetlink.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)

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

Re: [PATCH net-next v2 3/6] rtnetlink: add helper to dump qdisc name

[David Ahern]

On 9/22/17 12:10 AM, Florian Westphal wrote:
> We can use rcu here to make this safe even if we would not hold rtnl:
> qdisc_destroy uses call_rcu to free the Qdisc struct.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  net/core/rtnetlink.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)

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

Re: [PATCH net-next v2 1/6] selftests: rtnetlink.sh: add rudimentary vrf test

[David Ahern]

On 9/22/17 12:10 AM, Florian Westphal wrote:
> +kci_test_vrf()
> +{
> +	vrfname="test-vrf"
> +	ret=0
> +
> +	ip link show type vrf 2>/dev/null
> +	if [ $? -ne 0 ]; then
> +		echo "SKIP: vrf: iproute2 too old"
> +		return 0
> +	fi
> +
> +	ip link add "$vrfname" type vrf table 10
> +	check_err $?
> +	if [ $ret -ne 0 ];then
> +		echo "FAIL: can't add vrf interface, skipping test"
> +		return 0
> +	fi
> +
> +	ip -br link show type vrf | grep -q "$vrfname"
> +	check_err $?
> +	if [ $ret -ne 0 ];then
> +		echo "FAIL: created vrf device not found"
> +		return 1
> +	fi
> +
> +        ip link set dev "$vrfname" up

BTW, if there is a v3 of this set, that ip command is shifted - uses
spaces instead of tab.

Re: [PATCH net-next v2 5/6] rtnetlink: add helpers to dump vf and netnsid information

[David Ahern]

On 9/22/17 12:10 AM, Florian Westphal wrote:
> +static noinline_for_stack int rtnl_fill_vf(struct sk_buff *skb,
> +					   struct net_device *dev,
> +					   u32 ext_filter_mask)
> +{
> +	struct nlattr *vfinfo;
> +	int i, num_vfs;
> +
> +	if (!dev->dev.parent || ((ext_filter_mask & RTEXT_FILTER_VF) == 0))
> +		return 0;
> +
> +	num_vfs = dev_num_vf(dev->dev.parent);
> +	if (nla_put_u32(skb, IFLA_NUM_VF, num_vfs))
> +		return -EMSGSIZE;
> +
> +	if (!dev->netdev_ops->ndo_get_vf_config)
> +		return 0;
> +
> +	vfinfo = nla_nest_start(skb, IFLA_VFINFO_LIST);
> +	if (!vfinfo)
> +		return -EMSGSIZE;
> +
> +	for (i = 0; i < num_vfs; i++) {
> +		if (rtnl_fill_vfinfo(skb, dev, i, vfinfo))
> +			return -EMSGSIZE;
> +	}
> +
> +	nla_nest_end(skb, vfinfo);
> +	return 0;
> +}
> +
>  static int rtnl_fill_link_ifmap(struct sk_buff *skb, struct net_device *dev)
>  {
>  	struct rtnl_link_ifmap map;
> @@ -1355,6 +1385,23 @@ static noinline int nla_put_ifalias(struct sk_buff *skb, struct net_device *dev)
>  	return 0;
>  }
>  
> +static noinline int rtnl_fill_link_netnsid(struct sk_buff *skb,
> +					   const struct net_device *dev)
> +{
> +	if (dev->rtnl_link_ops && dev->rtnl_link_ops->get_link_net) {
> +		struct net *link_net = dev->rtnl_link_ops->get_link_net(dev);
> +
> +		if (!net_eq(dev_net(dev), link_net)) {
> +			int id = peernet2id_alloc(dev_net(dev), link_net);
> +
> +			if (nla_put_s32(skb, IFLA_LINK_NETNSID, id))
> +				return -EMSGSIZE;
> +		}
> +	}
> +
> +	return 0;
> +}
> +

No reason to combine vf and netnsid into 1 patch; completely separate topics


>  static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
>  			    int type, u32 pid, u32 seq, u32 change,
>  			    unsigned int flags, u32 ext_filter_mask,
> @@ -1428,27 +1475,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
>  	if (rtnl_fill_stats(skb, dev))
>  		goto nla_put_failure;
>  
> -	if (dev->dev.parent && (ext_filter_mask & RTEXT_FILTER_VF) &&
> -	    nla_put_u32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent)))
> +	if (rtnl_fill_vf(skb, dev, ext_filter_mask))
>  		goto nla_put_failure;
>  
> -	if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent &&
> -	    ext_filter_mask & RTEXT_FILTER_VF) {
> -		int i;
> -		struct nlattr *vfinfo;
> -		int num_vfs = dev_num_vf(dev->dev.parent);
> -
> -		vfinfo = nla_nest_start(skb, IFLA_VFINFO_LIST);
> -		if (!vfinfo)
> -			goto nla_put_failure;
> -		for (i = 0; i < num_vfs; i++) {
> -			if (rtnl_fill_vfinfo(skb, dev, i, vfinfo))
> -				goto nla_put_failure;
> -		}
> -
> -		nla_nest_end(skb, vfinfo);
> -	}
> -
>  	if (rtnl_port_fill(skb, dev, ext_filter_mask))
>  		goto nla_put_failure;
>  
> @@ -1460,17 +1489,8 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
>  			goto nla_put_failure;
>  	}
>  
> -	if (dev->rtnl_link_ops &&
> -	    dev->rtnl_link_ops->get_link_net) {
> -		struct net *link_net = dev->rtnl_link_ops->get_link_net(dev);
> -
> -		if (!net_eq(dev_net(dev), link_net)) {
> -			int id = peernet2id_alloc(dev_net(dev), link_net);
> -
> -			if (nla_put_s32(skb, IFLA_LINK_NETNSID, id))
> -				goto nla_put_failure;
> -		}
> -	}
> +	if (rtnl_fill_link_netnsid(skb, dev))
> +		goto nla_put_failure;
>  
>  	if (!(af_spec = nla_nest_start(skb, IFLA_AF_SPEC)))
>  		goto nla_put_failure;
> 

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

Re: [PATCH net-next v2 4/6] rtnetlink: add helper to dump ifalias

[David Ahern]

On 9/22/17 12:10 AM, Florian Westphal wrote:
> ifalias is currently protected by rtnl mutex, add assertion
> as a reminder.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  net/core/rtnetlink.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index ad3f27da37a8..42ff582a010e 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1345,6 +1345,16 @@ static int nla_put_qdisc(struct sk_buff *skb, struct net_device *dev)
>  	return ret;
>  }
>  
> +static noinline int nla_put_ifalias(struct sk_buff *skb, struct net_device *dev)
> +{
> +	ASSERT_RTNL();

The assert is not needed given the code path.

> +
> +	if (dev->ifalias)
> +		return nla_put_string(skb, IFLA_IFALIAS, dev->ifalias);
> +
> +	return 0;
> +}
> +
>  static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
>  			    int type, u32 pid, u32 seq, u32 change,
>  			    unsigned int flags, u32 ext_filter_mask,
> @@ -1386,8 +1396,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
>  	    put_master_ifindex(skb, dev) ||
>  	    nla_put_u8(skb, IFLA_CARRIER, netif_carrier_ok(dev)) ||
>  	    nla_put_qdisc(skb, dev) ||
> -	    (dev->ifalias &&
> -	     nla_put_string(skb, IFLA_IFALIAS, dev->ifalias)) ||
> +	    nla_put_ifalias(skb, dev) ||
>  	    nla_put_u32(skb, IFLA_CARRIER_CHANGES,
>  			atomic_read(&dev->carrier_changes)) ||
>  	    nla_put_u8(skb, IFLA_PROTO_DOWN, dev->proto_down))
> 

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

Re: [PATCH net-next v2 6/6] rtnetlink: rtnl_have_link_slave_info doesn't need rtnl

[David Ahern]

On 9/22/17 12:10 AM, Florian Westphal wrote:
> Switch it to rcu.
> 
> rtnl_link_slave_info_fill on to other hand does need rtnl mutex for now so
> add an annotation to its caller as a reminder.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  Change since v1:
>   - don't add ASSERT_RTNL to rtnl_link_slave_info_fill and
>   rtnl_link_info_fill they are only called via rtnl_link_fill.
> 
>  net/core/rtnetlink.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 590823f70cc3..e6f9e36d9d5a 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -522,11 +522,15 @@ static size_t rtnl_link_get_af_size(const struct net_device *dev,
>  static bool rtnl_have_link_slave_info(const struct net_device *dev)
>  {
>  	struct net_device *master_dev;
> +	bool ret = false;
>  
> -	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 && master_dev->rtnl_link_ops)
> -		return true;
> -	return false;
> +		ret = true;
> +	rcu_read_unlock();
> +	return ret;
>  }
>  
>  static int rtnl_link_slave_info_fill(struct sk_buff *skb,
> @@ -598,6 +602,8 @@ static int rtnl_link_fill(struct sk_buff *skb, const struct net_device *dev)
>  	struct nlattr *linkinfo;
>  	int err = -EMSGSIZE;
>  
> +	ASSERT_RTNL();

Rather than sprinkling the ASSERT_RTNL why not just add a comment above
the function which is done in many places. Since this is really meant as
your notes as you remove rtnl usage a comment serves the same purpose.

> +
>  	linkinfo = nla_nest_start(skb, IFLA_LINKINFO);
>  	if (linkinfo == NULL)
>  		goto out;
> 


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

Re: [PATCH net-next v2 3/6] rtnetlink: add helper to dump qdisc name

[Eric Dumazet]

On Fri, 2017-09-22 at 08:10 +0200, Florian Westphal wrote:
> We can use rcu here to make this safe even if we would not hold rtnl:
> qdisc_destroy uses call_rcu to free the Qdisc struct.


Where do you see call_rcu() called from qdisc_destroy() ?

You missed this commit I guess

752fbcc33405d6f8249465e4b2c4e420091bb825
("net_sched: no need to free qdisc in RCU callback")

Re: [PATCH net-next v2 3/6] rtnetlink: add helper to dump qdisc name

[Florian Westphal]

Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2017-09-22 at 08:10 +0200, Florian Westphal wrote:
> > We can use rcu here to make this safe even if we would not hold rtnl:
> > qdisc_destroy uses call_rcu to free the Qdisc struct.
> 
> 
> Where do you see call_rcu() called from qdisc_destroy() ?
> 
> You missed this commit I guess
> 
> 752fbcc33405d6f8249465e4b2c4e420091bb825
> ("net_sched: no need to free qdisc in RCU callback")

Indeed, I did, patch dropped, thanks for the heads-up.