henrik.bjoernlund at microchip.com
2020-Sep-15 09:49 UTC
[Bridge] [PATCH RFC 5/7] bridge: cfm: Netlink Interface.
Thanks for the review. Comments below. The 09/08/2020 13:47, Nikolay Aleksandrov wrote:> > On Fri, 2020-09-04 at 09:15 +0000, Henrik Bjoernlund wrote: > > This is the implementation of CFM netlink configuration > > and status information interface. > > > > Add new nested netlink attributes. These attributes are used by the > > user space to create/delete/configure CFM instances and get status. > > Also they are used by the kernel to notify the user space when changes > > in any status happens. > [snip] > > __u64 transition_fwd; > > diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h > > index 9b814c92de12..fdd408f6a5d2 100644 > > --- a/include/uapi/linux/rtnetlink.h > > +++ b/include/uapi/linux/rtnetlink.h > > @@ -779,6 +779,8 @@ enum { > > #define RTEXT_FILTER_BRVLAN_COMPRESSED (1 << 2) > > #define RTEXT_FILTER_SKIP_STATS (1 << 3) > > #define RTEXT_FILTER_MRP (1 << 4) > > +#define RTEXT_FILTER_CFM_CONFIG (1 << 5) > > +#define RTEXT_FILTER_CFM_STATUS (1 << 6) > > > > /* End of information exported to user level */ > > > > diff --git a/net/bridge/Makefile b/net/bridge/Makefile > > index ddc0a9192348..4702702a74d3 100644 > > --- a/net/bridge/Makefile > > +++ b/net/bridge/Makefile > > @@ -28,4 +28,4 @@ obj-$(CONFIG_NETFILTER) += netfilter/ > > > > bridge-$(CONFIG_BRIDGE_MRP) += br_mrp_switchdev.o br_mrp.o br_mrp_netlink.o > > > > -bridge-$(CONFIG_BRIDGE_CFM) += br_cfm.o > > +bridge-$(CONFIG_BRIDGE_CFM) += br_cfm.o br_cfm_netlink.o > > diff --git a/net/bridge/br_cfm_netlink.c b/net/bridge/br_cfm_netlink.c > > new file mode 100644 > > index 000000000000..4e39aab1cd0b > > --- /dev/null > > +++ b/net/bridge/br_cfm_netlink.c > > @@ -0,0 +1,684 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > + > > +#include <net/genetlink.h> > > + > > +#include "br_private.h" > > +#include "br_private_cfm.h" > > + > > +static inline struct mac_addr nla_get_mac(const struct nlattr *nla) > > +{ > > + struct mac_addr mac; > > + > > + nla_memcpy(&mac.addr, nla, sizeof(mac.addr)); > > + > > + return mac; > > +} > > + > > +static inline struct br_cfm_maid nla_get_maid(const struct nlattr *nla) > > +{ > > + struct br_cfm_maid maid; > > + > > + nla_memcpy(&maid.data, nla, sizeof(maid.data)); > > + > > + return maid; > > +} > > IMO, these 1-line helpers don't really help readability. >I think they make a difference - you can write nicely in one line: config.unicast_mac = nla_get_mac(tb[IFLA_BRIDGE_CFM_MEP_CONFIG_UNICAST_MAC]); Also you can argue the same with functions nla_get_u32() and nla_get_u8 () they are one liner functions. I you think it must be changed I will ofc do it.> > + > > +static inline int nla_put_u64(struct sk_buff *skb, int attrtype, u64 value) > > +{ > > + u64 tmp = value; > > + > > + return nla_put(skb, attrtype, sizeof(u64), &tmp); > > +} > > What?! Read include/net/netlink.h >I have removed this. Not used anyway.> > + > > +static const struct nla_policy > > +br_cfm_policy[IFLA_BRIDGE_CFM_MAX + 1] = { > > + [IFLA_BRIDGE_CFM_UNSPEC] = { .type = NLA_REJECT }, > > + [IFLA_BRIDGE_CFM_MEP_CREATE] = { .type = NLA_NESTED }, > > + [IFLA_BRIDGE_CFM_MEP_DELETE] = { .type = NLA_NESTED }, > > + [IFLA_BRIDGE_CFM_MEP_CONFIG] = { .type = NLA_NESTED }, > > + [IFLA_BRIDGE_CFM_CC_CONFIG] = { .type = NLA_NESTED }, > > + [IFLA_BRIDGE_CFM_CC_PEER_MEP_ADD] = { .type = NLA_NESTED }, > > + [IFLA_BRIDGE_CFM_CC_PEER_MEP_REMOVE] = { .type = NLA_NESTED }, > > + [IFLA_BRIDGE_CFM_CC_RDI] = { .type = NLA_NESTED }, > > + [IFLA_BRIDGE_CFM_CC_CCM_TX] = { .type = NLA_NESTED }, > > +}; > > + > > +static const struct nla_policy > > +br_cfm_mep_create_policy[IFLA_BRIDGE_CFM_MEP_CREATE_MAX + 1] = { > > + [IFLA_BRIDGE_CFM_MEP_CREATE_UNSPEC] = { .type = NLA_REJECT }, > > + [IFLA_BRIDGE_CFM_MEP_CREATE_INSTANCE] = { .type = NLA_U32 }, > > + [IFLA_BRIDGE_CFM_MEP_CREATE_DOMAIN] = { .type = NLA_U32 }, > > + [IFLA_BRIDGE_CFM_MEP_CREATE_DIRECTION] = { .type = NLA_U32 }, > > + [IFLA_BRIDGE_CFM_MEP_CREATE_IFINDEX] = { .type = NLA_U32 }, > > +}; > > + > > +static const struct nla_policy > > +br_cfm_mep_delete_policy[IFLA_BRIDGE_CFM_MEP_DELETE_MAX + 1] = { > > + [IFLA_BRIDGE_CFM_MEP_DELETE_UNSPEC] = { .type = NLA_REJECT }, > > + [IFLA_BRIDGE_CFM_MEP_DELETE_INSTANCE] = { .type = NLA_U32 }, > > +}; > > + > > +static const struct nla_policy > > +br_cfm_mep_config_policy[IFLA_BRIDGE_CFM_MEP_CONFIG_MAX + 1] = { > > + [IFLA_BRIDGE_CFM_MEP_CONFIG_UNSPEC] = { .type = NLA_REJECT }, > > + [IFLA_BRIDGE_CFM_MEP_CONFIG_INSTANCE] = { .type = NLA_U32 }, > > + [IFLA_BRIDGE_CFM_MEP_CONFIG_UNICAST_MAC] = NLA_POLICY_ETH_ADDR, > > + [IFLA_BRIDGE_CFM_MEP_CONFIG_MDLEVEL] = { .type = NLA_U32 }, > > + [IFLA_BRIDGE_CFM_MEP_CONFIG_MEPID] = { .type = NLA_U32 }, > > +}; > > + > > +static const struct nla_policy > > +br_cfm_cc_config_policy[IFLA_BRIDGE_CFM_CC_CONFIG_MAX + 1] = { > > + [IFLA_BRIDGE_CFM_CC_CONFIG_UNSPEC] = { .type = NLA_REJECT }, > > + [IFLA_BRIDGE_CFM_CC_CONFIG_INSTANCE] = { .type = NLA_U32 }, > > + [IFLA_BRIDGE_CFM_CC_CONFIG_ENABLE] = { .type = NLA_U32 }, > > + [IFLA_BRIDGE_CFM_CC_CONFIG_EXP_INTERVAL] = { .type = NLA_U32 }, > > + [IFLA_BRIDGE_CFM_CC_CONFIG_EXP_MAID] = { > > + .type = NLA_BINARY, .len = CFM_MAID_LENGTH }, > > +}; > > + > > +static const struct nla_policy > > +br_cfm_cc_peer_mep_policy[IFLA_BRIDGE_CFM_CC_PEER_MEP_MAX + 1] = { > > + [IFLA_BRIDGE_CFM_CC_PEER_MEP_UNSPEC] = { .type = NLA_REJECT }, > > + [IFLA_BRIDGE_CFM_CC_PEER_MEP_INSTANCE] = { .type = NLA_U32 }, > > + [IFLA_BRIDGE_CFM_CC_PEER_MEPID] = { .type = NLA_U32 }, > > +}; > > + > > +static const struct nla_policy > > +br_cfm_cc_rdi_policy[IFLA_BRIDGE_CFM_CC_RDI_MAX + 1] = { > > + [IFLA_BRIDGE_CFM_CC_RDI_UNSPEC] = { .type = NLA_REJECT }, > > + [IFLA_BRIDGE_CFM_CC_RDI_INSTANCE] = { .type = NLA_U32 }, > > + [IFLA_BRIDGE_CFM_CC_RDI_RDI] = { .type = NLA_U32 }, > > +}; > > + > > +static const struct nla_policy > > +br_cfm_cc_ccm_tx_policy[IFLA_BRIDGE_CFM_CC_CCM_TX_MAX + 1] = { > > + [IFLA_BRIDGE_CFM_CC_CCM_TX_UNSPEC] = { .type = NLA_REJECT }, > > + [IFLA_BRIDGE_CFM_CC_CCM_TX_INSTANCE] = { .type = NLA_U32 }, > > + [IFLA_BRIDGE_CFM_CC_CCM_TX_DMAC] = NLA_POLICY_ETH_ADDR, > > + [IFLA_BRIDGE_CFM_CC_CCM_TX_SEQ_NO_UPDATE] = { .type = NLA_U32 }, > > + [IFLA_BRIDGE_CFM_CC_CCM_TX_PERIOD] = { .type = NLA_U32 }, > > + [IFLA_BRIDGE_CFM_CC_CCM_TX_IF_TLV] = { .type = NLA_U32 }, > > + [IFLA_BRIDGE_CFM_CC_CCM_TX_IF_TLV_VALUE] = { .type = NLA_U8 }, > > + [IFLA_BRIDGE_CFM_CC_CCM_TX_PORT_TLV] = { .type = NLA_U32 }, > > + [IFLA_BRIDGE_CFM_CC_CCM_TX_PORT_TLV_VALUE] = { .type = NLA_U8 }, > > +}; > > + > > +static int br_mep_create_parse(struct net_bridge *br, struct nlattr *attr, > > + struct netlink_ext_ack *extack) > > +{ > > + struct nlattr *tb[IFLA_BRIDGE_CFM_MEP_CREATE_MAX + 1]; > > + struct br_cfm_mep_create create; > > + u32 instance; > > + int err; > > + > > + err = nla_parse_nested(tb, IFLA_BRIDGE_CFM_MEP_CREATE_MAX, attr, > > + br_cfm_mep_create_policy, extack); > > + if (err) > > + return err; > > + > > + if (!tb[IFLA_BRIDGE_CFM_MEP_CREATE_INSTANCE] || > > + !tb[IFLA_BRIDGE_CFM_MEP_CREATE_DOMAIN] || > > + !tb[IFLA_BRIDGE_CFM_MEP_CREATE_DIRECTION] || > > + !tb[IFLA_BRIDGE_CFM_MEP_CREATE_IFINDEX]) { > > + NL_SET_ERR_MSG_MOD(extack, > > + "Missing attribute: INSTANCE or DOMAIN or DIRECTION or VID or IFINDEX"); > > Break these into separate errors. > > > + return -EINVAL; > > + } > > + > > + memset(&create, 0, sizeof(create)); > > + > > + instance = nla_get_u32(tb[IFLA_BRIDGE_CFM_MEP_CREATE_INSTANCE]); > > + create.domain = nla_get_u32(tb[IFLA_BRIDGE_CFM_MEP_CREATE_DOMAIN]); > > + create.direction = nla_get_u32(tb[IFLA_BRIDGE_CFM_MEP_CREATE_DIRECTION]); > > + create.ifindex = nla_get_u32(tb[IFLA_BRIDGE_CFM_MEP_CREATE_IFINDEX]); > > + > > + return br_cfm_mep_create(br, instance, &create, extack); > > +} > > + > > +static int br_mep_delete_parse(struct net_bridge *br, struct nlattr *attr, > > + struct netlink_ext_ack *extack) > > +{ > > + struct nlattr *tb[IFLA_BRIDGE_CFM_MEP_DELETE_MAX + 1]; > > + u32 instance; > > + int err; > > + > > + err = nla_parse_nested(tb, IFLA_BRIDGE_CFM_MEP_DELETE_MAX, attr, > > + br_cfm_mep_delete_policy, extack); > > + if (err) > > + return err; > > + > > + if (!tb[IFLA_BRIDGE_CFM_MEP_CREATE_INSTANCE]) { > > + NL_SET_ERR_MSG_MOD(extack, > > + "Missing attribute: INSTANCE"); > > "Missing instance attribute". Same for all similar messages. >I will change that as suggested.> > + return -EINVAL; > > + } > > + > > + instance = nla_get_u32(tb[IFLA_BRIDGE_CFM_MEP_CREATE_INSTANCE]); > > + > > + return br_cfm_mep_delete(br, instance, extack); > > +} > > + > > +static int br_mep_config_parse(struct net_bridge *br, struct nlattr *attr, > > + struct netlink_ext_ack *extack) > > +{ > > + struct nlattr *tb[IFLA_BRIDGE_CFM_MEP_CONFIG_MAX + 1]; > > + struct br_cfm_mep_config config; > > + u32 instance; > > + int err; > > + > > + err = nla_parse_nested(tb, IFLA_BRIDGE_CFM_MEP_CONFIG_MAX, attr, > > + br_cfm_mep_config_policy, extack); > > + if (err) > > + return err; > > + > > + if (!tb[IFLA_BRIDGE_CFM_MEP_CONFIG_INSTANCE] || > > + !tb[IFLA_BRIDGE_CFM_MEP_CONFIG_UNICAST_MAC] || > > + !tb[IFLA_BRIDGE_CFM_MEP_CONFIG_MDLEVEL] || > > + !tb[IFLA_BRIDGE_CFM_MEP_CONFIG_MEPID]) { > > + NL_SET_ERR_MSG_MOD(extack, > > + "Missing attribute: INSTANCE or UNICAST_MAC or MDLEVEL or MEPID or VID"); > > Break these into separate errors. >I will change that as suggested.> > + return -EINVAL; > > + } > > + > > + memset(&config, 0, sizeof(config)); > > + > > + instance = nla_get_u32(tb[IFLA_BRIDGE_CFM_MEP_CONFIG_INSTANCE]); > > + config.unicast_mac = nla_get_mac(tb[IFLA_BRIDGE_CFM_MEP_CONFIG_UNICAST_MAC]); > > + config.mdlevel = nla_get_u32(tb[IFLA_BRIDGE_CFM_MEP_CONFIG_MDLEVEL]); > > + config.mepid = nla_get_u32(tb[IFLA_BRIDGE_CFM_MEP_CONFIG_MEPID]); > > + config.vid = 0; > > + > > + return br_cfm_mep_config_set(br, instance, &config, extack); > > +} > > + > > +static int br_cc_config_parse(struct net_bridge *br, struct nlattr *attr, > > + struct netlink_ext_ack *extack) > > +{ > > + struct nlattr *tb[IFLA_BRIDGE_CFM_CC_CONFIG_MAX + 1]; > > + struct br_cfm_cc_config config; > > + u32 instance; > > + int err; > > + > > + err = nla_parse_nested(tb, IFLA_BRIDGE_CFM_CC_CONFIG_MAX, attr, > > + br_cfm_cc_config_policy, extack); > > + if (err) > > + return err; > > + > > + if (!tb[IFLA_BRIDGE_CFM_CC_CONFIG_INSTANCE] || > > + !tb[IFLA_BRIDGE_CFM_CC_CONFIG_ENABLE] || > > + !tb[IFLA_BRIDGE_CFM_CC_CONFIG_EXP_INTERVAL] || > > + !tb[IFLA_BRIDGE_CFM_CC_CONFIG_EXP_MAID]) { > > + NL_SET_ERR_MSG_MOD(extack, > > + "Missing attribute: INSTANCE or ENABLE or INTERVAL or PRIORITY or MAID"); > > Break these into separate errors. >I will change that as suggested.> > + return -EINVAL; > > + } > > + > > + memset(&config, 0, sizeof(config)); > > + > > + instance = nla_get_u32(tb[IFLA_BRIDGE_CFM_CC_CONFIG_INSTANCE]); > > + config.enable = nla_get_u32(tb[IFLA_BRIDGE_CFM_CC_CONFIG_ENABLE]); > > + config.exp_interval = nla_get_u32(tb[IFLA_BRIDGE_CFM_CC_CONFIG_EXP_INTERVAL]); > > + config.exp_priority = 0; > > + config.exp_maid = nla_get_maid(tb[IFLA_BRIDGE_CFM_CC_CONFIG_EXP_MAID]); > > + > > + return br_cfm_cc_config_set(br, instance, &config, extack); > > +} > > + > > +static int br_cc_peer_mep_add_parse(struct net_bridge *br, struct nlattr *attr, > > + struct netlink_ext_ack *extack) > > +{ > > + struct nlattr *tb[IFLA_BRIDGE_CFM_CC_PEER_MEP_MAX + 1]; > > + u32 instance, peer_mep_id; > > + int err; > > + > > + err = nla_parse_nested(tb, IFLA_BRIDGE_CFM_CC_PEER_MEP_MAX, attr, > > + br_cfm_cc_peer_mep_policy, extack); > > + if (err) > > + return err; > > + > > + if (!tb[IFLA_BRIDGE_CFM_CC_PEER_MEP_INSTANCE] || > > + !tb[IFLA_BRIDGE_CFM_CC_PEER_MEPID]) { > > + NL_SET_ERR_MSG_MOD(extack, > > + "Missing attribute: INSTANCE or PEER_MEP_ID"); > > Break these into separate errors. >I will change that as suggested.> > + return -EINVAL; > > + } > > + > > + instance = nla_get_u32(tb[IFLA_BRIDGE_CFM_CC_PEER_MEP_INSTANCE]); > > + peer_mep_id = nla_get_u32(tb[IFLA_BRIDGE_CFM_CC_PEER_MEPID]); > > + > > + return br_cfm_cc_peer_mep_add(br, instance, peer_mep_id, extack); > > +} > > + > > +static int br_cc_peer_mep_remove_parse(struct net_bridge *br, struct nlattr *attr, > > + struct netlink_ext_ack *extack) > > +{ > > + struct nlattr *tb[IFLA_BRIDGE_CFM_CC_PEER_MEP_MAX + 1]; > > + u32 instance, peer_mep_id; > > + int err; > > + > > + err = nla_parse_nested(tb, IFLA_BRIDGE_CFM_CC_PEER_MEP_MAX, attr, > > + br_cfm_cc_peer_mep_policy, extack); > > + if (err) > > + return err; > > + > > + if (!tb[IFLA_BRIDGE_CFM_CC_PEER_MEP_INSTANCE] || > > + !tb[IFLA_BRIDGE_CFM_CC_PEER_MEPID]) { > > + NL_SET_ERR_MSG_MOD(extack, > > + "Missing attribute: INSTANCE or PEER_MEP_ID"); > > Break these into separate errors. >I will change that as suggested.> > + return -EINVAL; > > + } > > + > > + instance = nla_get_u32(tb[IFLA_BRIDGE_CFM_CC_PEER_MEP_INSTANCE]); > > + peer_mep_id = nla_get_u32(tb[IFLA_BRIDGE_CFM_CC_PEER_MEPID]); > > + > > + return br_cfm_cc_peer_mep_remove(br, instance, peer_mep_id, extack); > > +} > > + > > +static int br_cc_rdi_parse(struct net_bridge *br, struct nlattr *attr, > > + struct netlink_ext_ack *extack) > > +{ > > + struct nlattr *tb[IFLA_BRIDGE_CFM_CC_RDI_MAX + 1]; > > + u32 instance, rdi; > > + int err; > > + > > + err = nla_parse_nested(tb, IFLA_BRIDGE_CFM_CC_RDI_MAX, attr, > > + br_cfm_cc_rdi_policy, extack); > > + if (err) > > + return err; > > + > > + if (!tb[IFLA_BRIDGE_CFM_CC_RDI_INSTANCE] || > > + !tb[IFLA_BRIDGE_CFM_CC_RDI_RDI]) { > > + NL_SET_ERR_MSG_MOD(extack, > > + "Missing attribute: INSTANCE or RDI"); > > Break these into separate errors. >I will change that as suggested.> > + return -EINVAL; > > + } > > + > > + instance = nla_get_u32(tb[IFLA_BRIDGE_CFM_CC_RDI_INSTANCE]); > > + rdi = nla_get_u32(tb[IFLA_BRIDGE_CFM_CC_RDI_RDI]); > > + > > + return br_cfm_cc_rdi_set(br, instance, rdi, extack); > > +} > > + > > +static int br_cc_ccm_tx_parse(struct net_bridge *br, struct nlattr *attr, > > + struct netlink_ext_ack *extack) > > +{ > > + struct nlattr *tb[IFLA_BRIDGE_CFM_CC_CCM_TX_MAX + 1]; > > + u32 instance; > > + struct br_cfm_cc_ccm_tx_info tx_info; > > + int err; > > + > > + err = nla_parse_nested(tb, IFLA_BRIDGE_CFM_CC_CCM_TX_MAX, attr, > > + br_cfm_cc_ccm_tx_policy, extack); > > + if (err) > > + return err; > > + > > + if (!tb[IFLA_BRIDGE_CFM_CC_CCM_TX_INSTANCE] || > > + !tb[IFLA_BRIDGE_CFM_CC_CCM_TX_DMAC] || > > + !tb[IFLA_BRIDGE_CFM_CC_CCM_TX_SEQ_NO_UPDATE] || > > + !tb[IFLA_BRIDGE_CFM_CC_CCM_TX_PERIOD] || > > + !tb[IFLA_BRIDGE_CFM_CC_CCM_TX_IF_TLV] || > > + !tb[IFLA_BRIDGE_CFM_CC_CCM_TX_IF_TLV_VALUE] || > > + !tb[IFLA_BRIDGE_CFM_CC_CCM_TX_PORT_TLV] || > > + !tb[IFLA_BRIDGE_CFM_CC_CCM_TX_PORT_TLV_VALUE]) { > > + NL_SET_ERR_MSG_MOD(extack, > > + "Missing attribute: INSTANCE or PRIORITY or DEI or DMAC or SEQ_NO_UPDATE or PERIOD or IF_TLV or IF_TLV_VALUE or PORT_TLV or PORT_TLV_VALUE"); > > Break these into separate errors. >I will change that as suggested.> > + return -EINVAL; > > + } > > + > > + instance = nla_get_u32(tb[IFLA_BRIDGE_CFM_CC_RDI_INSTANCE]); > > + tx_info.priority = 0; > > + tx_info.dei = 0; > > + tx_info.dmac = nla_get_mac(tb[IFLA_BRIDGE_CFM_CC_CCM_TX_DMAC]); > > + tx_info.seq_no_update = nla_get_u32(tb[IFLA_BRIDGE_CFM_CC_CCM_TX_SEQ_NO_UPDATE]); > > + tx_info.period = nla_get_u32(tb[IFLA_BRIDGE_CFM_CC_CCM_TX_PERIOD]); > > + tx_info.if_tlv = nla_get_u32(tb[IFLA_BRIDGE_CFM_CC_CCM_TX_IF_TLV]); > > + tx_info.if_tlv_value = nla_get_u8(tb[IFLA_BRIDGE_CFM_CC_CCM_TX_IF_TLV_VALUE]); > > + tx_info.port_tlv = nla_get_u32(tb[IFLA_BRIDGE_CFM_CC_CCM_TX_PORT_TLV]); > > + tx_info.port_tlv_value = nla_get_u8(tb[IFLA_BRIDGE_CFM_CC_CCM_TX_PORT_TLV_VALUE]); > > + > > + return br_cfm_cc_ccm_tx(br, instance, &tx_info, extack); > > +} > > + > > +int br_cfm_parse(struct net_bridge *br, struct net_bridge_port *p, > > + struct nlattr *attr, int cmd, struct netlink_ext_ack *extack) > > +{ > > + struct nlattr *tb[IFLA_BRIDGE_CFM_MAX + 1]; > > + int err; > > + > > + /* When this function is called for a port then the br pointer is > > + * invalid, therefor set the br to point correctly > > + */ > > + if (p) > > + br = p->br; > > + > > + err = nla_parse_nested(tb, IFLA_BRIDGE_CFM_MAX, attr, > > + br_cfm_policy, extack); > > + if (err) > > + return err; > > + > > + if (tb[IFLA_BRIDGE_CFM_MEP_CREATE]) { > > + err = br_mep_create_parse(br, tb[IFLA_BRIDGE_CFM_MEP_CREATE], > > + extack); > > + if (err) > > + return err; > > + } > > + > > + if (tb[IFLA_BRIDGE_CFM_MEP_DELETE]) { > > + err = br_mep_delete_parse(br, tb[IFLA_BRIDGE_CFM_MEP_DELETE], > > + extack); > > + if (err) > > + return err; > > + } > > + > > + if (tb[IFLA_BRIDGE_CFM_MEP_CONFIG]) { > > + err = br_mep_config_parse(br, tb[IFLA_BRIDGE_CFM_MEP_CONFIG], > > + extack); > > + if (err) > > + return err; > > + } > > + > > + if (tb[IFLA_BRIDGE_CFM_CC_CONFIG]) { > > + err = br_cc_config_parse(br, tb[IFLA_BRIDGE_CFM_CC_CONFIG], > > + extack); > > + if (err) > > + return err; > > + } > > + > > + if (tb[IFLA_BRIDGE_CFM_CC_PEER_MEP_ADD]) { > > + err = br_cc_peer_mep_add_parse(br, tb[IFLA_BRIDGE_CFM_CC_PEER_MEP_ADD], > > + extack); > > + if (err) > > + return err; > > + } > > + > > + if (tb[IFLA_BRIDGE_CFM_CC_PEER_MEP_REMOVE]) { > > + err = br_cc_peer_mep_remove_parse(br, tb[IFLA_BRIDGE_CFM_CC_PEER_MEP_REMOVE], > > + extack); > > + if (err) > > + return err; > > + } > > + > > + if (tb[IFLA_BRIDGE_CFM_CC_RDI]) { > > + err = br_cc_rdi_parse(br, tb[IFLA_BRIDGE_CFM_CC_RDI], > > + extack); > > + if (err) > > + return err; > > + } > > + > > + if (tb[IFLA_BRIDGE_CFM_CC_CCM_TX]) { > > + err = br_cc_ccm_tx_parse(br, tb[IFLA_BRIDGE_CFM_CC_CCM_TX], > > + extack); > > + if (err) > > + return err; > > + } > > + > > + return 0; > > +} > > + > > +int br_cfm_config_fill_info(struct sk_buff *skb, struct net_bridge *br) > > +{ > > + struct nlattr *tb, *cfm_tb; > > + struct br_cfm_mep *mep; > > + struct br_cfm_peer_mep *peer_mep; > > + > > + cfm_tb = nla_nest_start_noflag(skb, IFLA_BRIDGE_CFM); > > Why _noflag everywhere? >I will change that to nla_nest_start().> > + if (!cfm_tb) > > + return -EMSGSIZE; > > + > > + list_for_each_entry_rcu(mep, &br->mep_list, head) { > > + tb = nla_nest_start_noflag(skb, IFLA_BRIDGE_CFM_MEP_CREATE_INFO); > > + if (!tb) > > + goto nla_info_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_MEP_CREATE_INSTANCE, > > + mep->instance)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_MEP_CREATE_DOMAIN, > > + mep->create.domain)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_MEP_CREATE_DIRECTION, > > + mep->create.direction)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_MEP_CREATE_IFINDEX, > > + mep->create.ifindex)) > > + goto nla_put_failure; > > + > > + nla_nest_end(skb, tb); > > + > > + tb = nla_nest_start_noflag(skb, IFLA_BRIDGE_CFM_MEP_CONFIG_INFO); > > + > > + if (!tb) > > + goto nla_info_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_MEP_CONFIG_INSTANCE, > > + mep->instance)) > > + goto nla_put_failure; > > + > > + if (nla_put(skb, IFLA_BRIDGE_CFM_MEP_CONFIG_UNICAST_MAC, > > + sizeof(mep->config.unicast_mac.addr), > > + mep->config.unicast_mac.addr)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_MEP_CONFIG_MDLEVEL, > > + mep->config.mdlevel)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_MEP_CONFIG_MEPID, > > + mep->config.mepid)) > > + goto nla_put_failure; > > + > > + nla_nest_end(skb, tb); > > + > > + tb = nla_nest_start_noflag(skb, IFLA_BRIDGE_CFM_CC_CONFIG_INFO); > > + > > + if (!tb) > > + goto nla_info_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_CONFIG_INSTANCE, > > + mep->instance)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_CONFIG_ENABLE, > > + mep->cc_config.enable)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_CONFIG_EXP_INTERVAL, > > + mep->cc_config.exp_interval)) > > + goto nla_put_failure; > > + > > + if (nla_put(skb, IFLA_BRIDGE_CFM_CC_CONFIG_EXP_MAID, > > + sizeof(mep->cc_config.exp_maid.data), > > + mep->cc_config.exp_maid.data)) > > + goto nla_put_failure; > > + > > + nla_nest_end(skb, tb); > > + > > + tb = nla_nest_start_noflag(skb, IFLA_BRIDGE_CFM_CC_RDI_INFO); > > + > > + if (!tb) > > + goto nla_info_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_RDI_INSTANCE, > > + mep->instance)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_RDI_RDI, > > + mep->rdi)) > > + goto nla_put_failure; > > + > > + nla_nest_end(skb, tb); > > + > > + tb = nla_nest_start_noflag(skb, IFLA_BRIDGE_CFM_CC_CCM_TX_INFO); > > + > > + if (!tb) > > + goto nla_info_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_CCM_TX_INSTANCE, > > + mep->instance)) > > + goto nla_put_failure; > > + > > + if (nla_put(skb, IFLA_BRIDGE_CFM_CC_CCM_TX_DMAC, > > + sizeof(mep->cc_ccm_tx_info.dmac), > > + mep->cc_ccm_tx_info.dmac.addr)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_CCM_TX_SEQ_NO_UPDATE, > > + mep->cc_ccm_tx_info.seq_no_update)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_CCM_TX_PERIOD, > > + mep->cc_ccm_tx_info.period)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_CCM_TX_IF_TLV, > > + mep->cc_ccm_tx_info.if_tlv)) > > + goto nla_put_failure; > > + > > + if (nla_put_u8(skb, IFLA_BRIDGE_CFM_CC_CCM_TX_IF_TLV_VALUE, > > + mep->cc_ccm_tx_info.if_tlv_value)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_CCM_TX_PORT_TLV, > > + mep->cc_ccm_tx_info.port_tlv)) > > + goto nla_put_failure; > > + > > + if (nla_put_u8(skb, IFLA_BRIDGE_CFM_CC_CCM_TX_PORT_TLV_VALUE, > > + mep->cc_ccm_tx_info.port_tlv_value)) > > + goto nla_put_failure; > > + > > + nla_nest_end(skb, tb); > > + > > + list_for_each_entry_rcu(peer_mep, &mep->peer_mep_list, head) { > > + tb = nla_nest_start_noflag(skb, IFLA_BRIDGE_CFM_CC_PEER_MEP_INFO); > > + > > + if (!tb) > > + goto nla_info_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_PEER_MEP_INSTANCE, > > + mep->instance)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_PEER_MEPID, > > + peer_mep->mepid)) > > + goto nla_put_failure; > > + > > + nla_nest_end(skb, tb); > > + } > > + } > > + nla_nest_end(skb, cfm_tb); > > + > > + return 0; > > + > > +nla_put_failure: > > + nla_nest_cancel(skb, tb); > > + > > +nla_info_failure: > > + nla_nest_cancel(skb, cfm_tb); > > + > > + return -EMSGSIZE; > > +} > > + > > +int br_cfm_status_fill_info(struct sk_buff *skb, struct net_bridge *br) > > +{ > > + struct nlattr *tb, *cfm_tb; > > + struct br_cfm_mep *mep; > > + struct br_cfm_peer_mep *peer_mep; > > + > > + cfm_tb = nla_nest_start_noflag(skb, IFLA_BRIDGE_CFM); > > + if (!cfm_tb) > > + return -EMSGSIZE; > > + > > + list_for_each_entry_rcu(mep, &br->mep_list, head) { > > + tb = nla_nest_start_noflag(skb, IFLA_BRIDGE_CFM_MEP_STATUS_INFO); > > + if (!tb) > > + goto nla_info_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_MEP_STATUS_INSTANCE, > > + mep->instance)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_MEP_STATUS_OPCODE_UNEXP_SEEN, > > + mep->status.opcode_unexp_seen)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_MEP_STATUS_VERSION_UNEXP_SEEN, > > + mep->status.version_unexp_seen)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_MEP_STATUS_RX_LEVEL_LOW_SEEN, > > + mep->status.rx_level_low_seen)) > > + goto nla_put_failure; > > + > > + /* Clear all 'seen' indications */ > > + mep->status.opcode_unexp_seen = false; > > + mep->status.version_unexp_seen = false; > > + mep->status.rx_level_low_seen = false; > > + > > + nla_nest_end(skb, tb); > > + > > + list_for_each_entry_rcu(peer_mep, &mep->peer_mep_list, head) { > > + tb = nla_nest_start_noflag(skb, IFLA_BRIDGE_CFM_CC_PEER_STATUS_INFO); > > + > > + if (!tb) > > + goto nla_info_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_PEER_STATUS_INSTANCE, > > + mep->instance)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_PEER_STATUS_PEER_MEPID, > > + peer_mep->mepid)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_PEER_STATUS_CCM_DEFECT, > > + peer_mep->cc_status.ccm_defect)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_PEER_STATUS_RDI, > > + peer_mep->cc_status.rdi)) > > + goto nla_put_failure; > > + > > + if (nla_put_u8(skb, IFLA_BRIDGE_CFM_CC_PEER_STATUS_PORT_TLV_VALUE, > > + peer_mep->cc_status.port_tlv_value)) > > + goto nla_put_failure; > > + > > + if (nla_put_u8(skb, IFLA_BRIDGE_CFM_CC_PEER_STATUS_IF_TLV_VALUE, > > + peer_mep->cc_status.if_tlv_value)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_PEER_STATUS_SEEN, > > + peer_mep->cc_status.seen)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_PEER_STATUS_TLV_SEEN, > > + peer_mep->cc_status.tlv_seen)) > > + goto nla_put_failure; > > + > > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_PEER_STATUS_SEQ_UNEXP_SEEN, > > + peer_mep->cc_status.seq_unexp_seen)) > > + goto nla_put_failure; > > + > > + /* Clear all 'seen' indications */ > > + peer_mep->cc_status.seen = false; > > + peer_mep->cc_status.tlv_seen = false; > > + peer_mep->cc_status.seq_unexp_seen = false; > > + > > + nla_nest_end(skb, tb); > > + } > > + } > > + nla_nest_end(skb, cfm_tb); > > + > > + return 0; > > + > > +nla_put_failure: > > + nla_nest_cancel(skb, tb); > > + > > +nla_info_failure: > > + nla_nest_cancel(skb, cfm_tb); > > + > > + return -EMSGSIZE; > > +} > > diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c > > index 8a71c60fa357..6de5cb1295f6 100644 > > --- a/net/bridge/br_netlink.c > > +++ b/net/bridge/br_netlink.c > > @@ -16,6 +16,7 @@ > > > > #include "br_private.h" > > #include "br_private_stp.h" > > +#include "br_private_cfm.h" > > #include "br_private_tunnel.h" > > > > static int __get_num_vlan_infos(struct net_bridge_vlan_group *vg, > > @@ -481,6 +482,48 @@ static int br_fill_ifinfo(struct sk_buff *skb, > > nla_nest_end(skb, af); > > } > > > > + if (filter_mask & RTEXT_FILTER_CFM_CONFIG) { > > + struct nlattr *af; > > + int err; > > + > > + if (!br_cfm_created(br) || port) > > + goto done; > > + > > + af = nla_nest_start_noflag(skb, IFLA_AF_SPEC); > > + if (!af) > > + goto nla_put_failure; > > + > > + rcu_read_lock(); > > + err = br_cfm_config_fill_info(skb, br); > > + rcu_read_unlock(); > > + > > + if (err) > > + goto nla_put_failure; > > + > > + nla_nest_end(skb, af); > > + } > > + > > + if (filter_mask & RTEXT_FILTER_CFM_STATUS) { > > + struct nlattr *af; > > + int err; > > + > > + if (!br_cfm_created(br) || port) > > + goto done; > > + > > + af = nla_nest_start_noflag(skb, IFLA_AF_SPEC); > > + if (!af) > > + goto nla_put_failure; > > + > > + rcu_read_lock(); > > + err = br_cfm_status_fill_info(skb, br); > > + rcu_read_unlock(); > > + > > + if (err) > > + goto nla_put_failure; > > + > > + nla_nest_end(skb, af); > > + } > > I actually noticed this just now, you can't have multiple IFLA_AF_SPEC > attributes. It seems we already have that problem with MRP. > Since filter_mask is a mask one could request multiple rtext attributes. >I will change to assure that only one IFLA_AF_SPEC is inserted for all possible flags set in filter_mask. I will also assure that only one IFLA_BRIDGE_CFM is inserted if both RTEXT_FILTER_CFM_CONFIG and RTEXT_FILTER_CFM_STATUS is set in filter_mask.> > + > > done: > > nlmsg_end(skb, nlh); > > return 0; > > @@ -542,7 +585,9 @@ int br_getlink(struct sk_buff *skb, u32 pid, u32 seq, > > > > if (!port && !(filter_mask & RTEXT_FILTER_BRVLAN) && > > !(filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED) && > > - !(filter_mask & RTEXT_FILTER_MRP)) > > + !(filter_mask & RTEXT_FILTER_MRP) && > > + !(filter_mask & RTEXT_FILTER_CFM_CONFIG) && > > + !(filter_mask & RTEXT_FILTER_CFM_STATUS)) > > return 0; > > > > return br_fill_ifinfo(skb, port, pid, seq, RTM_NEWLINK, nlflags, > > @@ -704,6 +749,11 @@ static int br_afspec(struct net_bridge *br, > > if (err) > > return err; > > break; > > + case IFLA_BRIDGE_CFM: > > + err = br_cfm_parse(br, p, attr, cmd, extack); > > + if (err) > > + return err; > > + break; > > } > > } > > > > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > > index 385a6a6aa17e..fe36592f7525 100644 > > --- a/net/bridge/br_private.h > > +++ b/net/bridge/br_private.h > > @@ -1365,9 +1365,20 @@ static inline int br_mrp_fill_info(struct sk_buff *skb, struct net_bridge *br) > > > > /* br_cfm.c */ > > #if IS_ENABLED(CONFIG_BRIDGE_CFM) > > +int br_cfm_parse(struct net_bridge *br, struct net_bridge_port *p, > > + struct nlattr *attr, int cmd, struct netlink_ext_ack *extack); > > int br_cfm_rx_frame_process(struct net_bridge_port *p, struct sk_buff *skb); > > bool br_cfm_created(struct net_bridge *br); > > +int br_cfm_config_fill_info(struct sk_buff *skb, struct net_bridge *br); > > +int br_cfm_status_fill_info(struct sk_buff *skb, struct net_bridge *br); > > #else > > +static inline int br_cfm_parse(struct net_bridge *br, struct net_bridge_port *p, > > + struct nlattr *attr, int cmd, > > + struct netlink_ext_ack *extack) > > +{ > > + return -EOPNOTSUPP; > > +} > > + > > static inline int br_cfm_rx_frame_process(struct net_bridge_port *p, struct sk_buff *skb) > > { > > return -EOPNOTSUPP; > > @@ -1377,6 +1388,16 @@ static inline bool br_cfm_created(struct net_bridge *br) > > { > > return false; > > } > > + > > +static inline int br_cfm_config_fill_info(struct sk_buff *skb, struct net_bridge *br) > > +{ > > + return -EOPNOTSUPP; > > +} > > + > > +static inline int br_cfm_status_fill_info(struct sk_buff *skb, struct net_bridge *br) > > +{ > > + return -EOPNOTSUPP; > > +} > > #endif > > > > /* br_netlink.c */ >-- /Henrik