thr3ads.net - Linux Ethernet Bridging - [Bridge] [RFC PATCH net-next 17/19] bridge: mcast: Allow user space to add (*, G) with a source list and filter mode [Nov 2022]

If this information is useful, please help other people find it:
Share via:

Ido Schimmel

2022-Oct-18 12:04 UTC

[Bridge] [RFC PATCH net-next 17/19] bridge: mcast: Allow user space to add (*, G) with a source list and filter mode

Add new netlink attributes to the RTM_NEWMDB request that allow user
space to add (*, G) with a source list and filter mode.

The RTM_NEWMDB message can already dump such entries (created by the
kernel) so there is no need to add dump support. However, the message
contains a different set of attributes depending if it is a request or a
response. The naming and structure of the new attributes try to follow
the existing ones used in the response.

Request:

[ struct nlmsghdr ]
[ struct br_port_msg ]
[ MDBA_SET_ENTRY ]
	struct br_mdb_entry
[ MDBA_SET_ENTRY_ATTRS ]
	[ MDBE_ATTR_SOURCE ]
		struct in_addr / struct in6_addr
	[ MDBE_ATTR_SRC_LIST ]		// new
		[ MDBE_SRC_LIST_ENTRY ]
			[ MDBE_SRCATTR_ADDRESS ]
				struct in_addr / struct in6_addr
		[ ...]
	[ MDBE_ATTR_GROUP_MODE ]	// new
		u8

Response:

[ struct nlmsghdr ]
[ struct br_port_msg ]
[ MDBA_MDB ]
	[ MDBA_MDB_ENTRY ]
		[ MDBA_MDB_ENTRY_INFO ]
			struct br_mdb_entry
		[ MDBA_MDB_EATTR_TIMER ]
			u32
		[ MDBA_MDB_EATTR_SOURCE ]
			struct in_addr / struct in6_addr
		[ MDBA_MDB_EATTR_RTPROT ]
			u8
		[ MDBA_MDB_EATTR_SRC_LIST ]
			[ MDBA_MDB_SRCLIST_ENTRY ]
				[ MDBA_MDB_SRCATTR_ADDRESS ]
					struct in_addr / struct in6_addr
				[ MDBA_MDB_SRCATTR_TIMER ]
					u8
			[...]
		[ MDBA_MDB_EATTR_GROUP_MODE ]
			u8

Signed-off-by: Ido Schimmel <idosch at nvidia.com>
---
 include/uapi/linux/if_bridge.h |  20 +++++
 net/bridge/br_mdb.c            | 132 +++++++++++++++++++++++++++++++++
 2 files changed, 152 insertions(+)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index a86a7e7b811f..0d9fe73fc48c 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -723,10 +723,30 @@ enum {
 enum {
 	MDBE_ATTR_UNSPEC,
 	MDBE_ATTR_SOURCE,
+	MDBE_ATTR_SRC_LIST,
+	MDBE_ATTR_GROUP_MODE,
 	__MDBE_ATTR_MAX,
 };
 #define MDBE_ATTR_MAX (__MDBE_ATTR_MAX - 1)
 
+/* per mdb entry source */
+enum {
+	MDBE_SRC_LIST_UNSPEC,
+	MDBE_SRC_LIST_ENTRY,
+	__MDBE_SRC_LIST_MAX,
+};
+#define MDBE_SRC_LIST_MAX (__MDBE_SRC_LIST_MAX - 1)
+
+/* per mdb entry per source attributes
+ * these are embedded in MDBE_SRC_LIST_ENTRY
+ */
+enum {
+	MDBE_SRCATTR_UNSPEC,
+	MDBE_SRCATTR_ADDRESS,
+	__MDBE_SRCATTR_MAX,
+};
+#define MDBE_SRCATTR_MAX (__MDBE_SRCATTR_MAX - 1)
+
 /* Embedded inside LINK_XSTATS_TYPE_BRIDGE */
 enum {
 	BRIDGE_XSTATS_UNSPEC,
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 8fc8816a76bf..909b0fb49a0c 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -663,10 +663,25 @@ void br_rtr_notify(struct net_device *dev, struct
net_bridge_mcast_port *pmctx,
 	rtnl_set_sk_err(net, RTNLGRP_MDB, err);
 }
 
+static const struct nla_policy
+br_mdbe_src_list_entry_pol[MDBE_SRCATTR_MAX + 1] = {
+	[MDBE_SRCATTR_ADDRESS] = NLA_POLICY_RANGE(NLA_BINARY,
+						  sizeof(struct in_addr),
+						  sizeof(struct in6_addr)),
+};
+
+static const struct nla_policy
+br_mdbe_src_list_pol[MDBE_SRC_LIST_MAX + 1] = {
+	[MDBE_SRC_LIST_ENTRY] = NLA_POLICY_NESTED(br_mdbe_src_list_entry_pol),
+};
+
 static const struct nla_policy br_mdbe_attrs_pol[MDBE_ATTR_MAX + 1] = {
 	[MDBE_ATTR_SOURCE] = NLA_POLICY_RANGE(NLA_BINARY,
 					      sizeof(struct in_addr),
 					      sizeof(struct in6_addr)),
+	[MDBE_ATTR_GROUP_MODE] = NLA_POLICY_RANGE(NLA_U8, MCAST_EXCLUDE,
+						  MCAST_INCLUDE),
+	[MDBE_ATTR_SRC_LIST] = NLA_POLICY_NESTED(br_mdbe_src_list_pol),
 };
 
 static bool is_valid_mdb_entry(struct br_mdb_entry *entry,
@@ -1052,6 +1067,73 @@ static int __br_mdb_add(struct br_mdb_config *cfg,
 	return ret;
 }
 
+static int br_mdb_config_src_entry_init(struct nlattr *src_entry,
+					struct br_mdb_config *cfg,
+					struct netlink_ext_ack *extack)
+{
+	struct nlattr *tb[MDBE_SRCATTR_MAX + 1];
+	struct br_mdb_src_entry *src;
+	int err;
+
+	err = nla_parse_nested(tb, MDBE_SRCATTR_MAX, src_entry,
+			       br_mdbe_src_list_entry_pol, extack);
+	if (err)
+		return err;
+
+	if (NL_REQ_ATTR_CHECK(extack, src_entry, tb, MDBE_SRCATTR_ADDRESS))
+		return -EINVAL;
+
+	if (!is_valid_mdb_source(tb[MDBE_SRCATTR_ADDRESS],
+				 cfg->entry->addr.proto, extack))
+		return -EINVAL;
+
+	src = kzalloc(sizeof(*src), GFP_KERNEL);
+	if (!src)
+		return -ENOMEM;
+	src->addr.proto = cfg->entry->addr.proto;
+	nla_memcpy(&src->addr.src, tb[MDBE_SRCATTR_ADDRESS],
+		   nla_len(tb[MDBE_SRCATTR_ADDRESS]));
+	list_add_tail(&src->list, &cfg->src_list);
+
+	return 0;
+}
+
+static void br_mdb_config_src_entry_fini(struct br_mdb_src_entry *src)
+{
+	list_del(&src->list);
+	kfree(src);
+}
+
+static int br_mdb_config_src_list_init(struct nlattr *src_list,
+				       struct br_mdb_config *cfg,
+				       struct netlink_ext_ack *extack)
+{
+	struct br_mdb_src_entry *src, *tmp;
+	struct nlattr *src_entry;
+	int rem, err;
+
+	nla_for_each_nested(src_entry, src_list, rem) {
+		err = br_mdb_config_src_entry_init(src_entry, cfg, extack);
+		if (err)
+			goto err_src_entry_init;
+	}
+
+	return 0;
+
+err_src_entry_init:
+	list_for_each_entry_safe(src, tmp, &cfg->src_list, list)
+		br_mdb_config_src_entry_fini(src);
+	return err;
+}
+
+static void br_mdb_config_src_list_fini(struct br_mdb_config *cfg)
+{
+	struct br_mdb_src_entry *src, *tmp;
+
+	list_for_each_entry_safe(src, tmp, &cfg->src_list, list)
+		br_mdb_config_src_entry_fini(src);
+}
+
 static int br_mdb_config_attrs_init(struct nlattr *set_attrs,
 				    struct br_mdb_config *cfg,
 				    struct netlink_ext_ack *extack)
@@ -1071,9 +1153,52 @@ static int br_mdb_config_attrs_init(struct nlattr
*set_attrs,
 
 	__mdb_entry_to_br_ip(cfg->entry, &cfg->group, mdb_attrs);
 
+	if (mdb_attrs[MDBE_ATTR_GROUP_MODE]) {
+		if (!cfg->p) {
+			NL_SET_ERR_MSG_MOD(extack, "Filter mode cannot be set for host
groups");
+			return -EINVAL;
+		}
+		if (!br_multicast_is_star_g(&cfg->group)) {
+			NL_SET_ERR_MSG_MOD(extack, "Filter mode can only be set for (*, G)
entries");
+			return -EINVAL;
+		}
+		cfg->filter_mode = nla_get_u8(mdb_attrs[MDBE_ATTR_GROUP_MODE]);
+	} else {
+		cfg->filter_mode = MCAST_EXCLUDE;
+	}
+
+	if (mdb_attrs[MDBE_ATTR_SRC_LIST]) {
+		if (!cfg->p) {
+			NL_SET_ERR_MSG_MOD(extack, "Source list cannot be set for host
groups");
+			return -EINVAL;
+		}
+		if (!br_multicast_is_star_g(&cfg->group)) {
+			NL_SET_ERR_MSG_MOD(extack, "Source list can only be set for (*, G)
entries");
+			return -EINVAL;
+		}
+		if (!mdb_attrs[MDBE_ATTR_GROUP_MODE]) {
+			NL_SET_ERR_MSG_MOD(extack, "Source list cannot be set without filter
mode");
+			return -EINVAL;
+		}
+		err = br_mdb_config_src_list_init(mdb_attrs[MDBE_ATTR_SRC_LIST],
+						  cfg, extack);
+		if (err)
+			return err;
+	}
+
+	if (list_empty(&cfg->src_list) && cfg->filter_mode ==
MCAST_INCLUDE) {
+		NL_SET_ERR_MSG_MOD(extack, "Cannot add (*, G) INCLUDE with an empty
source list");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
+static void br_mdb_config_attrs_fini(struct br_mdb_config *cfg)
+{
+	br_mdb_config_src_list_fini(cfg);
+}
+
 static int br_mdb_config_init(struct net *net, struct sk_buff *skb,
 			      struct nlmsghdr *nlh, struct br_mdb_config *cfg,
 			      struct netlink_ext_ack *extack)
@@ -1164,6 +1289,11 @@ static int br_mdb_config_init(struct net *net, struct
sk_buff *skb,
 	return 0;
 }
 
+static void br_mdb_config_fini(struct br_mdb_config *cfg)
+{
+	br_mdb_config_attrs_fini(cfg);
+}
+
 static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 		      struct netlink_ext_ack *extack)
 {
@@ -1222,6 +1352,7 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr
*nlh,
 	}
 
 out:
+	br_mdb_config_fini(&cfg);
 	return err;
 }
 
@@ -1297,6 +1428,7 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr
*nlh,
 		err = __br_mdb_del(&cfg);
 	}
 
+	br_mdb_config_fini(&cfg);
 	return err;
 }
 
-- 
2.37.3

Nikolay Aleksandrov

2022-Oct-19 13:28 UTC

head link

[Bridge] [RFC PATCH net-next 17/19] bridge: mcast: Allow user space to add (*, G) with a source list and filter mode

On 18/10/2022 15:04, Ido Schimmel wrote:> Add new netlink attributes to the RTM_NEWMDB request that allow user
> space to add (*, G) with a source list and filter mode.
> 
> The RTM_NEWMDB message can already dump such entries (created by the
> kernel) so there is no need to add dump support. However, the message
> contains a different set of attributes depending if it is a request or a
> response. The naming and structure of the new attributes try to follow
> the existing ones used in the response.
> 
> Request:
> 
> [ struct nlmsghdr ]
> [ struct br_port_msg ]
> [ MDBA_SET_ENTRY ]
> 	struct br_mdb_entry
> [ MDBA_SET_ENTRY_ATTRS ]
> 	[ MDBE_ATTR_SOURCE ]
> 		struct in_addr / struct in6_addr
> 	[ MDBE_ATTR_SRC_LIST ]		// new
> 		[ MDBE_SRC_LIST_ENTRY ]
> 			[ MDBE_SRCATTR_ADDRESS ]
> 				struct in_addr / struct in6_addr
> 		[ ...]
> 	[ MDBE_ATTR_GROUP_MODE ]	// new
> 		u8
> 
> Response:
> 
> [ struct nlmsghdr ]
> [ struct br_port_msg ]
> [ MDBA_MDB ]
> 	[ MDBA_MDB_ENTRY ]
> 		[ MDBA_MDB_ENTRY_INFO ]
> 			struct br_mdb_entry
> 		[ MDBA_MDB_EATTR_TIMER ]
> 			u32
> 		[ MDBA_MDB_EATTR_SOURCE ]
> 			struct in_addr / struct in6_addr
> 		[ MDBA_MDB_EATTR_RTPROT ]
> 			u8
> 		[ MDBA_MDB_EATTR_SRC_LIST ]
> 			[ MDBA_MDB_SRCLIST_ENTRY ]
> 				[ MDBA_MDB_SRCATTR_ADDRESS ]
> 					struct in_addr / struct in6_addr
> 				[ MDBA_MDB_SRCATTR_TIMER ]
> 					u8
> 			[...]
> 		[ MDBA_MDB_EATTR_GROUP_MODE ]
> 			u8
> 
> Signed-off-by: Ido Schimmel <idosch at nvidia.com>
> ---
>  include/uapi/linux/if_bridge.h |  20 +++++
>  net/bridge/br_mdb.c            | 132 +++++++++++++++++++++++++++++++++
>  2 files changed, 152 insertions(+)
> 
> diff --git a/include/uapi/linux/if_bridge.h
b/include/uapi/linux/if_bridge.h
> index a86a7e7b811f..0d9fe73fc48c 100644
> --- a/include/uapi/linux/if_bridge.h
> +++ b/include/uapi/linux/if_bridge.h
> @@ -723,10 +723,30 @@ enum {
>  enum {
>  	MDBE_ATTR_UNSPEC,
>  	MDBE_ATTR_SOURCE,
> +	MDBE_ATTR_SRC_LIST,
> +	MDBE_ATTR_GROUP_MODE,
>  	__MDBE_ATTR_MAX,
>  };
>  #define MDBE_ATTR_MAX (__MDBE_ATTR_MAX - 1)
>  
> +/* per mdb entry source */
> +enum {
> +	MDBE_SRC_LIST_UNSPEC,
> +	MDBE_SRC_LIST_ENTRY,
> +	__MDBE_SRC_LIST_MAX,
> +};
> +#define MDBE_SRC_LIST_MAX (__MDBE_SRC_LIST_MAX - 1)
> +
> +/* per mdb entry per source attributes
> + * these are embedded in MDBE_SRC_LIST_ENTRY
> + */
> +enum {
> +	MDBE_SRCATTR_UNSPEC,
> +	MDBE_SRCATTR_ADDRESS,
> +	__MDBE_SRCATTR_MAX,
> +};
> +#define MDBE_SRCATTR_MAX (__MDBE_SRCATTR_MAX - 1)
> +
>  /* Embedded inside LINK_XSTATS_TYPE_BRIDGE */
>  enum {
>  	BRIDGE_XSTATS_UNSPEC,
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index 8fc8816a76bf..909b0fb49a0c 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -663,10 +663,25 @@ void br_rtr_notify(struct net_device *dev, struct
net_bridge_mcast_port *pmctx,
>  	rtnl_set_sk_err(net, RTNLGRP_MDB, err);
>  }
>  
> +static const struct nla_policy
> +br_mdbe_src_list_entry_pol[MDBE_SRCATTR_MAX + 1] = {
> +	[MDBE_SRCATTR_ADDRESS] = NLA_POLICY_RANGE(NLA_BINARY,
> +						  sizeof(struct in_addr),
> +						  sizeof(struct in6_addr)),
> +};
> +
> +static const struct nla_policy
> +br_mdbe_src_list_pol[MDBE_SRC_LIST_MAX + 1] = {
> +	[MDBE_SRC_LIST_ENTRY] = NLA_POLICY_NESTED(br_mdbe_src_list_entry_pol),
> +};
> +
>  static const struct nla_policy br_mdbe_attrs_pol[MDBE_ATTR_MAX + 1] = {
>  	[MDBE_ATTR_SOURCE] = NLA_POLICY_RANGE(NLA_BINARY,
>  					      sizeof(struct in_addr),
>  					      sizeof(struct in6_addr)),
> +	[MDBE_ATTR_GROUP_MODE] = NLA_POLICY_RANGE(NLA_U8, MCAST_EXCLUDE,
> +						  MCAST_INCLUDE),
> +	[MDBE_ATTR_SRC_LIST] = NLA_POLICY_NESTED(br_mdbe_src_list_pol),
>  };
>  
>  static bool is_valid_mdb_entry(struct br_mdb_entry *entry,
> @@ -1052,6 +1067,73 @@ static int __br_mdb_add(struct br_mdb_config *cfg,
>  	return ret;
>  }
>  
> +static int br_mdb_config_src_entry_init(struct nlattr *src_entry,
> +					struct br_mdb_config *cfg,
> +					struct netlink_ext_ack *extack)
> +{
> +	struct nlattr *tb[MDBE_SRCATTR_MAX + 1];
> +	struct br_mdb_src_entry *src;
> +	int err;
> +
> +	err = nla_parse_nested(tb, MDBE_SRCATTR_MAX, src_entry,
> +			       br_mdbe_src_list_entry_pol, extack);
> +	if (err)
> +		return err;
> +
> +	if (NL_REQ_ATTR_CHECK(extack, src_entry, tb, MDBE_SRCATTR_ADDRESS))
> +		return -EINVAL;
> +
> +	if (!is_valid_mdb_source(tb[MDBE_SRCATTR_ADDRESS],
> +				 cfg->entry->addr.proto, extack))
> +		return -EINVAL;
> +
> +	src = kzalloc(sizeof(*src), GFP_KERNEL);
> +	if (!src)
> +		return -ENOMEM;
> +	src->addr.proto = cfg->entry->addr.proto;
> +	nla_memcpy(&src->addr.src, tb[MDBE_SRCATTR_ADDRESS],
> +		   nla_len(tb[MDBE_SRCATTR_ADDRESS]));
> +	list_add_tail(&src->list, &cfg->src_list);
> +
> +	return 0;
> +}
> +
> +static void br_mdb_config_src_entry_fini(struct br_mdb_src_entry *src)
> +{
> +	list_del(&src->list);
> +	kfree(src);
> +}
> +
> +static int br_mdb_config_src_list_init(struct nlattr *src_list,
> +				       struct br_mdb_config *cfg,
> +				       struct netlink_ext_ack *extack)
> +{
> +	struct br_mdb_src_entry *src, *tmp;
> +	struct nlattr *src_entry;
> +	int rem, err;
> +
> +	nla_for_each_nested(src_entry, src_list, rem) {
> +		err = br_mdb_config_src_entry_init(src_entry, cfg, extack);
Hmm, since we know the exact number of these (due to attr embedding) can't
we allocate
all at once and drop the list? They should not be more than 32
(PG_SRC_ENT_LIMIT) IIRC,
which makes it at most 1152 bytes. Might simplify the code a bit and reduce
allocations.
> +		if (err)
> +			goto err_src_entry_init;
> +	}
> +
> +	return 0;
> +
> +err_src_entry_init:
> +	list_for_each_entry_safe(src, tmp, &cfg->src_list, list)
> +		br_mdb_config_src_entry_fini(src);
> +	return err;
> +}
> +
> +static void br_mdb_config_src_list_fini(struct br_mdb_config *cfg)
> +{
> +	struct br_mdb_src_entry *src, *tmp;
> +
> +	list_for_each_entry_safe(src, tmp, &cfg->src_list, list)
> +		br_mdb_config_src_entry_fini(src);
> +}
> +
>  static int br_mdb_config_attrs_init(struct nlattr *set_attrs,
>  				    struct br_mdb_config *cfg,
>  				    struct netlink_ext_ack *extack)
> @@ -1071,9 +1153,52 @@ static int br_mdb_config_attrs_init(struct nlattr
*set_attrs,
>  
>  	__mdb_entry_to_br_ip(cfg->entry, &cfg->group, mdb_attrs);
>  
> +	if (mdb_attrs[MDBE_ATTR_GROUP_MODE]) {
> +		if (!cfg->p) {
> +			NL_SET_ERR_MSG_MOD(extack, "Filter mode cannot be set for host
groups");
> +			return -EINVAL;
> +		}
> +		if (!br_multicast_is_star_g(&cfg->group)) {
> +			NL_SET_ERR_MSG_MOD(extack, "Filter mode can only be set for (*, G)
entries");
> +			return -EINVAL;
> +		}
> +		cfg->filter_mode = nla_get_u8(mdb_attrs[MDBE_ATTR_GROUP_MODE]);
> +	} else {
> +		cfg->filter_mode = MCAST_EXCLUDE;
> +	}
> +
> +	if (mdb_attrs[MDBE_ATTR_SRC_LIST]) {
> +		if (!cfg->p) {
> +			NL_SET_ERR_MSG_MOD(extack, "Source list cannot be set for host
groups");
> +			return -EINVAL;
> +		}
> +		if (!br_multicast_is_star_g(&cfg->group)) {
> +			NL_SET_ERR_MSG_MOD(extack, "Source list can only be set for (*, G)
entries");
> +			return -EINVAL;
> +		}
> +		if (!mdb_attrs[MDBE_ATTR_GROUP_MODE]) {
> +			NL_SET_ERR_MSG_MOD(extack, "Source list cannot be set without
filter mode");
> +			return -EINVAL;
> +		}
> +		err = br_mdb_config_src_list_init(mdb_attrs[MDBE_ATTR_SRC_LIST],
> +						  cfg, extack);
> +		if (err)
> +			return err;
> +	}
> +
> +	if (list_empty(&cfg->src_list) && cfg->filter_mode ==
MCAST_INCLUDE) {
> +		NL_SET_ERR_MSG_MOD(extack, "Cannot add (*, G) INCLUDE with an empty
source list");
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  
> +static void br_mdb_config_attrs_fini(struct br_mdb_config *cfg)
> +{
> +	br_mdb_config_src_list_fini(cfg);
> +}
> +
>  static int br_mdb_config_init(struct net *net, struct sk_buff *skb,
>  			      struct nlmsghdr *nlh, struct br_mdb_config *cfg,
>  			      struct netlink_ext_ack *extack)
> @@ -1164,6 +1289,11 @@ static int br_mdb_config_init(struct net *net,
struct sk_buff *skb,
>  	return 0;
>  }
>  
> +static void br_mdb_config_fini(struct br_mdb_config *cfg)
> +{
> +	br_mdb_config_attrs_fini(cfg);
> +}
> +
Is there more coming to these two _fini helpers? If not, I think one would be
enough, i.e.
just call br_mdb_config_src_list_fini() from br_mdb_config_fini()
>  static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
>  		      struct netlink_ext_ack *extack)
>  {
> @@ -1222,6 +1352,7 @@ static int br_mdb_add(struct sk_buff *skb, struct
nlmsghdr *nlh,
>  	}
>  
>  out:
> +	br_mdb_config_fini(&cfg);
>  	return err;
>  }
>  
> @@ -1297,6 +1428,7 @@ static int br_mdb_del(struct sk_buff *skb, struct
nlmsghdr *nlh,
>  		err = __br_mdb_del(&cfg);
>  	}
>  
> +	br_mdb_config_fini(&cfg);
>  	return err;
>  }
>

Ido Schimmel

2022-Nov-03 09:09 UTC

head link

[Bridge] [RFC PATCH net-next 17/19] bridge: mcast: Allow user space to add (*, G) with a source list and filter mode

On Wed, Oct 19, 2022 at 04:28:23PM +0300, Nikolay Aleksandrov
wrote:> On 18/10/2022 15:04, Ido Schimmel wrote:
> > +static int br_mdb_config_src_list_init(struct nlattr *src_list,
> > +				       struct br_mdb_config *cfg,
> > +				       struct netlink_ext_ack *extack)
> > +{
> > +	struct br_mdb_src_entry *src, *tmp;
> > +	struct nlattr *src_entry;
> > +	int rem, err;
> > +
> > +	nla_for_each_nested(src_entry, src_list, rem) {
> > +		err = br_mdb_config_src_entry_init(src_entry, cfg, extack);
> 
> Hmm, since we know the exact number of these (due to attr embedding)
can't we allocate
> all at once and drop the list? They should not be more than 32
(PG_SRC_ENT_LIMIT) IIRC,
> which makes it at most 1152 bytes. Might simplify the code a bit and reduce
allocations.
I didn't see how I can reliably determine the exact number of source
entries without going all the 'MDBE_SRC_LIST_ENTRY' attributes. I mean,
the entries can have varying sizes in case user space provided mixed
IPv4/IPv6 sources (which will be rejected later on) and in the future we
might have more attributes per-source entry other than the address
(e.g., source timer), which might be specified only for a subset of the
entries.

So, I did end up converting to an array like you suggested, but I'm
going over the entries twice. Once to understand how large the array
should be and again to initialize it. It's control path so it should be
fine. The advantages are that the number of allocations are reduced and
that I can reject a too long source list before doing any processing:

if (cfg->num_src_entries >= PG_SRC_ENT_LIMIT) {
	NL_SET_ERR_MSG_FMT_MOD(extack, "Exceeded maximum number of source entries
(%u)",
			       PG_SRC_ENT_LIMIT);
	return -EINVAL;
}

[...]
> > +static void br_mdb_config_fini(struct br_mdb_config *cfg)
> > +{
> > +	br_mdb_config_attrs_fini(cfg);
> > +}
> > +
> 
> Is there more coming to these two _fini helpers? If not, I think one would
be enough, i.e.
> just call br_mdb_config_src_list_fini() from br_mdb_config_fini()
Done.

Thanks!

Linux Ethernet Bridging - Nov 2022 - [Bridge] [RFC PATCH net-next 17/19] bridge: mcast: Allow user space to add (*, G) with a source list and filter mode

[Bridge] [RFC PATCH net-next 17/19] bridge: mcast: Allow user space to add (*, G) with a source list and filter mode

[Bridge] [RFC PATCH net-next 17/19] bridge: mcast: Allow user space to add (*, G) with a source list and filter mode

[Bridge] [RFC PATCH net-next 17/19] bridge: mcast: Allow user space to add (*, G) with a source list and filter mode