[net-next PATCH 1/2] bridge: fdb dumping takes a filter device

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

 Dumping a bridge fdb dumps every fdb entry
 held. With this change we are going to filter
 on selected bridge port.

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c      |    3 ++-
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c |    5 +++--
 include/linux/netdevice.h                        |    1 +
 include/linux/rtnetlink.h                        |    1 +
 net/bridge/br_fdb.c                              |    5 +++++
 net/bridge/br_private.h                          |    2 +-
 net/core/rtnetlink.c                             |    7 ++++---
 7 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 63147a6..7bc806c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c

@@ -6817,13 +6817,14 @@ static int i40e_ndo_fdb_del(struct ndmsg *ndm,
 static int i40e_ndo_fdb_dump(struct sk_buff *skb,
 			     struct netlink_callback *cb,
 			     struct net_device *dev,
+			     struct net_device *filter_dev,
 			     int idx)
 {
 	struct i40e_netdev_priv *np = netdev_priv(dev);
 	struct i40e_pf *pf = np->vsi->back;
 
 	if (pf->flags & I40E_FLAG_SRIOV_ENABLED)
-		idx = ndo_dflt_fdb_dump(skb, cb, dev, idx);
+		idx = ndo_dflt_fdb_dump(skb, cb, dev, filter_dev, idx);
 
 	return idx;
 }

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
index f06ba90b..233f282 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c

@@ -427,12 +427,13 @@ static int qlcnic_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 }
 
 static int qlcnic_fdb_dump(struct sk_buff *skb, struct netlink_callback *ncb,
-			struct net_device *netdev, int idx)
+			struct net_device *netdev, struct net_device *filter_dev,
+			int idx)
 {
 	struct qlcnic_adapter *adapter = netdev_priv(netdev);
 
 	if (!adapter->fdb_mac_learn)
-		return ndo_dflt_fdb_dump(skb, ncb, netdev, idx);
+		return ndo_dflt_fdb_dump(skb, ncb, netdev, filter_dev, idx);
 
 	if ((adapter->flags & QLCNIC_ESWITCH_ENABLED) ||
 	    qlcnic_sriov_check(adapter))

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 774e539..a464057 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h

@@ -1114,6 +1114,7 @@ struct net_device_ops {
 	int			(*ndo_fdb_dump)(struct sk_buff *skb,
 						struct netlink_callback *cb,
 						struct net_device *dev,
+						struct net_device *filter_dev,
 						int idx);
 
 	int			(*ndo_bridge_setlink)(struct net_device *dev,

diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index 953937e..167bae7 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h

@@ -78,6 +78,7 @@ extern void __rtnl_unlock(void);
 extern int ndo_dflt_fdb_dump(struct sk_buff *skb,
 			     struct netlink_callback *cb,
 			     struct net_device *dev,
+			     struct net_device *filter_dev,
 			     int idx);
 extern int ndo_dflt_fdb_add(struct ndmsg *ndm,
 			    struct nlattr *tb[],

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index b524c36..48449fc 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c

@@ -678,6 +678,7 @@ errout:
 int br_fdb_dump(struct sk_buff *skb,
 		struct netlink_callback *cb,
 		struct net_device *dev,
+		struct net_device *filter_dev,
 		int idx)
 {
 	struct net_bridge *br = netdev_priv(dev);

@@ -693,6 +694,10 @@ int br_fdb_dump(struct sk_buff *skb,
 			if (idx < cb->args[0])
 				goto skip;
 
+			if (filter_dev && (!f->dst || !f->dst->dev ||
+					   f->dst->dev != filter_dev))
+				goto skip;
+
 			if (fdb_fill_info(skb, br, f,
 					  NETLINK_CB(cb->skb).portid,
 					  cb->nlh->nlmsg_seq,

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index bc17210..d72daf3 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h

@@ -398,7 +398,7 @@ int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
 int br_fdb_add(struct ndmsg *nlh, struct nlattr *tb[], struct net_device *dev,
 	       const unsigned char *addr, u16 nlh_flags);
 int br_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
-		struct net_device *dev, int idx);
+		struct net_device *dev, struct net_device *fdev, int idx);
 int br_fdb_sync_static(struct net_bridge *br, struct net_bridge_port *p);
 void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p);
 

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 741b22c..8721f1b 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c

@@ -2494,6 +2494,7 @@ skip:
 int ndo_dflt_fdb_dump(struct sk_buff *skb,
 		      struct netlink_callback *cb,
 		      struct net_device *dev,
+		      struct net_device *filter_dev,
 		      int idx)
 {
 	int err;

@@ -2524,13 +2525,13 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
 			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);
+				idx = ops->ndo_fdb_dump(skb, cb, dev, NULL, idx);
 		}
 
 		if (dev->netdev_ops->ndo_fdb_dump)
-			idx = dev->netdev_ops->ndo_fdb_dump(skb, cb, dev, idx);
+			idx = dev->netdev_ops->ndo_fdb_dump(skb, cb, dev, NULL, idx);
 		else
-			idx = ndo_dflt_fdb_dump(skb, cb, dev, idx);
+			idx = ndo_dflt_fdb_dump(skb, cb, dev, NULL, idx);
 	}
 	rcu_read_unlock();
 

-- 
1.7.9.5

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

Actually better than brctl showmacs because we can filter by bridge
port in the kernel.
The current bridge netlink interface doesnt scale when you have many
bridges each with large fdbs or even bridges with many bridge ports

For example usage look at accompanying iproute2 patch.

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/bridge/br_fdb.c  |   17 +++++++++---
 net/core/rtnetlink.c |   71 +++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 72 insertions(+), 16 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 48449fc..7114382 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c

@@ -694,9 +694,20 @@ int br_fdb_dump(struct sk_buff *skb,
 			if (idx < cb->args[0])
 				goto skip;
 
-			if (filter_dev && (!f->dst || !f->dst->dev ||
-					   f->dst->dev != filter_dev))
-				goto skip;
+			if (filter_dev && (!f->dst || f->dst->dev != filter_dev)) {
+				if (filter_dev != dev)
+					goto skip;
+				else {
+					/* 
+					 * !f->dst is a speacial case for bridge
+					 * It means the MAC belongs to the bridge
+					 * Therefore need a little more filtering
+					 * we only want to dump the !f->dst case
+					 */
+					if (f->dst)
+						goto skip;
+				}
+			}
 
 			if (fdb_fill_info(skb, br, f,
 					  NETLINK_CB(cb->skb).portid,

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 8721f1b..2a3c225 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c

@@ -2512,26 +2512,71 @@ EXPORT_SYMBOL(ndo_dflt_fdb_dump);
 
 static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
 {
-	int idx = 0;
-	struct net *net = sock_net(skb->sk);
 	struct net_device *dev;
+	struct nlattr *tb[IFLA_MAX+1];
+	struct net_device *bdev = NULL; /*pacify stoopid gcc*/
+	struct net_device *br_dev = NULL; /*pacify stoopid gcc*/
+	const struct net_device_ops *ops = NULL; /*pacify stoopid gcc*/
+	struct ifinfomsg *ifm = nlmsg_data(cb->nlh);
+	struct net *net = sock_net(skb->sk);
+	int brport_idx = 0;
+	int br_idx = 0;
+	int idx = 0;
+
+	if (nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb, IFLA_MAX,
+			ifla_policy) == 0) {
+		if (tb[IFLA_MASTER])
+			br_idx = nla_get_u32(tb[IFLA_MASTER]);
+	}
+	
+	brport_idx = ifm->ifi_index;
 
 	rcu_read_lock();
+	if (br_idx) {
+		br_dev = __dev_get_by_index(net, br_idx);
+		if (!br_dev) {
+			rcu_read_unlock();
+			return -ENODEV;
+		}
+		ops = br_dev->netdev_ops;
+		bdev = br_dev;
+	}
+
 	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, NULL, idx);
+
+		if (brport_idx && (dev->ifindex != brport_idx))
+			continue;
+
+		if (!br_idx) { /* user did not specify a specific bridge */
+			if (dev->priv_flags & IFF_BRIDGE_PORT) {
+				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, br_dev,
+								dev, idx);
+			}
+
+			bdev = dev;
+		} else {
+			if (dev != br_dev &&
+			    !(dev->priv_flags & IFF_BRIDGE_PORT))
+				continue;
+
+			if (br_dev != netdev_master_upper_dev_get(dev) &&
+			    !(dev->priv_flags & IFF_EBRIDGE))
+				continue;
+
+			if (dev->priv_flags & IFF_BRIDGE_PORT)
+				idx = ops->ndo_fdb_dump(skb, cb, br_dev,
+							dev, idx);
 		}
 
-		if (dev->netdev_ops->ndo_fdb_dump)
-			idx = dev->netdev_ops->ndo_fdb_dump(skb, cb, dev, NULL, idx);
-		else
+		if (dev->netdev_ops->ndo_fdb_dump) {
+			idx = dev->netdev_ops->ndo_fdb_dump(skb, cb, bdev, dev,
+							    idx);
+		} else {
 			idx = ndo_dflt_fdb_dump(skb, cb, dev, NULL, idx);
+		}
 	}
 	rcu_read_unlock();
 

-- 
1.7.9.5

And now for the tests that Dave doesnt want me to add to the commit ;->

Vlad, the last part should satisfy your earlier comment.

---------
// show all..
root@moja-1:/configs/may30-iprt/bridge# ./bridge fdb show
33:33:00:00:00:01 dev bond0 self permanent
33:33:00:00:00:01 dev dummy0 self permanent
33:33:00:00:00:01 dev ifb0 self permanent
33:33:00:00:00:01 dev ifb1 self permanent
33:33:00:00:00:01 dev eth0 self permanent
01:00:5e:00:00:01 dev eth0 self permanent
33:33:ff:22:01:01 dev eth0 self permanent
02:00:00:12:01:02 dev eth1 vlan 0 master br0 permanent
00:17:42:8a:b4:05 dev eth1 vlan 0 master br0 permanent
00:17:42:8a:b4:07 dev eth1 self permanent
33:33:00:00:00:01 dev eth1 self permanent
33:33:00:00:00:01 dev gretap0 self permanent
da:ac:46:27:d9:53 dev sw1-p1 vlan 0 master br0 permanent
33:33:00:00:00:01 dev sw1-p1 self permanent

//filter by bridge
root@moja-1:/configs/may30-iprt/bridge# ./bridge fdb show br br0
02:00:00:12:01:02 dev eth1 vlan 0 master br0 permanent
00:17:42:8a:b4:05 dev eth1 vlan 0 master br0 permanent
00:17:42:8a:b4:07 dev eth1 self permanent
33:33:00:00:00:01 dev eth1 self permanent
da:ac:46:27:d9:53 dev sw1-p1 vlan 0 master br0 permanent
33:33:00:00:00:01 dev sw1-p1 self permanent

// bridge sw1 has no ports attached
root@moja-1:/configs/may30-iprt/bridge# ./bridge fdb show br sw1

//filter by port
root@moja-1:/configs/may30-iprt/bridge# ./bridge fdb show brport eth1
02:00:00:12:01:02 vlan 0 master br0 permanent
00:17:42:8a:b4:05 vlan 0 master br0 permanent
00:17:42:8a:b4:07 self permanent
33:33:00:00:00:01 self permanent

// filter by port + bridge
root@moja-1:/configs/may30-iprt/bridge# ./bridge fdb show br br0 brport 
sw1-p1
da:ac:46:27:d9:53 vlan 0 master br0 permanent
33:33:00:00:00:01 self permanent

// for shits and giggles, lets change the mac that br0 uses
// Note: a magical fdb entry with no brport is added ...
root@moja-1:/configs/may30-iprt/bridge# ip link set dev br0 address 
02:00:00:12:01:04

// lets see if we can see it ..
root@moja-1:/configs/may30-iprt/bridge# ./bridge fdb show
33:33:00:00:00:01 dev bond0 self permanent
33:33:00:00:00:01 dev dummy0 self permanent
33:33:00:00:00:01 dev ifb0 self permanent
33:33:00:00:00:01 dev ifb1 self permanent
33:33:00:00:00:01 dev eth0 self permanent
01:00:5e:00:00:01 dev eth0 self permanent
33:33:ff:22:01:01 dev eth0 self permanent
02:00:00:12:01:02 dev eth1 vlan 0 master br0 permanent
00:17:42:8a:b4:05 dev eth1 vlan 0 master br0 permanent
00:17:42:8a:b4:07 dev eth1 self permanent
33:33:00:00:00:01 dev eth1 self permanent
33:33:00:00:00:01 dev gretap0 self permanent
02:00:00:12:01:04 dev br0 vlan 0 master br0 permanent
da:ac:46:27:d9:53 dev sw1-p1 vlan 0 master br0 permanent
33:33:00:00:00:01 dev sw1-p1 self permanent

//yep, it is there.
//can we see it if we filter by bridge?
root@moja-1:/configs/may30-iprt/bridge# ./bridge fdb show br br0
02:00:00:12:01:02 dev eth1 vlan 0 master br0 permanent
00:17:42:8a:b4:05 dev eth1 vlan 0 master br0 permanent
00:17:42:8a:b4:07 dev eth1 self permanent
33:33:00:00:00:01 dev eth1 self permanent
02:00:00:12:01:04 dev br0 vlan 0 master br0 permanent
da:ac:46:27:d9:53 dev sw1-p1 vlan 0 master br0 permanent
33:33:00:00:00:01 dev sw1-p1 self permanent
---------

cheers,
jamal





On 06/07/14 10:27, Jamal Hadi Salim wrote:

On 06/07/2014 10:27 AM, Jamal Hadi Salim wrote:

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

Actually better than brctl showmacs because we can filter by bridge
port in the kernel.
The current bridge netlink interface doesnt scale when you have many
bridges each with large fdbs or even bridges with many bridge ports

For example usage look at accompanying iproute2 patch.

The code was a bit tough to follow.  I think the main reason is
that you now always pass a filtering devices even when there was
no filtering information requested.

I am wondering if it could be made simpler...

I think this can be outside of the rcu since you hold an rtnl at this time.

-vlad

 	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, NULL, idx);
+
+		if (brport_idx && (dev->ifindex != brport_idx))
+			continue;
+
+		if (!br_idx) { /* user did not specify a specific bridge */
+			if (dev->priv_flags & IFF_BRIDGE_PORT) {
+				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, br_dev,
+								dev, idx);
+			}
+
+			bdev = dev;
+		} else {
+			if (dev != br_dev &&
+			    !(dev->priv_flags & IFF_BRIDGE_PORT))
+				continue;
+
+			if (br_dev != netdev_master_upper_dev_get(dev) &&
+			    !(dev->priv_flags & IFF_EBRIDGE))
+				continue;
+
+			if (dev->priv_flags & IFF_BRIDGE_PORT)
+				idx = ops->ndo_fdb_dump(skb, cb, br_dev,
+							dev, idx);
 		}
 
-		if (dev->netdev_ops->ndo_fdb_dump)
-			idx = dev->netdev_ops->ndo_fdb_dump(skb, cb, dev, NULL, idx);
-		else
+		if (dev->netdev_ops->ndo_fdb_dump) {
+			idx = dev->netdev_ops->ndo_fdb_dump(skb, cb, bdev, dev,
+							    idx);
+		} else {
 			idx = ndo_dflt_fdb_dump(skb, cb, dev, NULL, idx);
+		}
 	}
 	rcu_read_unlock();
 

On 06/09/14 12:41, Vlad Yasevich wrote:

On 06/07/2014 10:27 AM, Jamal Hadi Salim wrote:

quoted

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

Actually better than brctl showmacs because we can filter by bridge
port in the kernel.
The current bridge netlink interface doesnt scale when you have many
bridges each with large fdbs or even bridges with many bridge ports

For example usage look at accompanying iproute2 patch.

The code was a bit tough to follow.  I think the main reason is
that you now always pass a filtering devices even when there was
no filtering information requested.

I am wondering if it could be made simpler...

The patch may be hard to follow i think. I cant think of a simple
way to do filtering by br and brport. If you have suggestions, shoot.

quoted

  	rcu_read_lock();
+	if (br_idx) {
+		br_dev = __dev_get_by_index(net, br_idx);
+		if (!br_dev) {
+			rcu_read_unlock();
+			return -ENODEV;
+		}
+		ops = br_dev->netdev_ops;
+		bdev = br_dev;
+	}
+

I think this can be outside of the rcu since you hold an rtnl at this time.

Will fix on next iteration.

cheers,
jamal

On 06/10/2014 07:41 AM, Jamal Hadi Salim wrote:

On 06/09/14 12:41, Vlad Yasevich wrote:

quoted

On 06/07/2014 10:27 AM, Jamal Hadi Salim wrote:

quoted

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

Actually better than brctl showmacs because we can filter by bridge
port in the kernel.
The current bridge netlink interface doesnt scale when you have many
bridges each with large fdbs or even bridges with many bridge ports

For example usage look at accompanying iproute2 patch.

The code was a bit tough to follow.  I think the main reason is
that you now always pass a filtering devices even when there was
no filtering information requested.

I am wondering if it could be made simpler...

The patch may be hard to follow i think. I cant think of a simple
way to do filtering by br and brport. If you have suggestions, shoot.

I gave it some thought and I think something like the following
pseudo-code would work.

dump_dev_fdbs(dev, filter)
{
        if (dev->dumper)
                dev->ndo_dumper(dev, filter);
        else
                default_dumper(dev, filter);
}

for_each_netdev() {
        if (bridge_filter) {
                if (dev->index != bridge_filter)
                        skip;

                dump_dev_fdbs(dev, port_filter);
        } else {
                if (port_filter) {
                        if (bridge_port &&
                            dev->index != port_filter)
                                skip;

                }

                if (bridge_port) {
                        br_dev = get_bridge();
                        dump_dev_fdbs(br_dev, port_filter);
                }

                dump_dev_fdbs(dev, port_filter);
        }
}


What do you think?

-vlad

quoted

quoted

      rcu_read_lock();
+    if (br_idx) {
+        br_dev = __dev_get_by_index(net, br_idx);
+        if (!br_dev) {
+            rcu_read_unlock();
+            return -ENODEV;
+        }
+        ops = br_dev->netdev_ops;
+        bdev = br_dev;
+    }
+

I think this can be outside of the rcu since you hold an rtnl at this
time.

Will fix on next iteration.

cheers,
jamal

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Sat, 07 Jun 2014 10:34:22 -0400

And now for the tests that Dave doesnt want me to add to the commit
;->

My objections were to large subject lines, ones that were the size
of a commit message body :-)

I don't care if you write a spy novel in the commit message body
itself, the more information the better.

So please put these tests into your next iteration.

On 06/10/14 09:25, Vlad Yasevich wrote:

I gave it some thought and I think something like the following
pseudo-code would work.

dump_dev_fdbs(dev, filter)
{
         if (dev->dumper)
                 dev->ndo_dumper(dev, filter);
         else
                 default_dumper(dev, filter);
}

for_each_netdev() {
         if (bridge_filter) {
                 if (dev->index != bridge_filter)
                         skip;

                 dump_dev_fdbs(dev, port_filter);
         } else {
                 if (port_filter) {
                         if (bridge_port &&
                             dev->index != port_filter)
                                 skip;

                 }

                 if (bridge_port) {
                         br_dev = get_bridge();
                         dump_dev_fdbs(br_dev, port_filter);
                 }

                 dump_dev_fdbs(dev, port_filter);
         }
}


What do you think?

Too bad i missed the net-next submission.
I am not sure what you suggest above will improve upon readability,
but i will take another run at it when Dave opens up.
I know reading the patch was hard - the code was not as bad.
We'll see.

cheers,
jamal

On 06/15/14 11:28, Jamal Hadi Salim wrote:

Too bad i missed the net-next submission.
I am not sure what you suggest above will improve upon readability,
but i will take another run at it when Dave opens up.
I know reading the patch was hard - the code was not as bad.
We'll see.

had some cycles this morning - so i sent out a new version.
Ive tried to simplify; couldnt make it any cleverr without making
it fail for some specific use case.

cheers,
jamal