[net-next PATCH v2 1/1] bring netlink interface to par with brctl show macs

[net-next PATCH v2 1/1] bring netlink interface to par with brctl show macs

[Jamal Hadi Salim]

From: Jamal Hadi Salim <jhs@mojatatu.com>

This patch allows something equivalent to
"brctl showmacs <bridge device>" with iproute2
syntax "bridge fdb show br <bridge device>"
Filtering by bridge is done in the kernel.
The current setup doesnt scale when you have many bridges each
with large fdbs (preliminary fix with the kernel patch).

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/core/rtnetlink.c |   49 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index f31268d..1ad4828 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2509,26 +2509,45 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	int idx = 0;
 	struct net *net = sock_net(skb->sk);
+	const struct net_device_ops *ops;
 	struct net_device *dev;
+	struct ndmsg *ndm;
 
-	rcu_read_lock();
-	for_each_netdev_rcu(net, dev) {
-		if (dev->priv_flags & IFF_BRIDGE_PORT) {
-			struct net_device *br_dev;
-			const struct net_device_ops *ops;
-
-			br_dev = netdev_master_upper_dev_get(dev);
-			ops = br_dev->netdev_ops;
-			if (ops->ndo_fdb_dump)
-				idx = ops->ndo_fdb_dump(skb, cb, dev, idx);
+	ndm = nlmsg_data(cb->nlh);
+	if (ndm->ndm_ifindex) {
+		dev = __dev_get_by_index(net, ndm->ndm_ifindex);
+		if (dev == NULL) {
+			pr_info("PF_BRIDGE: RTM_GETNEIGH with unknown ifindex\n");
+			return -ENODEV;
 		}
 
-		if (dev->netdev_ops->ndo_fdb_dump)
-			idx = dev->netdev_ops->ndo_fdb_dump(skb, cb, dev, idx);
-		else
-			idx = ndo_dflt_fdb_dump(skb, cb, dev, idx);
+		ops = dev->netdev_ops;
+		if (ops->ndo_fdb_dump) {
+			idx = ops->ndo_fdb_dump(skb, cb, dev, idx);
+		} else {
+			pr_info("PF_BRIDGE: RTM_GETNEIGH %s no dumper\n",
+				dev->name);
+			return -EINVAL;
+		}
+	} else {
+		rcu_read_lock();
+		for_each_netdev_rcu(net, dev) {
+			if (dev->priv_flags & IFF_BRIDGE_PORT) {
+				struct net_device *br_dev;
+				br_dev = netdev_master_upper_dev_get(dev);
+				ops = br_dev->netdev_ops;
+				if (ops->ndo_fdb_dump)
+					idx = ops->ndo_fdb_dump(skb, cb, dev, idx);
+			}
+
+			if (dev->netdev_ops->ndo_fdb_dump)
+				idx = dev->netdev_ops->ndo_fdb_dump(skb, cb, dev,
+								    idx);
+			else
+				idx = ndo_dflt_fdb_dump(skb, cb, dev, idx);
+		}
+		rcu_read_unlock();
 	}
-	rcu_read_unlock();
 
 	cb->args[0] = idx;
 	return skb->len;
-- 
1.7.9.5

Re: [net-next PATCH v2 1/1] bring netlink interface to par with brctl show macs

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2324 bytes --]

On Sun, 2014-05-25 at 09:15 -0400, Jamal Hadi Salim wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
> 
> This patch allows something equivalent to
> "brctl showmacs <bridge device>" with iproute2
> syntax "bridge fdb show br <bridge device>"
> Filtering by bridge is done in the kernel.
> The current setup doesnt scale when you have many bridges each
> with large fdbs (preliminary fix with the kernel patch).
> 
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
>  net/core/rtnetlink.c |   49 ++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index f31268d..1ad4828 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -2509,26 +2509,45 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
>  {
>  	int idx = 0;
>  	struct net *net = sock_net(skb->sk);
> +	const struct net_device_ops *ops;
>  	struct net_device *dev;
> +	struct ndmsg *ndm;
>  
> -	rcu_read_lock();
> -	for_each_netdev_rcu(net, dev) {
> -		if (dev->priv_flags & IFF_BRIDGE_PORT) {
> -			struct net_device *br_dev;
> -			const struct net_device_ops *ops;
> -
> -			br_dev = netdev_master_upper_dev_get(dev);
> -			ops = br_dev->netdev_ops;
> -			if (ops->ndo_fdb_dump)
> -				idx = ops->ndo_fdb_dump(skb, cb, dev, idx);
> +	ndm = nlmsg_data(cb->nlh);
> +	if (ndm->ndm_ifindex) {
> +		dev = __dev_get_by_index(net, ndm->ndm_ifindex);
> +		if (dev == NULL) {
> +			pr_info("PF_BRIDGE: RTM_GETNEIGH with unknown ifindex\n");

You left another debug message here.

> +			return -ENODEV;
>  		}
>  
> -		if (dev->netdev_ops->ndo_fdb_dump)
> -			idx = dev->netdev_ops->ndo_fdb_dump(skb, cb, dev, idx);
> -		else
> -			idx = ndo_dflt_fdb_dump(skb, cb, dev, idx);
> +		ops = dev->netdev_ops;
> +		if (ops->ndo_fdb_dump) {
> +			idx = ops->ndo_fdb_dump(skb, cb, dev, idx);
> +		} else {
> +			pr_info("PF_BRIDGE: RTM_GETNEIGH %s no dumper\n",
> +				dev->name);

And here.

> +			return -EINVAL;
> +		}
[...]

Why does this only call the device's own ndo_fdb_dump and not a
bridge-port's bridge's ndo_fdb_dump or ndo_dflt_fdb_dump?

Ben.

-- 
Ben Hutchings
Humans are not rational beings; they are rationalising beings.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [net-next PATCH v2 1/1] bring netlink interface to par with brctl show macs

[Jamal Hadi Salim]

On 05/25/14 22:35, Ben Hutchings wrote:
> On Sun, 2014-05-25 at 09:15 -0400, Jamal Hadi Salim wrote:

>> +		if (dev == NULL) {
>> +			pr_info("PF_BRIDGE: RTM_GETNEIGH with unknown ifindex\n");
>
> You left another debug message here.
>
>> +			return -ENODEV;
>>   		}

>> +			pr_info("PF_BRIDGE: RTM_GETNEIGH %s no dumper\n",
>> +				dev->name);
>
> And here.

Those two just adhere to the coding style used in the rest of the fdb
code. I could remove them and send subsequent patches to remove
equivalent debugs in the rest of the code. To me they seem useful
although i have seen very strong views against them in the past.

>> +			return -EINVAL;
>> +		}
> [...]
>
> Why does this only call the device's own ndo_fdb_dump and not a
> bridge-port's bridge's ndo_fdb_dump or ndo_dflt_fdb_dump?

The target is a specific bridge not a bridge port. Having
said that, it is (probably) possible there are physical or virtual
bridge ports whose master is the targeted bridge that are sheltering
fdb entries we want to dump that are not in the bridge's fdb.
Macvlans or vmdq maybe? What did you have in mind?

cheers,
jamal



> Ben.
>

Re: [net-next PATCH v2 1/1] bring netlink interface to par with brctl show macs

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Mon, 26 May 2014 07:05:01 -0400

> On 05/25/14 22:35, Ben Hutchings wrote:
>> On Sun, 2014-05-25 at 09:15 -0400, Jamal Hadi Salim wrote:
> 
>>> +		if (dev == NULL) {
>>> + pr_info("PF_BRIDGE: RTM_GETNEIGH with unknown ifindex\n");
>>
>> You left another debug message here.
>>
>>> +			return -ENODEV;
>>>   		}
> 
>>> +			pr_info("PF_BRIDGE: RTM_GETNEIGH %s no dumper\n",
>>> +				dev->name);
>>
>> And here.
> 
> Those two just adhere to the coding style used in the rest of the fdb
> code. I could remove them and send subsequent patches to remove
> equivalent debugs in the rest of the code. To me they seem useful
> although i have seen very strong views against them in the past.

I think we have to get rid of these, because they are essentially
user triggerable.