This patch series extends MRP netlink interface with IFLA_BRIDGE_MRP_CLEAR.
To allow the userspace to clear all MRP instances when is started. The
second patch in the series fix different sparse warnings.
v3:
- add the second patch to fix sparse warnings
v2:
- use list_for_each_entry_safe instead of list_for_each_entry_rcu
when deleting mrp instances
Horatiu Vultur (2):
bridge: mrp: Extend MRP netlink interface with IFLA_BRIDGE_MRP_CLEAR
bridge: mrp: Fix endian conversion and some other warnings
include/uapi/linux/if_bridge.h | 8 ++++++++
net/bridge/br_mrp.c | 17 ++++++++++++++++-
net/bridge/br_mrp_netlink.c | 26 ++++++++++++++++++++++++++
net/bridge/br_private.h | 2 +-
net/bridge/br_private_mrp.h | 3 ++-
5 files changed, 53 insertions(+), 3 deletions(-)
--
2.26.2
The following sparse warnings are fixed:
net/bridge/br_mrp.c:106:18: warning: incorrect type in assignment (different base types)
net/bridge/br_mrp.c:106:18: expected unsigned short [usertype]
net/bridge/br_mrp.c:106:18: got restricted __be16 [usertype]
net/bridge/br_mrp.c:281:23: warning: incorrect type in argument 1 (different modifiers)
net/bridge/br_mrp.c:281:23: expected struct list_head *entry
net/bridge/br_mrp.c:281:23: got struct list_head [noderef] *
net/bridge/br_mrp.c:332:28: warning: incorrect type in argument 1 (different modifiers)
net/bridge/br_mrp.c:332:28: expected struct list_head *new
net/bridge/br_mrp.c:332:28: got struct list_head [noderef] *
net/bridge/br_mrp.c:332:40: warning: incorrect type in argument 2 (different modifiers)
net/bridge/br_mrp.c:332:40: expected struct list_head *head
net/bridge/br_mrp.c:332:40: got struct list_head [noderef] *
net/bridge/br_mrp.c:691:29: warning: incorrect type in argument 1 (different modifiers)
net/bridge/br_mrp.c:691:29: expected struct list_head const *head
net/bridge/br_mrp.c:691:29: got struct list_head [noderef] *
net/bridge/br_mrp.c:383:9: sparse: sparse: dereference of noderef expression
net/bridge/br_mrp.c:383:9: sparse: sparse: dereference of noderef expression
net/bridge/br_mrp.c:383:9: sparse: sparse: dereference of noderef expression
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Horatiu Vultur <[email protected]>
---
net/bridge/br_mrp.c | 2 +-
net/bridge/br_private.h | 2 +-
net/bridge/br_private_mrp.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
index dcbf21b91313d..7541482898642 100644
--- a/net/bridge/br_mrp.c
+++ b/net/bridge/br_mrp.c
@@ -86,7 +86,7 @@ static struct sk_buff *br_mrp_skb_alloc(struct net_bridge_port *p,
{
struct ethhdr *eth_hdr;
struct sk_buff *skb;
- u16 *version;
+ __be16 *version;
skb = dev_alloc_skb(MRP_MAX_FRAME_LENGTH);
if (!skb)
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 6a7d8e218ae7e..bbffbaac1beb6 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -434,7 +434,7 @@ struct net_bridge {
struct hlist_head fdb_list;
#if IS_ENABLED(CONFIG_BRIDGE_MRP)
- struct list_head __rcu mrp_list;
+ struct list_head mrp_list;
#endif
};
diff --git a/net/bridge/br_private_mrp.h b/net/bridge/br_private_mrp.h
index 25c3b8596c25b..3243a2cc3a729 100644
--- a/net/bridge/br_private_mrp.h
+++ b/net/bridge/br_private_mrp.h
@@ -8,7 +8,7 @@
struct br_mrp {
/* list of mrp instances */
- struct list_head __rcu list;
+ struct list_head list;
struct net_bridge_port __rcu *p_port;
struct net_bridge_port __rcu *s_port;
--
2.26.2
In case the userspace daemon dies, then when is restarted it doesn't
know if there are any MRP instances in the kernel. Therefore extend the
netlink interface to allow the daemon to clear all MRP instances when is
started.
Signed-off-by: Horatiu Vultur <[email protected]>
---
include/uapi/linux/if_bridge.h | 8 ++++++++
net/bridge/br_mrp.c | 15 +++++++++++++++
net/bridge/br_mrp_netlink.c | 26 ++++++++++++++++++++++++++
net/bridge/br_private_mrp.h | 1 +
4 files changed, 50 insertions(+)
diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index caa6914a3e53a..2ae7d0c0d46b8 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -166,6 +166,7 @@ enum {
IFLA_BRIDGE_MRP_RING_STATE,
IFLA_BRIDGE_MRP_RING_ROLE,
IFLA_BRIDGE_MRP_START_TEST,
+ IFLA_BRIDGE_MRP_CLEAR,
__IFLA_BRIDGE_MRP_MAX,
};
@@ -228,6 +229,13 @@ enum {
#define IFLA_BRIDGE_MRP_START_TEST_MAX (__IFLA_BRIDGE_MRP_START_TEST_MAX - 1)
+enum {
+ IFLA_BRIDGE_MRP_CLEAR_UNSPEC,
+ __IFLA_BRIDGE_MRP_CLEAR_MAX,
+};
+
+#define IFLA_BRIDGE_MRP_CLEAR_MAX (__IFLA_BRIDGE_MRP_CLEAR_MAX - 1)
+
struct br_mrp_instance {
__u32 ring_id;
__u32 p_ifindex;
diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
index 779e1eb754430..dcbf21b91313d 100644
--- a/net/bridge/br_mrp.c
+++ b/net/bridge/br_mrp.c
@@ -372,6 +372,21 @@ int br_mrp_del(struct net_bridge *br, struct br_mrp_instance *instance)
return 0;
}
+/* Deletes all MRP instances on the bridge
+ * note: called under rtnl_lock
+ */
+int br_mrp_clear(struct net_bridge *br)
+{
+ struct br_mrp *mrp;
+ struct br_mrp *tmp;
+
+ list_for_each_entry_safe(mrp, tmp, &br->mrp_list, list) {
+ br_mrp_del_impl(br, mrp);
+ }
+
+ return 0;
+}
+
/* Set port state, port state can be forwarding, blocked or disabled
* note: already called with rtnl_lock
*/
diff --git a/net/bridge/br_mrp_netlink.c b/net/bridge/br_mrp_netlink.c
index 34b3a8776991f..5e743538464f6 100644
--- a/net/bridge/br_mrp_netlink.c
+++ b/net/bridge/br_mrp_netlink.c
@@ -14,6 +14,7 @@ static const struct nla_policy br_mrp_policy[IFLA_BRIDGE_MRP_MAX + 1] = {
[IFLA_BRIDGE_MRP_RING_STATE] = { .type = NLA_NESTED },
[IFLA_BRIDGE_MRP_RING_ROLE] = { .type = NLA_NESTED },
[IFLA_BRIDGE_MRP_START_TEST] = { .type = NLA_NESTED },
+ [IFLA_BRIDGE_MRP_CLEAR] = { .type = NLA_NESTED },
};
static const struct nla_policy
@@ -235,6 +236,25 @@ static int br_mrp_start_test_parse(struct net_bridge *br, struct nlattr *attr,
return br_mrp_start_test(br, &test);
}
+static const struct nla_policy
+br_mrp_clear_policy[IFLA_BRIDGE_MRP_CLEAR_MAX + 1] = {
+ [IFLA_BRIDGE_MRP_CLEAR_UNSPEC] = { .type = NLA_REJECT },
+};
+
+static int br_mrp_clear_parse(struct net_bridge *br, struct nlattr *attr,
+ struct netlink_ext_ack *extack)
+{
+ struct nlattr *tb[IFLA_BRIDGE_MRP_START_TEST_MAX + 1];
+ int err;
+
+ err = nla_parse_nested(tb, IFLA_BRIDGE_MRP_CLEAR_MAX, attr,
+ br_mrp_clear_policy, extack);
+ if (err)
+ return err;
+
+ return br_mrp_clear(br);
+}
+
int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
struct nlattr *attr, int cmd, struct netlink_ext_ack *extack)
{
@@ -301,6 +321,12 @@ int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
return err;
}
+ if (tb[IFLA_BRIDGE_MRP_CLEAR]) {
+ err = br_mrp_clear_parse(br, tb[IFLA_BRIDGE_MRP_CLEAR], extack);
+ if (err)
+ return err;
+ }
+
return 0;
}
diff --git a/net/bridge/br_private_mrp.h b/net/bridge/br_private_mrp.h
index 33b255e38ffec..25c3b8596c25b 100644
--- a/net/bridge/br_private_mrp.h
+++ b/net/bridge/br_private_mrp.h
@@ -36,6 +36,7 @@ struct br_mrp {
/* br_mrp.c */
int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance);
int br_mrp_del(struct net_bridge *br, struct br_mrp_instance *instance);
+int br_mrp_clear(struct net_bridge *br);
int br_mrp_set_port_state(struct net_bridge_port *p,
enum br_mrp_port_state_type state);
int br_mrp_set_port_role(struct net_bridge_port *p,
--
2.26.2
From: Horatiu Vultur <[email protected]>
Date: Fri, 26 Jun 2020 09:33:47 +0200
> This patch series extends MRP netlink interface with IFLA_BRIDGE_MRP_CLEAR.
> To allow the userspace to clear all MRP instances when is started. The
> second patch in the series fix different sparse warnings.
>
> v3:
> - add the second patch to fix sparse warnings
These changes are completely unrelated.
The sparse stuff should probably be submitted to 'net'.
And I have to ask why you really need a clear operation. Routing
daemons come up and see what routes are installed, and update their
internal SW tables to match. This not only allows efficient restart
after a crash, but it also allows multiple daemons to work
cooperatively as an agent for the same forwarding/routing table.
Your usage model limits one daemon to manage the table and that
limitation is completely unnecessary.
Furthermore, even in a one-daemon scenerio, it's wasteful to throw
away all the work the previous daemon did to load the MRP entries into
the bridge.
Thanks.
The 06/26/2020 13:00, David Miller wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> From: Horatiu Vultur <[email protected]>
> Date: Fri, 26 Jun 2020 09:33:47 +0200
>
> > This patch series extends MRP netlink interface with IFLA_BRIDGE_MRP_CLEAR.
> > To allow the userspace to clear all MRP instances when is started. The
> > second patch in the series fix different sparse warnings.
> >
> > v3:
> > - add the second patch to fix sparse warnings
Hi,
>
> These changes are completely unrelated.
>
> The sparse stuff should probably be submitted to 'net'.
I will send a patch for this to 'net'.
>
> And I have to ask why you really need a clear operation.
Because we didn't have any way for the userspace to know what ports are
part of the MRP ring. I thought the easiest way would be for the daemon
to clear everything when is started.
> Routing
> daemons come up and see what routes are installed, and update their
> internal SW tables to match. This not only allows efficient restart
> after a crash, but it also allows multiple daemons to work
> cooperatively as an agent for the same forwarding/routing table.
I think it would be possible to have something similar for the MRP
daemon. But I still have problems to see how to have multiple MRP
daemons running at the same time. Because each deamon implements MRP
state machine. So for example if the link of one of the MRP ports is
changing then each daemon is notified about this change so then each
daemon will send some frames, and that would mean that we have duplicate
frames in the network.
>
> Your usage model limits one daemon to manage the table and that
> limitation is completely unnecessary.
>
> Furthermore, even in a one-daemon scenerio, it's wasteful to throw
> away all the work the previous daemon did to load the MRP entries into
> the bridge.
>
> Thanks.
>
--
/Horatiu