On 24/06/2020 11:05, Horatiu Vultur wrote:
> 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 <horatiu.vultur@xxxxxxxxxxxxx>
> ---
> 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,
> +};
Out of curiousity - do you plan to have any clean attributes?
In case you don't this can simply be a flag that clears the MRP instances instead
of a nested attribute.
> +
> +#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 24986ec7d38cc..02d102edaaaad 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;
> +
> + list_for_each_entry_rcu(mrp, &br->mrp_list, list,
> + lockdep_rtnl_is_held()) {
> + br_mrp_del_impl(br, mrp);
I don't think you're in RCU-protected region here, as the lockdep above confirms, which
means br_mrp_del_impl() can free mrp and you can access freed memory while walking the list
(trying to access of the next element).
You should be using list_for_each_entry_safe() to delete elements while walking the list.
Cheers,
Nik
> + }
> +
> + 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,
>
