Re: RFC: bridge get fdb by bridge device

[John Fastabend <john.fastabend@gmail.com>]

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