[PATCH 3/3] bridge: Add support for VLANs

Tobias Jungel tobias.jungel at bisdn.de
Wed Nov 25 07:47:29 PST 2015


some more comments inline here as well.
/tobi


On Di, 2015-11-24 at 11:56 -0800, David Ahern wrote:
> Signed-off-by: Wilson Kok <wkok at cumulusnetworks.com>
> Signed-off-by: David Ahern <dsa at cumulusnetworks.com>
> ---
>  include/linux-private/linux/if_bridge.h |  14 +++
>  include/linux-private/linux/rtnetlink.h |   6 +
>  include/netlink/route/link/bridge.h     |  21 ++++
>  lib/route/link/bridge.c                 | 203
> ++++++++++++++++++++++++++++++++
>  lib/route/neigh.c                       |   9 +-
>  5 files changed, 251 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux-private/linux/if_bridge.h b/include/linux-
> private/linux/if_bridge.h
> index 5db297514aec..810d3489c090 100644
> --- a/include/linux-private/linux/if_bridge.h
> +++ b/include/linux-private/linux/if_bridge.h
> @@ -108,15 +108,29 @@ struct __fdb_entry {
>   * [IFLA_AF_SPEC] = {
>   *     [IFLA_BRIDGE_FLAGS]
>   *     [IFLA_BRIDGE_MODE]
> + *     [IFLA_BRIDGE_VLAN_INFO]
>   * }
>   */
>  enum {
>  	IFLA_BRIDGE_FLAGS,
>  	IFLA_BRIDGE_MODE,
> +	IFLA_BRIDGE_VLAN_INFO,
>  	__IFLA_BRIDGE_MAX,
>  };
>  #define IFLA_BRIDGE_MAX (__IFLA_BRIDGE_MAX - 1)
>  
> +#define BRIDGE_VLAN_INFO_MASTER	(1<<0)	/* Operate on
> Bridge device as well */
> +#define BRIDGE_VLAN_INFO_PVID	(1<<1)	/* VLAN is PVID,
> ingress untagged */
> +#define BRIDGE_VLAN_INFO_UNTAGGED	(1<<2)	/* VLAN
> egresses untagged */
> +#define BRIDGE_VLAN_INFO_RANGE_BEGIN	(1<<3) /* VLAN is start
> of vlan range */
> +#define BRIDGE_VLAN_INFO_RANGE_END	(1<<4) /* VLAN is end of
> vlan range */
> +#define BRIDGE_VLAN_INFO_BRENTRY	(1<<5) /* Global bridge VLAN
> entry */
> +
> +struct bridge_vlan_info {
> +	__u16 flags;
> +	__u16 vid;
> +};
> +
>  /* Bridge multicast database attributes
>   * [MDBA_MDB] = {
>   *     [MDBA_MDB_ENTRY] = {
> diff --git a/include/linux-private/linux/rtnetlink.h b/include/linux-
> private/linux/rtnetlink.h
> index 2363c18ed62c..f3393d634343 100644
> --- a/include/linux-private/linux/rtnetlink.h
> +++ b/include/linux-private/linux/rtnetlink.h
> @@ -600,6 +600,12 @@ struct tcamsg {
>  #define TCA_ACT_TAB 1 /* attr type must be >=1 */	
>  #define TCAA_MAX 1
>  
> +/* New extended info filters for IFLA_EXT_MASK */
> +#define RTEXT_FILTER_VF		(1 << 0)
> +#define RTEXT_FILTER_BRVLAN	(1 << 1)
> +#define RTEXT_FILTER_BRVLAN_COMPRESSED	(1 << 2)
> +#define	RTEXT_FILTER_SKIP_STATS	(1 << 3)
> +
>  /* End of information exported to user level */
>  
>  #endif	/* __LINUX_RTNETLINK_H */
> diff --git a/include/netlink/route/link/bridge.h
> b/include/netlink/route/link/bridge.h
> index 16a4505f8b74..5dc6a367595c 100644
> --- a/include/netlink/route/link/bridge.h
> +++ b/include/netlink/route/link/bridge.h
> @@ -19,6 +19,22 @@
>  extern "C" {
>  #endif
>  
> +/* maximum vlan id */
> +#define VLAN_N_VID		4096
> +
> +#define BITS_PER_BYTE		8
> +#define BITS_PER_LONG		(BITS_PER_BYTE *
> sizeof(unsigned long))
> +#define DIV_ROUND_UP(n,d)	(((n) + (d) - 1) / (d))
> +#define BITS_TO_LONGS(nr)	DIV_ROUND_UP(nr, BITS_PER_LONG)
> +#define BR_VLAN_BITMAP_LEN	BITS_TO_LONGS(VLAN_N_VID)
> +
> +struct bridge_vlan
> +{
> +	uint16_t                pvid;
> +	unsigned long           vlan_bitmap[BR_VLAN_BITMAP_LEN];
> +	unsigned long           untagged_bitmap[BR_VLAN_BITMAP_LEN];
> +};
> +

Fine with me, even if I personnaly would rather opt for the kernels
compressed data structure, but this is probably just a matter of taste.

>  /**
>   * Bridge flags
>   * @ingroup bridge
> @@ -52,6 +68,11 @@ extern char * rtnl_link_bridge_flags2str(int, char
> *, size_t);
>  extern int	rtnl_link_bridge_str2flags(const char *);
>  
>  extern int	rtnl_link_bridge_add(struct nl_sock *sk, const
> char *name);
> +
> +extern int	rtnl_link_bridge_pvid(struct rtnl_link *link);
> +extern int	rtnl_link_bridge_has_vlan(struct rtnl_link *link);
> +
> +extern struct bridge_vlan *rtnl_bridge_get_port_vlan(struct
> rtnl_link *link);
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/route/link/bridge.c b/lib/route/link/bridge.c
> index 544f02cd379b..d965a40469a5 100644
> --- a/lib/route/link/bridge.c
> +++ b/lib/route/link/bridge.c
> @@ -25,11 +25,14 @@
>  #include <netlink-private/route/link/api.h>
>  #include <linux/if_bridge.h>
>  
> +#define VLAN_VID_MASK           0x0fff /* VLAN Identifier */
> +
>  /** @cond SKIP */
>  #define BRIDGE_ATTR_PORT_STATE		(1 << 0)
>  #define BRIDGE_ATTR_PRIORITY		(1 << 1)
>  #define BRIDGE_ATTR_COST		(1 << 2)
>  #define BRIDGE_ATTR_FLAGS		(1 << 3)
> +#define BRIDGE_ATTR_PORT_VLAN          (1 << 4)
>  
>  #define PRIV_FLAG_NEW_ATTRS		(1 << 0)
>  
> @@ -42,8 +45,49 @@ struct bridge_data
>  	uint32_t		b_flags;
>  	uint32_t		b_flags_mask;
>  	uint32_t                ce_mask; /* HACK to support attr
> macros */
> +	struct bridge_vlan	vlan_info;
>  };
>  
> +static void set_bit(unsigned nr, unsigned long *addr)
> +{
> +	unsigned long mask, *p;
> +
> +	if (nr < VLAN_N_VID) {
> +		mask = (1UL << (nr % BITS_PER_LONG));
> +		p = addr + nr / BITS_PER_LONG;
> +		*p |= mask;
> +	}
> +}
> +
> +#if 0
> +static void clear_bit(unsigned nr, unsigned long *addr)
> +{
> +	unsigned long mask, *p;
> +
> +	if (nr < VLAN_N_VID) {
> +		mask = (1UL << (nr % BITS_PER_LONG));
> +		p = addr + nr / BITS_PER_LONG;
> +		*p &= ~mask;
> +	}
> +}
> +#endif
> +
> +static int find_next_bit(int i, unsigned long x)
> +{
> +	int j;
> +
> +	if (i >= (int) sizeof(x) * 8)
> +		return -1;
> +
> +	/* find first bit */
> +	if (i < 0)
> +		return __builtin_ffsl(x);
> +
> +	/* mask off prior finds to get next */
> +	j = __builtin_ffsl(x >> i);
> +	return j ? j += i : 0;
> +}
> +
>  static struct rtnl_link_af_ops bridge_ops;
>  
>  #define IS_BRIDGE_LINK_ASSERT(link) \
> @@ -141,6 +185,112 @@ static int bridge_parse_protinfo(struct
> rtnl_link *link, struct nlattr *attr,
>  	return 0;
>  }
>  
> +static int bridge_parse_af(struct rtnl_link *link, struct nlattr
> *attr,
> +			   void *data)
> +{
> +	struct bridge_data *bd = data;
> +	struct bridge_vlan_info *vinfo = NULL;
> +
> +	if (nla_type(attr) != IFLA_BRIDGE_VLAN_INFO)
> +		return 0;
> +
> +	if (nla_len(attr) != sizeof(struct bridge_vlan_info))
> +		return -EINVAL;
> +
> +	vinfo = nla_data(attr);
> +	if (!vinfo->vid || vinfo->vid >= VLAN_VID_MASK)
> +		return -EINVAL;
> +
> +	if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN) {
> +		NL_DBG(1, "Unexpected BRIDGE_VLAN_INFO_RANGE_BEGIN
> flag; can not handle it.\n");
> +		return -EINVAL;

afaik RTEXT_FILTER_BRVLAN_COMPRESSED is the default for the kernel to
send notifications, so I see this rather mandatory to have.

> +	}
> +
> +	if (vinfo->flags & BRIDGE_VLAN_INFO_PVID)
> +		bd->vlan_info.pvid = vinfo->vid;
> +
> +	if (vinfo->flags & BRIDGE_VLAN_INFO_UNTAGGED)
> +		set_bit(vinfo->vid, bd->vlan_info.untagged_bitmap);
> +
> +	set_bit(vinfo->vid, bd->vlan_info.vlan_bitmap);
> +
> +	bd->ce_mask |= BRIDGE_ATTR_PORT_VLAN;
> +
> +	return 0;
> +}
> +
> +static int bridge_get_af(struct nl_msg *msg)
> +{
> +	__u32 ext_filter_mask = RTEXT_FILTER_BRVLAN;
> +
> +	return nla_put(msg, IFLA_EXT_MASK, sizeof(ext_filter_mask),
> &ext_filter_mask);
> +}
> +
> +static void walk_bitmap(struct nl_dump_params *p, unsigned long *b,
> int nr)
> +{
> +	int i = -1, j, k;
> +	int start = -1, prev = -1;
> +	int done, found = 0;
> +
> +	for (k = 0; k < nr; k++) {
> +		int base_bit;
> +		unsigned long a = b[k];
> +
> +		base_bit = k * BITS_PER_LONG;
> +		i = -1;
> +		done = 0;
> +		while (!done) {
> +			j = find_next_bit(i, a);
> +			if (j > 0) {
> +				/* first hit of any bit */
> +				if (start < 0 && prev < 0) {
> +					start = prev = j - 1 +
> base_bit;
> +					goto next;
> +				}
> +				/* this bit is a continuation of
> prior bits */
> +				if (j - 2 + base_bit == prev) {
> +					prev++;
> +					goto next;
> +				}
> +			} else
> +				done = 1;
> +
> +			if (start >= 0) {
> +				found++;
> +				if (done && k < nr - 1)
> +					break;
> +
> +				nl_dump(p, " %d", start);
> +				if (start != prev)
> +					nl_dump(p, "-%d", prev);
> +
> +				if (done)
> +					break;
> +			}
> +			if (j > 0)
> +				start = prev = j - 1 + base_bit;
> +next:
> +			i = j;
> +		}
> +	}
> +	if (!found)
> +		nl_dump(p, " <none>");
> +
> +	return;
> +}
> +
> +static void rtnl_link_bridge_dump_vlans(struct nl_dump_params *p,
> +					struct bridge_data *bd)
> +{
> +	nl_dump(p, "pvid %u", bd->vlan_info.pvid);
> +
> +	nl_dump(p, "   all vlans:");
> +	walk_bitmap(p, &bd->vlan_info.vlan_bitmap[0],
> BR_VLAN_BITMAP_LEN);
> +
> +	nl_dump(p, "   untagged vlans:");
> +	walk_bitmap(p, &bd->vlan_info.untagged_bitmap[0],
> BR_VLAN_BITMAP_LEN);
> +}
> +
>  static void bridge_dump_details(struct rtnl_link *link,
>  				struct nl_dump_params *p, void
> *data)
>  {
> @@ -157,6 +307,17 @@ static void bridge_dump_details(struct rtnl_link
> *link,
>  	if (bd->ce_mask & BRIDGE_ATTR_COST)
>  		nl_dump(p, "cost %u ", bd->b_cost);
>  
> +	if (bd->ce_mask & BRIDGE_ATTR_PORT_VLAN)
> +		rtnl_link_bridge_dump_vlans(p, bd);
> +
> +	if (bd->ce_mask & BRIDGE_ATTR_FLAGS) {
> +		char buf[256];
> +
> +		rtnl_link_bridge_flags2str(bd->b_flags & bd-
> >b_flags_mask,
> +					   buf, sizeof(buf));
> +		nl_dump(p, "%s", buf);
> +	}
> +
>  	nl_dump(p, "\n");
>  }
>  
> @@ -171,6 +332,8 @@ static int bridge_compare(struct rtnl_link *_a,
> struct rtnl_link *_b,
>  	diff |= BRIDGE_DIFF(PORT_STATE,	a->b_port_state != b-
> >b_port_state);
>  	diff |= BRIDGE_DIFF(PRIORITY, a->b_priority != b-
> >b_priority);
>  	diff |= BRIDGE_DIFF(COST, a->b_cost != b->b_cost);
> +	diff |= BRIDGE_DIFF(PORT_VLAN, memcmp(&a->vlan_info, &b-
> >vlan_info,
> +					      sizeof(struct
> bridge_vlan)));
>  
>  	if (flags & LOOSE_COMPARISON)
>  		diff |= BRIDGE_DIFF(FLAGS,
> @@ -503,6 +666,44 @@ int rtnl_link_bridge_str2flags(const char *name)
>  
>  /** @} */
>  
> +int rtnl_link_bridge_pvid(struct rtnl_link *link)

this function seems to be public, so its missing in the .sym file.

> +{
> +	struct bridge_data *bd = link->l_af_data[AF_BRIDGE];
> +
> +	if (bd->ce_mask & BRIDGE_ATTR_PORT_VLAN)
> +		return (int) bd->vlan_info.pvid;
> +
> +	return -EINVAL;
> +}
> +
> +int rtnl_link_bridge_has_vlan(struct rtnl_link *link)

same as above

> +{
> +	struct bridge_data *bd = link->l_af_data[AF_BRIDGE];
> +	int i;
> +
> +	if (bd->ce_mask & BRIDGE_ATTR_PORT_VLAN) {
> +		if (bd->vlan_info.pvid)
> +			return 1;
> +
> +		for (i = 0; i < BR_VLAN_BITMAP_LEN; ++i) {
> +			if (bd->vlan_info.vlan_bitmap[i] ||
> +			    bd->vlan_info.untagged_bitmap[i])
> +				return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
> +struct bridge_vlan *rtnl_bridge_get_port_vlan(struct rtnl_link
> *link)

same as above and I guess the function should be renamed to
rtnl_link_bridge_get_port_vlan

> +{
> +	struct bridge_data *data = link->l_af_data[AF_BRIDGE];
> +
> +	if (data && (data->ce_mask & BRIDGE_ATTR_PORT_VLAN))
> +		return &data->vlan_info;
> +
> +	return NULL;
> +}
> +
>  static struct rtnl_link_af_ops bridge_ops = {
>  	.ao_family			= AF_BRIDGE,
>  	.ao_alloc			= &bridge_alloc,
> @@ -511,6 +712,8 @@ static struct rtnl_link_af_ops bridge_ops = {
>  	.ao_parse_protinfo		= &bridge_parse_protinfo,
>  	.ao_dump[NL_DUMP_DETAILS]	= &bridge_dump_details,
>  	.ao_compare			= &bridge_compare,
> +	.ao_parse_af			= &bridge_parse_af,
> +	.ao_get_af			= &bridge_get_af,
>  };
>  
>  static void __init bridge_init(void)
> diff --git a/lib/route/neigh.c b/lib/route/neigh.c
> index 6059e7f4d8f3..436d766a6a11 100644
> --- a/lib/route/neigh.c
> +++ b/lib/route/neigh.c
> @@ -210,6 +210,7 @@ static void neigh_keygen(struct nl_object *obj,
> uint32_t *hashkey,
>  	struct neigh_hash_key {
>  		uint32_t	n_family;
>  		uint32_t	n_ifindex;
> +		uint16_t	n_vlan;
>  		char		n_addr[0];
>  	} __attribute__((packed)) *nkey;
>  #ifdef NL_DEBUG
> @@ -234,6 +235,7 @@ static void neigh_keygen(struct nl_object *obj,
> uint32_t *hashkey,
>  	}
>  	nkey->n_family = neigh->n_family;
>  	if (neigh->n_family == AF_BRIDGE) {
> +		nkey->n_vlan = neigh->n_vlan;
>  		if (neigh->n_flags & NTF_SELF)
>  			nkey->n_ifindex = neigh->n_ifindex;
>  		else
> @@ -316,9 +318,9 @@ static uint32_t neigh_id_attrs_get(struct
> nl_object *obj)
>  
>  	if (neigh->n_family == AF_BRIDGE) {
>  		if (neigh->n_flags & NTF_SELF)
> -			return (NEIGH_ATTR_LLADDR |
> NEIGH_ATTR_FAMILY | NEIGH_ATTR_IFINDEX);
> +			return (NEIGH_ATTR_LLADDR |
> NEIGH_ATTR_FAMILY | NEIGH_ATTR_IFINDEX | NEIGH_ATTR_VLAN);
>  		else
> -			return (NEIGH_ATTR_LLADDR |
> NEIGH_ATTR_FAMILY | NEIGH_ATTR_MASTER);
> +			return (NEIGH_ATTR_LLADDR |
> NEIGH_ATTR_FAMILY | NEIGH_ATTR_MASTER | NEIGH_ATTR_VLAN);
>  	} else
>  		return (NEIGH_ATTR_IFINDEX | NEIGH_ATTR_DST |
> NEIGH_ATTR_FAMILY);
>  }
> @@ -472,6 +474,9 @@ static void neigh_dump_line(struct nl_object *a,
> struct nl_dump_params *p)
>  		nl_dump(p, "lladdr %s ",
>  			nl_addr2str(n->n_lladdr, lladdr,
> sizeof(lladdr)));
>  
> +	if (n->ce_mask & NEIGH_ATTR_VLAN)
> +		nl_dump(p, "vlan %d ", n->n_vlan);
> +
>  	rtnl_neigh_state2str(n->n_state, state, sizeof(state));
>  	rtnl_neigh_flags2str(n->n_flags, flags, sizeof(flags));
>  



More information about the libnl mailing list