RFC: bridge get fdb by bridge device

RFC: bridge get fdb by bridge device

[Jamal Hadi Salim]

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


This patch allows something equivalent to
"brctl showmacs <bridge device>" with iproute2
syntax "bridge link 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).

iproute2 allows filtering by bridge port, example:
"bridge link br br1234 dev port1234"
but the filtering is done in user space.
In a future patch i would like to do the port filtering
in the kernel. As well, adding a MAC filter in the kernel
makes sense.

Kernel patch is against net-next.

cheers,
jamal

[-- Attachment #2: bridge-fdb-filter1 --]
[-- Type: text/plain, Size: 2009 bytes --]

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 393b1bc..507ea4e 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2423,26 +2423,50 @@ 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->priv_flags & IFF_EBRIDGE)) {
+			pr_info("PF_BRIDGE: RTM_GETNEIGH %s not a bridge device\n",
+				dev->name);
+			return -EINVAL;
 		}
+		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);
+			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;

[-- Attachment #3: iprt-fdb-brfilter1 --]
[-- Type: text/plain, Size: 1896 bytes --]

diff --git a/bridge/fdb.c b/bridge/fdb.c
index e2e53f1..f3073d6 100644
--- a/bridge/fdb.c
+++ b/bridge/fdb.c
@@ -33,7 +33,7 @@ static void usage(void)
 	fprintf(stderr, "Usage: bridge fdb { add | append | del | replace } ADDR dev DEV {self|master} [ temp ]\n"
 		        "              [router] [ dst IPADDR] [ vlan VID ]\n"
 		        "              [ port PORT] [ vni VNI ] [via DEV]\n");
-	fprintf(stderr, "       bridge fdb {show} [ dev DEV ]\n");
+	fprintf(stderr, "       bridge fdb {show} [ br BRDEV ] [ dev DEV ]\n");
 	exit(-1);
 }
 
@@ -152,18 +152,35 @@ int print_fdb(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
 
 static int fdb_show(int argc, char **argv)
 {
+	struct ndmsg ndm = { };
 	char *filter_dev = NULL;
+	char *br = NULL;
+
+	ndm.ndm_family = PF_BRIDGE;
+	ndm.ndm_state = NUD_NOARP;
 
 	while (argc > 0) {
-		if (strcmp(*argv, "dev") == 0) {
+		if ((strcmp(*argv, "port") == 0) || strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
-			if (filter_dev)
-				duparg("dev", *argv);
 			filter_dev = *argv;
+		} else if (strcmp(*argv, "br") == 0) {
+			NEXT_ARG();
+			br = *argv;
+		} else {
+			if (matches(*argv, "help") == 0)
+				usage();
 		}
 		argc--; argv++;
 	}
 
+	if (br) {
+		ndm.ndm_ifindex = ll_name_to_index(br);
+		if (ndm.ndm_ifindex == 0) {
+			fprintf(stderr, "Cannot find bridge device \"%s\"\n", br);
+			return -1;
+		}
+	}
+
 	if (filter_dev) {
 		filter_index = if_nametoindex(filter_dev);
 		if (filter_index == 0) {
@@ -171,13 +188,15 @@ static int fdb_show(int argc, char **argv)
 				filter_dev);
 			return -1;
 		}
+
 	}
 
-	if (rtnl_wilddump_request(&rth, PF_BRIDGE, RTM_GETNEIGH) < 0) {
+	if (rtnl_dump_request(&rth, RTM_GETNEIGH, &ndm, sizeof(struct ndmsg)) < 0) {
 		perror("Cannot send dump request");
 		exit(1);
 	}
 
+
 	if (rtnl_dump_filter(&rth, print_fdb, stdout) < 0) {
 		fprintf(stderr, "Dump terminated\n");
 		exit(1);

Re: RFC: bridge get fdb by bridge device

[John Fastabend]

On 02/09/2014 07:06 AM, Jamal Hadi Salim wrote:
>
> This patch allows something equivalent to
> "brctl showmacs <bridge device>" with iproute2
> syntax "bridge link 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).
>
> iproute2 allows filtering by bridge port, example:
> "bridge link br br1234 dev port1234"
> but the filtering is done in user space.
> In a future patch i would like to do the port filtering
> in the kernel. As well, adding a MAC filter in the kernel
> makes sense.
>
> Kernel patch is against net-next.
>
> cheers,
> jamal

[...]

> +	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->priv_flags & IFF_EBRIDGE)) {

Can we drop this 'if case' and just use the 'if (ops->ndo_fdb_dump)'
below? IFF_EBRIDGE is specific to ./net/bridge so it will fail for
macvlans and I think the command is useful in both cases.


> +			pr_info("PF_BRIDGE: RTM_GETNEIGH %s not a bridge device\n",
> +				dev->name);
> +			return -EINVAL;
>  		}
> +		ops = dev->netdev_ops;
> +		if (ops->ndo_fdb_dump) {
> +			idx = ops->ndo_fdb_dump(skb, cb, dev, idx);
> +		} else {

Is there any problem with using the ndo_dflt_fdb_dump() in the else
here?

Userspace should be able to easily learn which ports are ebridge ports
so I don't think that should be an issue. Anyways with the above
IFF_EBRIDGE check you should never hit this else case although I think
its safe to drop the above check as noted.

> +			pr_info("PF_BRIDGE: RTM_GETNEIGH %s no dumper\n",
> +				dev->name);
> +			return -EINVAL;
> +		}
> +	} else {

Thanks,
John

-- 
John Fastabend         Intel Corporation

Re: RFC: bridge get fdb by bridge device

[Jamal Hadi Salim]

On 02/09/14 14:33, John Fastabend wrote:
> On 02/09/2014 07:06 AM, Jamal Hadi Salim wrote:

>
>> +    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->priv_flags & IFF_EBRIDGE)) {
>
> Can we drop this 'if case' and just use the 'if (ops->ndo_fdb_dump)'
> below? IFF_EBRIDGE is specific to ./net/bridge so it will fail for
> macvlans and I think the command is useful in both cases.
>

My only worry is:
The target ndm_ifindex is to a bridge device i.e something
that has an "fdb" (user space says "br X" then we use X to
mean we are talking about a bridge device. This is what
brctl showmacs <X> means.
I know this doesnt seem right relative to current point of
view where the target is the bridge port ifindex.
I'd be more than happy to make the change if it makes sense,
but i am struggling with that thought.

>
>> +            pr_info("PF_BRIDGE: RTM_GETNEIGH %s not a bridge device\n",
>> +                dev->name);
>> +            return -EINVAL;
>>          }
>> +        ops = dev->netdev_ops;
>> +        if (ops->ndo_fdb_dump) {
>> +            idx = ops->ndo_fdb_dump(skb, cb, dev, idx);
>> +        } else {
>
> Is there any problem with using the ndo_dflt_fdb_dump() in the else
> here?
>

If the assumption is the target ifindex is to a bridge device, I would
expect it MUST have a ops->ndo_fdb_dump(), no?
the default dumper deals with bridge ports (which may be bridges, but
if i wanted their databases i would address them)

> Userspace should be able to easily learn which ports are ebridge ports
> so I don't think that should be an issue. Anyways with the above
> IFF_EBRIDGE check you should never hit this else case although I think
> its safe to drop the above check as noted.
>

Right - so if i can find out which is an ebridge i can send it the
same request.

cheers,
jamal

Re: RFC: bridge get fdb by bridge device

[Vlad Yasevich]

On 02/09/2014 10:06 AM, Jamal Hadi Salim wrote:
>
> This patch allows something equivalent to
> "brctl showmacs <bridge device>" with iproute2
> syntax "bridge link 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).
>
> iproute2 allows filtering by bridge port, example:
> "bridge link br br1234 dev port1234"
> but the filtering is done in user space.
> In a future patch i would like to do the port filtering
> in the kernel. As well, adding a MAC filter in the kernel
> makes sense.
>
> Kernel patch is against net-next.
>
> cheers,
> jamal
>
> bridge-fdb-filter1
>
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 393b1bc..507ea4e 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -2423,26 +2423,50 @@ 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) {

We get really lucky here that ndm_ifindex and ifi_index happen to map to
the same location.

> +		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->priv_flags & IFF_EBRIDGE)) {
> +			pr_info("PF_BRIDGE: RTM_GETNEIGH %s not a bridge device\n",
> +				dev->name);
> +			return -EINVAL;
>  		}
> +		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;
> +		}

I agree with both of Johns commens fro the above code.
I think you can use ndo_dflt_fdb_dump() here and remove the first check
for IFF_EBRIDGE.

The only odd thing is that it would permit syntax like:
 # bridge fbd show br eth0
or
 # bridge fdb show br macvlan0

but I think that's ok.

> +	} 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);
> +			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;
>
>
> iprt-fdb-brfilter1
>
>
> diff --git a/bridge/fdb.c b/bridge/fdb.c
> index e2e53f1..f3073d6 100644
> --- a/bridge/fdb.c
> +++ b/bridge/fdb.c
> @@ -33,7 +33,7 @@ static void usage(void)
>  	fprintf(stderr, "Usage: bridge fdb { add | append | del | replace }
ADDR dev DEV {self|master} [ temp ]\n"
>  		        "              [router] [ dst IPADDR] [ vlan VID ]\n"
>  		        "              [ port PORT] [ vni VNI ] [via DEV]\n");
> -	fprintf(stderr, "       bridge fdb {show} [ dev DEV ]\n");
> +	fprintf(stderr, "       bridge fdb {show} [ br BRDEV ] [ dev DEV ]\n");

'port' option is now allowed in the show operation

-vlad

>  	exit(-1);
>  }
>
> @@ -152,18 +152,35 @@ int print_fdb(const struct sockaddr_nl *who,
struct nlmsghdr *n, void *arg)
>
>  static int fdb_show(int argc, char **argv)
>  {
> +	struct ndmsg ndm = { };
>  	char *filter_dev = NULL;
> +	char *br = NULL;
> +
> +	ndm.ndm_family = PF_BRIDGE;
> +	ndm.ndm_state = NUD_NOARP;
>
>  	while (argc > 0) {
> -		if (strcmp(*argv, "dev") == 0) {
> +		if ((strcmp(*argv, "port") == 0) || strcmp(*argv, "dev") == 0) {
>  			NEXT_ARG();
> -			if (filter_dev)
> -				duparg("dev", *argv);
>  			filter_dev = *argv;
> +		} else if (strcmp(*argv, "br") == 0) {
> +			NEXT_ARG();
> +			br = *argv;
> +		} else {
> +			if (matches(*argv, "help") == 0)
> +				usage();
>  		}
>  		argc--; argv++;
>  	}
>
> +	if (br) {
> +		ndm.ndm_ifindex = ll_name_to_index(br);
> +		if (ndm.ndm_ifindex == 0) {
> +			fprintf(stderr, "Cannot find bridge device \"%s\"\n", br);
> +			return -1;
> +		}
> +	}
> +
>  	if (filter_dev) {
>  		filter_index = if_nametoindex(filter_dev);
>  		if (filter_index == 0) {
> @@ -171,13 +188,15 @@ static int fdb_show(int argc, char **argv)
>  				filter_dev);
>  			return -1;
>  		}
> +
>  	}
>
> -	if (rtnl_wilddump_request(&rth, PF_BRIDGE, RTM_GETNEIGH) < 0) {
> +	if (rtnl_dump_request(&rth, RTM_GETNEIGH, &ndm, sizeof(struct
ndmsg)) < 0) {
>  		perror("Cannot send dump request");
>  		exit(1);
>  	}
>
> +
>  	if (rtnl_dump_filter(&rth, print_fdb, stdout) < 0) {
>  		fprintf(stderr, "Dump terminated\n");
>  		exit(1);
>

Re: RFC: bridge get fdb by bridge device

[Jamal Hadi Salim]

On 02/10/14 11:31, Vlad Yasevich wrote:
> On 02/09/2014 10:06 AM, Jamal Hadi Salim wrote:


>> +	ndm = nlmsg_data(cb->nlh);
>> +	if (ndm->ndm_ifindex) {
>
> We get really lucky here that ndm_ifindex and ifi_index happen to map to
> the same location.
>

Didnt follow - but I have a feeling you are looking at the reference
point of a bridge port.
Note as per my response to John: The target is a bridge device, not
a bridge port.



>
> I agree with both of Johns commens fro the above code.
> I think you can use ndo_dflt_fdb_dump() here and remove the first check
> for IFF_EBRIDGE.
>

Same comment i made to John. The goal is to emulate
brctl showmacs <bridge>
ndo_dflt_fdb_dump() gives me in theory all the bridge ports
unicast and multicast MAC addresses. There is a posibility that
the bridgeport is a bridge - in which case I can find out from
user space and safely request for it directly instead of via
its parent.

> The only odd thing is that it would permit syntax like:
>   # bridge fbd show br eth0
> or
>   # bridge fdb show br macvlan0
>
> but I think that's ok.

Ok, since both you and John point to macvlan - is that
considered as something with an fdb? It doesnt forward
packets between two devices.



>> diff --git a/bridge/fdb.c b/bridge/fdb.c
>> index e2e53f1..f3073d6 100644
>> --- a/bridge/fdb.c
>> +++ b/bridge/fdb.c
>> @@ -33,7 +33,7 @@ static void usage(void)
>>   	fprintf(stderr, "Usage: bridge fdb { add | append | del | replace }
> ADDR dev DEV {self|master} [ temp ]\n"
>>   		        "              [router] [ dst IPADDR] [ vlan VID ]\n"
>>   		        "              [ port PORT] [ vni VNI ] [via DEV]\n");
>> -	fprintf(stderr, "       bridge fdb {show} [ dev DEV ]\n");
>> +	fprintf(stderr, "       bridge fdb {show} [ br BRDEV ] [ dev DEV ]\n");
>
> 'port' option is now allowed in the show operation
>

Thanks - it is already taken seems by vxlan using the same interface.


cheers,
jamal

Re: RFC: bridge get fdb by bridge device

[Vlad Yasevich]

On 02/11/2014 12:07 PM, Jamal Hadi Salim wrote:
> On 02/10/14 11:31, Vlad Yasevich wrote:
>> On 02/09/2014 10:06 AM, Jamal Hadi Salim wrote:
> 
> 
>>> +    ndm = nlmsg_data(cb->nlh);
>>> +    if (ndm->ndm_ifindex) {
>>
>> We get really lucky here that ndm_ifindex and ifi_index happen to map to
>> the same location.
>>
> 
> Didnt follow - but I have a feeling you are looking at the reference
> point of a bridge port.
> Note as per my response to John: The target is a bridge device, not
> a bridge port.
> 

No, this was more the point that the current iproute code sends an
ifinfomsg struct down, and you change that to send ndmsg struct.
This is risky, but we luck out since the index is at the same offset
in both structs.

> 
> 
>>
>> I agree with both of Johns commens fro the above code.
>> I think you can use ndo_dflt_fdb_dump() here and remove the first check
>> for IFF_EBRIDGE.
>>
> 
> Same comment i made to John. The goal is to emulate
> brctl showmacs <bridge>
> ndo_dflt_fdb_dump() gives me in theory all the bridge ports
> unicast and multicast MAC addresses. There is a posibility that
> the bridgeport is a bridge - in which case I can find out from
> user space and safely request for it directly instead of via
> its parent.
> 

But that would only happen if the user said:
  # bridge fdb show br eth0

If eth0 in this case is a hw bridge device, getting the device's
version of fdb data is exactly what would be expected, isn't it?

If you mean a 'software bridge' above, then that's not an issue
since that's a disallowed config.  You can't stack software bridges
without something in the middle like bond or vlan.

>> The only odd thing is that it would permit syntax like:
>>   # bridge fbd show br eth0
>> or
>>   # bridge fdb show br macvlan0
>>
>> but I think that's ok.
> 
> Ok, since both you and John point to macvlan - is that
> considered as something with an fdb? It doesnt forward
> packets between two devices.
> 

Yes, macvlan can forward data to other macvlans, but that's
not the interesting thing.
When you configure multiple macvlan devices on top of the
same hw device, one could think of the hw device as a sort
of a bridge.  It's not really, but you could define it in
those terms.  The fdb entries, in this case, contain the mac
addresses of the macvlan devices.

> 
> 
>>> diff --git a/bridge/fdb.c b/bridge/fdb.c
>>> index e2e53f1..f3073d6 100644
>>> --- a/bridge/fdb.c
>>> +++ b/bridge/fdb.c
>>> @@ -33,7 +33,7 @@ static void usage(void)
>>>       fprintf(stderr, "Usage: bridge fdb { add | append | del |
>>> replace }
>> ADDR dev DEV {self|master} [ temp ]\n"
>>>                   "              [router] [ dst IPADDR] [ vlan VID ]\n"
>>>                   "              [ port PORT] [ vni VNI ] [via DEV]\n");
>>> -    fprintf(stderr, "       bridge fdb {show} [ dev DEV ]\n");
>>> +    fprintf(stderr, "       bridge fdb {show} [ br BRDEV ] [ dev DEV
>>> ]\n");
>>
>> 'port' option is now allowed in the show operation
>>
> 
> Thanks - it is already taken seems by vxlan using the same interface.
> 

Sorry, I wasn't very clear. What I meant was that you now support
  # bridge fdb show port <>

The usage message should reflect it.

-vlad
> 
> cheers,
> jamal
> 

Re: RFC: bridge get fdb by bridge device

[Jamal Hadi Salim]

On 02/11/14 13:21, Vlad Yasevich wrote:
> On 02/11/2014 12:07 PM, Jamal Hadi Salim wrote:
>> On 02/10/14 11:31, Vlad Yasevich wrote:

> No, this was more the point that the current iproute code sends an
> ifinfomsg struct down, and you change that to send ndmsg struct.
> This is risky, but we luck out since the index is at the same offset
> in both structs.
>

ah, ok, thanks for catching that. I should have said something - the
original code was wrong and i felt it was safe to make the change
given that the kernel code never even looked at what was being
sent to it. There is asymetry desires which are violated.
It doesnt make sense to send and ifm and expect back an ndm.
I should send that separately as a bug fix.


> But that would only happen if the user said:
>    # bridge fdb show br eth0
>
> If eth0 in this case is a hw bridge device, getting the device's
> version of fdb data is exactly what would be expected, isn't it?
>

Well, if it is a "bridge device" why would it not be tagged as a bridge
device?

> If you mean a 'software bridge' above, then that's not an issue
> since that's a disallowed config.  You can't stack software bridges
> without something in the middle like bond or vlan.
>

Ok, didnt realize that.
So i cant add a bridge as a bridge port to another bridge?

>
> Yes, macvlan can forward data to other macvlans, but that's
> not the interesting thing.

Sample config?

> When you configure multiple macvlan devices on top of the
> same hw device, one could think of the hw device as a sort
> of a bridge.  It's not really, but you could define it in
> those terms.  The fdb entries, in this case, contain the mac
> addresses of the macvlan devices.
>

It certainly has some equivalent semantics (looks at dst MAC then
picks the port). Possible to add Vlans as well?
Why dont we tag such a thing as a bridge then?

>
> Sorry, I wasn't very clear. What I meant was that you now support
>    # bridge fdb show port <>
>
> The usage message should reflect it.
>

Sorry - I noticed the word "port" at exactly where your quote came.
So i thought you noticed that "port" was already taken - it is used
for VXLAN fdb entries (for udp ports).


cheers,
jamal

Re: RFC: bridge get fdb by bridge device

[John Fastabend]

On 2/11/2014 12:15 PM, Jamal Hadi Salim wrote:
>
>>
>> Yes, macvlan can forward data to other macvlans, but that's
>> not the interesting thing.
>
> Sample config?

ip link add link ethx name mv1 type macvlan mode bridge
ip link add link ethx name mv2 type macvlan mode bridge

Now you have a macvlan on ethx that will forward data between
mv1 and mv2.

Re: RFC: bridge get fdb by bridge device

[John Fastabend]

On 2/11/2014 12:15 PM, Jamal Hadi Salim wrote:
> On 02/11/14 13:21, Vlad Yasevich wrote:
>> On 02/11/2014 12:07 PM, Jamal Hadi Salim wrote:
>>> On 02/10/14 11:31, Vlad Yasevich wrote:
>
>> No, this was more the point that the current iproute code sends an
>> ifinfomsg struct down, and you change that to send ndmsg struct.
>> This is risky, but we luck out since the index is at the same offset
>> in both structs.
>>
>
> ah, ok, thanks for catching that. I should have said something - the
> original code was wrong and i felt it was safe to make the change
> given that the kernel code never even looked at what was being
> sent to it. There is asymetry desires which are violated.
> It doesnt make sense to send and ifm and expect back an ndm.
> I should send that separately as a bug fix.
>
>
>> But that would only happen if the user said:
>>    # bridge fdb show br eth0
>>
>> If eth0 in this case is a hw bridge device, getting the device's
>> version of fdb data is exactly what would be expected, isn't it?
>>
>
> Well, if it is a "bridge device" why would it not be tagged as a bridge
> device?

What do you mean by "bridge device" are you specifically talking about
IFF_BRIDGE flag? This flag is used only for ./net/bridge devices. For
example macvlan uses its own flag. I think there is a good case to be
made for netdevices which are acting as the management interface for a
hardware bridge to set an identifying flag. Perhaps IFF_HWBRIDGE.

>
>> If you mean a 'software bridge' above, then that's not an issue
>> since that's a disallowed config.  You can't stack software bridges
>> without something in the middle like bond or vlan.
>>
>
> Ok, didnt realize that.
> So i cant add a bridge as a bridge port to another bridge?
>

# ip link set dev bridge0 master bridge1
RTNETLINK answers: Too many levels of symbolic links

in the bridge case this doesn't work. But you can stack a macvlan
on top of the bridge port,

# ip link add link bridge0 type macvlan mode vepa

11: macvlan0@bridge0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop 
state DOWN mode DEFAULT group default

And macvlans on macvlans is OK as well.

# ip link add link macvlan0 type macvlan mode vepa

[...]

>> When you configure multiple macvlan devices on top of the
>> same hw device, one could think of the hw device as a sort
>> of a bridge.  It's not really, but you could define it in
>> those terms.  The fdb entries, in this case, contain the mac
>> addresses of the macvlan devices.
>>
>
> It certainly has some equivalent semantics (looks at dst MAC then
> picks the port). Possible to add Vlans as well?
> Why dont we tag such a thing as a bridge then?
>

If its useful then we should. You can track them down in userspace
via /sys/class/net/ or looking for offloaded netdevices that point
to the interface but a flag is definitely more direct.

>>
>> Sorry, I wasn't very clear. What I meant was that you now support
>>    # bridge fdb show port <>
>>
>> The usage message should reflect it.
>>
>
> Sorry - I noticed the word "port" at exactly where your quote came.
> So i thought you noticed that "port" was already taken - it is used
> for VXLAN fdb entries (for udp ports).
>
>
> cheers,
> jamal

Re: RFC: bridge get fdb by bridge device

[Jamal Hadi Salim]

On 02/11/14 15:30, John Fastabend wrote:
> On 2/11/2014 12:15 PM, Jamal Hadi Salim wrote:

Thanks for the example on the other email.


> What do you mean by "bridge device" are you specifically talking about
> IFF_BRIDGE flag? This flag is used only for ./net/bridge devices.

Right - the simple definition is this thing has an fdb.
Yes, I know weve added vlan filtering and multicast snooping
but thats all lipstick. If it has an (ethernet) fdb it is a bridge.

>For
> example macvlan uses its own flag. I think there is a good case to be
> made for netdevices which are acting as the management interface for a
> hardware bridge to set an identifying flag. Perhaps IFF_HWBRIDGE.
>

If you introduce IFF_HWBRIDGE - I think that would satisfy the
distinction. The question then is why not just tag it IFF_BRIDGE?

>
> # ip link set dev bridge0 master bridge1
> RTNETLINK answers: Too many levels of symbolic links
>

pourquoi?  If the original rationale was to limit the
broadcast domain scope it sounds strange that a bridge in
the form a macvlan is allowed.

> in the bridge case this doesn't work. But you can stack a macvlan
> on top of the bridge port,
>
> # ip link add link bridge0 type macvlan mode vepa
>
> 11: macvlan0@bridge0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop
> state DOWN mode DEFAULT group default
>
> And macvlans on macvlans is OK as well.
>
> # ip link add link macvlan0 type macvlan mode vepa
>
> [...]
>

Ok, I need to let that sink in. Cool actually.


>
> If its useful then we should. You can track them down in userspace
> via /sys/class/net/ or looking for offloaded netdevices that point
> to the interface but a flag is definitely more direct.
>

I prefer a flag. Then i can deal with it via netlink.

cheers,
jamal

Re: RFC: bridge get fdb by bridge device

[John Fastabend]

On 2/11/2014 1:04 PM, Jamal Hadi Salim wrote:
> On 02/11/14 15:30, John Fastabend wrote:
>> On 2/11/2014 12:15 PM, Jamal Hadi Salim wrote:
>
> Thanks for the example on the other email.
>

Just to wrap things up in one email. Changing between VEB and VEPA
modes already triggers an event. So management applications can listen
for this.

And I can send out a patch to add a flag to hardware bridge devices
I'll likely get to it next week sometime unless someone beats me
to it.

>
>> What do you mean by "bridge device" are you specifically talking about
>> IFF_BRIDGE flag? This flag is used only for ./net/bridge devices.
>
> Right - the simple definition is this thing has an fdb.
> Yes, I know weve added vlan filtering and multicast snooping
> but thats all lipstick. If it has an (ethernet) fdb it is a bridge.
>

Sure, IEEE802.1Q would call these edge relays.

>> For
>> example macvlan uses its own flag. I think there is a good case to be
>> made for netdevices which are acting as the management interface for a
>> hardware bridge to set an identifying flag. Perhaps IFF_HWBRIDGE.
>>
>
> If you introduce IFF_HWBRIDGE - I think that would satisfy the
> distinction. The question then is why not just tag it IFF_BRIDGE?
>

Because it is not the same type of object as the software bridge.
Most notably it doesn't do learning. If anything its more like a
macvlan device and we could just as easily tag it IFF_MACVLAN. So
because it doesn't really match 1:1 with either of those object I
would just presume give its own flag. Userspace can always create
a small macro call it is_bridge_like() and check for any of the
handful of bridge like objects.

>>
>> # ip link set dev bridge0 master bridge1
>> RTNETLINK answers: Too many levels of symbolic links
>>
>
> pourquoi?  If the original rationale was to limit the
> broadcast domain scope it sounds strange that a bridge in
> the form a macvlan is allowed.
>

Agreed. But there it is.

>> in the bridge case this doesn't work. But you can stack a macvlan
>> on top of the bridge port,
>>
>> # ip link add link bridge0 type macvlan mode vepa
>>
>> 11: macvlan0@bridge0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop
>> state DOWN mode DEFAULT group default
>>
>> And macvlans on macvlans is OK as well.
>>
>> # ip link add link macvlan0 type macvlan mode vepa
>>
>> [...]
>>
>
> Ok, I need to let that sink in. Cool actually.
>

Also note you can mix VEB and VEPA modes and if you do it correctly
can create blocks of virtual ports that can do east-west traffic and
isolate others.

>
>>
>> If its useful then we should. You can track them down in userspace
>> via /sys/class/net/ or looking for offloaded netdevices that point
>> to the interface but a flag is definitely more direct.
>>
>
> I prefer a flag. Then i can deal with it via netlink.
>

OK. I'll add one here shortly.

> cheers,
> jamal
>

Re: RFC: bridge get fdb by bridge device

[Jamal Hadi Salim]

On 02/12/14 13:50, John Fastabend wrote:

>
> Just to wrap things up in one email. Changing between VEB and VEPA
> modes already triggers an event. So management applications can listen
> for this.
>


Ok - reasonable.

> And I can send out a patch to add a flag to hardware bridge devices
> I'll likely get to it next week sometime unless someone beats me
> to it.
>

You understand this better - so i will wait.
I'll send an updated version of the patch this weekend
now that net-next is open.

>
> Sure, IEEE802.1Q would call these edge relays.
>

Ok, so what older kids used to call "repeaters".
The more i think about it, the more it looks like this is
still a bridge and we have a bridgeport mode as VEPA vs VEB.
IOW, as you said - you can have a bridge with mix and match of
VEB/VEPA. We can easily add such a feature to the software bridge
as well. It sounds simple and useful enough.

>
> Because it is not the same type of object as the software bridge.
> Most notably it doesn't do learning. If anything its more like a
> macvlan device and we could just as easily tag it IFF_MACVLAN. So
> because it doesn't really match 1:1 with either of those object I
> would just presume give its own flag. Userspace can always create
> a small macro call it is_bridge_like() and check for any of the
> handful of bridge like objects.
>

I think VEB/VEP may be somehow covering the "port" aspect.
The challenge is what to call "eth0 when running in SRIOV"

>>>
>>> # ip link set dev bridge0 master bridge1
>>> RTNETLINK answers: Too many levels of symbolic links
>>>
>>
>> pourquoi?  If the original rationale was to limit the
>> broadcast domain scope it sounds strange that a bridge in
>> the form a macvlan is allowed.
>>
>
> Agreed. But there it is.
>

I am sure someone knows why - Stephen?

cheers,
jamal

Re: RFC: bridge get fdb by bridge device

[Jamal Hadi Salim]

On 02/12/14 13:50, John Fastabend wrote:
> On 2/11/2014 1:04 PM, Jamal Hadi Salim wrote:
>
> Because it is not the same type of object as the software bridge.
> Most notably it doesn't do learning.

This kept nagging at me.
Learning is optional for a bridge. So is flooding.
I think we got this right in recent kernels.

cheers,
jamal

Re: RFC: bridge get fdb by bridge device

[John Fastabend]

On 2/13/2014 7:37 AM, Jamal Hadi Salim wrote:
> On 02/12/14 13:50, John Fastabend wrote:
>> On 2/11/2014 1:04 PM, Jamal Hadi Salim wrote:
>>
>> Because it is not the same type of object as the software bridge.
>> Most notably it doesn't do learning.
>
> This kept nagging at me.
> Learning is optional for a bridge. So is flooding.
> I think we got this right in recent kernels.
>
> cheers,
> jamal

Yeah I remember now Vlad added them. The real distinction
is macvlan devices only have _one_ uplink port and support
other forwarding modes, VEPA, etc.

Re: RFC: bridge get fdb by bridge device

[Vlad Yasevich]

On 02/11/2014 03:15 PM, Jamal Hadi Salim wrote:
> On 02/11/14 13:21, Vlad Yasevich wrote:
>> On 02/11/2014 12:07 PM, Jamal Hadi Salim wrote:
>>> On 02/10/14 11:31, Vlad Yasevich wrote:
> 
>> No, this was more the point that the current iproute code sends an
>> ifinfomsg struct down, and you change that to send ndmsg struct.
>> This is risky, but we luck out since the index is at the same offset
>> in both structs.
>>
> 
> ah, ok, thanks for catching that. I should have said something - the
> original code was wrong and i felt it was safe to make the change
> given that the kernel code never even looked at what was being
> sent to it. There is asymetry desires which are violated.
> It doesnt make sense to send and ifm and expect back an ndm.
> I should send that separately as a bug fix.
> 
> 
>> But that would only happen if the user said:
>>    # bridge fdb show br eth0
>>
>> If eth0 in this case is a hw bridge device, getting the device's
>> version of fdb data is exactly what would be expected, isn't it?
>>
> 
> Well, if it is a "bridge device" why would it not be tagged as a bridge
> device?

Because it just a multi-function nic that isn't tagged with any
kine of bridge flag.  As John said, this might be useful, but not
done yet.

> 
>> If you mean a 'software bridge' above, then that's not an issue
>> since that's a disallowed config.  You can't stack software bridges
>> without something in the middle like bond or vlan.
>>
> 
> Ok, didnt realize that.
> So i cant add a bridge as a bridge port to another bridge?

Not directly.  However, if you put a layered software device in between
(vlan, bond, macvlan), then you can add that device to another bridge.
In fact, people do that to get GVRP working with VMs.

> 
>>
>> Yes, macvlan can forward data to other macvlans, but that's
>> not the interesting thing.
> 
> Sample config?
> 
>> When you configure multiple macvlan devices on top of the
>> same hw device, one could think of the hw device as a sort
>> of a bridge.  It's not really, but you could define it in
>> those terms.  The fdb entries, in this case, contain the mac
>> addresses of the macvlan devices.
>>
> 
> It certainly has some equivalent semantics (looks at dst MAC then
> picks the port). Possible to add Vlans as well?

I suppose.   You can do things like:
# ip link add link eth0 dev vlan100 protocol 8021Q id 100
# ip link add link vlan0 dev mac100 type macvlan

Now, you have a macvlan (mac100) that will only receive vlan100 traffic.
Expressing this in terms of fdb would be a bit difficult since each
interface is separate and eth0 doesn't really know about the stack.
It would require quite a lot of code.

> Why dont we tag such a thing as a bridge then?
> 

Because they are not always a bridge.  It could be just a nic capable of
mac filtering.

>>
>> Sorry, I wasn't very clear. What I meant was that you now support
>>    # bridge fdb show port <>
>>
>> The usage message should reflect it.
>>
> 
> Sorry - I noticed the word "port" at exactly where your quote came.
> So i thought you noticed that "port" was already taken - it is used
> for VXLAN fdb entries (for udp ports).
>

Didn't realize it has different connotation for vxlan.  The you probably
don't want to include and support in the bridge fdb show command.

-vlad

> 
> cheers,
> jamal

Re: RFC: bridge get fdb by bridge device

[Jamal Hadi Salim]

On 02/11/14 16:00, Vlad Yasevich wrote:
> On 02/11/2014 03:15 PM, Jamal Hadi Salim wrote:

>
> Because it just a multi-function nic that isn't tagged with any
> kine of bridge flag.  As John said, this might be useful, but not
> done yet.
>

Ok, fair enough. Someone should send a patch - John perhaps.

>
> Not directly.  However, if you put a layered software device in between
> (vlan, bond, macvlan), then you can add that device to another bridge.
> In fact, people do that to get GVRP working with VMs.
>

Do you recall the reasoning behind it?


>> It certainly has some equivalent semantics (looks at dst MAC then
>> picks the port). Possible to add Vlans as well?
>
> I suppose.   You can do things like:
> # ip link add link eth0 dev vlan100 protocol 8021Q id 100
> # ip link add link vlan0 dev mac100 type macvlan
>
> Now, you have a macvlan (mac100) that will only receive vlan100 traffic.
> Expressing this in terms of fdb would be a bit difficult since each
> interface is separate and eth0 doesn't really know about the stack.
> It would require quite a lot of code.
>

nice.

>> Why dont we tag such a thing as a bridge then?
>>
>
> Because they are not always a bridge.  It could be just a nic capable of
> mac filtering.
>

I think in one of the modes it is merely a filter.
But you turn on this other feature it is a bridge.

>
> Didn't realize it has different connotation for vxlan.  The you probably
> don't want to include and support in the bridge fdb show command.

Thats what i thought you said earlier ;->

cheers,
jamal

Re: RFC: bridge get fdb by bridge device

[Jamal Hadi Salim]

On 02/11/14 16:08, Jamal Hadi Salim wrote:
> On 02/11/14 16:00, Vlad Yasevich wrote:
>> On 02/11/2014 03:15 PM, Jamal Hadi Salim wrote:
>

> I think in one of the modes it is merely a filter.
> But you turn on this other feature it is a bridge.

IOW, VEPA should turn off IFF_BRIDGE and VEB should
turn it on. We probably need an event generated.


cheers,
jamal

Re: RFC: bridge get fdb by bridge device

[Vlad Yasevich]

On 02/11/2014 04:08 PM, Jamal Hadi Salim wrote:
> On 02/11/14 16:00, Vlad Yasevich wrote:
>> On 02/11/2014 03:15 PM, Jamal Hadi Salim wrote:
> 
>>
>> Because it just a multi-function nic that isn't tagged with any
>> kine of bridge flag.  As John said, this might be useful, but not
>> done yet.
>>
> 
> Ok, fair enough. Someone should send a patch - John perhaps.
> 
>>
>> Not directly.  However, if you put a layered software device in between
>> (vlan, bond, macvlan), then you can add that device to another bridge.
>> In fact, people do that to get GVRP working with VMs.
>>
> 
> Do you recall the reasoning behind it?

Before my time. It's there since before 2.6.12 :)

-vlad

> 
> 
>>> It certainly has some equivalent semantics (looks at dst MAC then
>>> picks the port). Possible to add Vlans as well?
>>
>> I suppose.   You can do things like:
>> # ip link add link eth0 dev vlan100 protocol 8021Q id 100
>> # ip link add link vlan0 dev mac100 type macvlan
>>
>> Now, you have a macvlan (mac100) that will only receive vlan100 traffic.
>> Expressing this in terms of fdb would be a bit difficult since each
>> interface is separate and eth0 doesn't really know about the stack.
>> It would require quite a lot of code.
>>
> 
> nice.
> 
>>> Why dont we tag such a thing as a bridge then?
>>>
>>
>> Because they are not always a bridge.  It could be just a nic capable of
>> mac filtering.
>>
> 
> I think in one of the modes it is merely a filter.
> But you turn on this other feature it is a bridge.
> 
>>
>> Didn't realize it has different connotation for vxlan.  The you probably
>> don't want to include and support in the bridge fdb show command.
> 
> Thats what i thought you said earlier ;->
> 
> cheers,
> jamal