| Message ID | 20230215192554.3126010-2-zahari.doychev@linux.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | flower add cfm support | expand |
From: Zahari Doychev <zahari.doychev@linux.com> Date: Wed, 15 Feb 2023 20:25:53 +0100 > From: Zahari Doychev <zdoychev@maxlinear.com> > > Add support to the tc flower classifier to match based on fields in CFM > information elements like level and opcode. > > tc filter add dev ens6 ingress protocol 802.1q \ > flower vlan_id 698 vlan_ethtype 0x8902 cfm mdl 5 op 46 \ > action drop > > Signed-off-by: Zahari Doychev <zdoychev@maxlinear.com> > --- > include/net/flow_dissector.h | 11 ++++ > include/uapi/linux/pkt_cls.h | 12 ++++ > net/core/flow_dissector.c | 41 ++++++++++++ > net/sched/cls_flower.c | 118 ++++++++++++++++++++++++++++++++++- > 4 files changed, 181 insertions(+), 1 deletion(-) > > diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h > index 5ccf52ef8809..a70497f96bed 100644 > --- a/include/net/flow_dissector.h > +++ b/include/net/flow_dissector.h > @@ -297,6 +297,16 @@ struct flow_dissector_key_l2tpv3 { > __be32 session_id; > }; > > +/** > + * struct flow_dissector_key_cfm > + * > + */ ??? Without a proper kernel-doc, this makes no sense. So either remove this comment or make a kernel-doc from it, describing the structure and each its member (I'd go for kernel-doc :P). > +struct flow_dissector_key_cfm { > + u8 mdl:3, > + ver:5; > + u8 opcode; > +}; > + > enum flow_dissector_key_id { > FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */ > FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */ > @@ -329,6 +339,7 @@ enum flow_dissector_key_id { > FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct flow_dissector_key_num_of_vlans */ > FLOW_DISSECTOR_KEY_PPPOE, /* struct flow_dissector_key_pppoe */ > FLOW_DISSECTOR_KEY_L2TPV3, /* struct flow_dissector_key_l2tpv3 */ > + FLOW_DISSECTOR_KEY_CFM, /* struct flow_dissector_key_cfm */ > > FLOW_DISSECTOR_KEY_MAX, > }; > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h > index 648a82f32666..d55f70ccfe3c 100644 > --- a/include/uapi/linux/pkt_cls.h > +++ b/include/uapi/linux/pkt_cls.h > @@ -594,6 +594,8 @@ enum { > > TCA_FLOWER_KEY_L2TPV3_SID, /* be32 */ > > + TCA_FLOWER_KEY_CFM, Each existing definitions within this enum have a comment mentioning the corresponding type (__be32, __u8 and so on), why doesn't this one? > + > __TCA_FLOWER_MAX, > }; > > @@ -702,6 +704,16 @@ enum { > TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST = (1 << 1), > }; > > +enum { > + TCA_FLOWER_KEY_CFM_OPT_UNSPEC, > + TCA_FLOWER_KEY_CFM_MD_LEVEL, > + TCA_FLOWER_KEY_CFM_OPCODE, > + __TCA_FLOWER_KEY_CFM_OPT_MAX, > +}; > + > +#define TCA_FLOWER_KEY_CFM_OPT_MAX \ > + (__TCA_FLOWER_KEY_CFM_OPT_MAX - 1) This fits into one line... Can't we put it into the enum itself? > + > #define TCA_FLOWER_MASK_FLAGS_RANGE (1 << 0) /* Range-based match */ > > /* Match-all classifier */ > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c > index 25fb0bbc310f..adb23d31f199 100644 > --- a/net/core/flow_dissector.c > +++ b/net/core/flow_dissector.c > @@ -547,6 +547,41 @@ __skb_flow_dissect_arp(const struct sk_buff *skb, > return FLOW_DISSECT_RET_OUT_GOOD; > } > > +static enum flow_dissect_ret > +__skb_flow_dissect_cfm(const struct sk_buff *skb, > + struct flow_dissector *flow_dissector, > + void *target_container, const void *data, > + int nhoff, int hlen) > +{ > + struct flow_dissector_key_cfm *key_cfm; > + struct cfm_common_hdr { I don't see this type used anywhere else in the code, so you can leave it anonymous. > + __u8 mdlevel_version; > + __u8 opcode; > + __u8 flags; > + __u8 tlv_offset; This is a purely-kernel-side structure, so use simply `u8` here for each of them. > + } *hdr, _hdr; > +#define CFM_MD_LEVEL_SHIFT 5 > +#define CFM_MD_VERSION_MASK 0x1f > + > + if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_CFM)) > + return FLOW_DISSECT_RET_OUT_GOOD; > + > + hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, > + hlen, &_hdr); > + if (!hdr) > + return FLOW_DISSECT_RET_OUT_BAD; > + > + key_cfm = skb_flow_dissector_target(flow_dissector, > + FLOW_DISSECTOR_KEY_CFM, > + target_container); > + > + key_cfm->mdl = hdr->mdlevel_version >> CFM_MD_LEVEL_SHIFT; > + key_cfm->ver = hdr->mdlevel_version & CFM_MD_VERSION_MASK; I'd highly recommend using FIELD_GET() here. Or wait, why can't you just use one structure for both FD and the actual header? You only need two fields going next to each other, so you could save some cycles by just directly assigning them (I mean, just define the fields you need, not the whole header since you use only first two fields). > + key_cfm->opcode = hdr->opcode; > + > + return FLOW_DISSECT_RET_OUT_GOOD; > +} > + > static enum flow_dissect_ret > __skb_flow_dissect_gre(const struct sk_buff *skb, > struct flow_dissector_key_control *key_control, > @@ -1390,6 +1425,12 @@ bool __skb_flow_dissect(const struct net *net, > break; > } > > + case htons(ETH_P_CFM): { > + fdret = __skb_flow_dissect_cfm(skb, flow_dissector, > + target_container, data, > + nhoff, hlen); > + break; > + } > default: > fdret = FLOW_DISSECT_RET_OUT_BAD; > break; > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c > index 885c95191ccf..91f2268e1577 100644 > --- a/net/sched/cls_flower.c > +++ b/net/sched/cls_flower.c > @@ -71,6 +71,7 @@ struct fl_flow_key { > struct flow_dissector_key_num_of_vlans num_of_vlans; > struct flow_dissector_key_pppoe pppoe; > struct flow_dissector_key_l2tpv3 l2tpv3; > + struct flow_dissector_key_cfm cfm; > } __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */ > > struct fl_flow_mask_range { > @@ -711,7 +712,7 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = { > [TCA_FLOWER_KEY_PPPOE_SID] = { .type = NLA_U16 }, > [TCA_FLOWER_KEY_PPP_PROTO] = { .type = NLA_U16 }, > [TCA_FLOWER_KEY_L2TPV3_SID] = { .type = NLA_U32 }, > - > + [TCA_FLOWER_KEY_CFM] = { .type = NLA_NESTED }, > }; > > static const struct nla_policy > @@ -760,6 +761,12 @@ mpls_stack_entry_policy[TCA_FLOWER_KEY_MPLS_OPT_LSE_MAX + 1] = { > [TCA_FLOWER_KEY_MPLS_OPT_LSE_LABEL] = { .type = NLA_U32 }, > }; > > +static const struct nla_policy > +cfm_opt_policy[TCA_FLOWER_KEY_CFM_OPT_MAX + 1] = { Why not just use %__TCA_FLOWER_KEY_CFM_OPT_MAX here which is the same? I know it's not how it's been done historically, but anyway. > + [TCA_FLOWER_KEY_CFM_MD_LEVEL] = { .type = NLA_U8 }, > + [TCA_FLOWER_KEY_CFM_OPCODE] = { .type = NLA_U8 }, > +}; > + > static void fl_set_key_val(struct nlattr **tb, > void *val, int val_type, > void *mask, int mask_type, int len) > @@ -1644,6 +1651,67 @@ static bool is_vlan_key(struct nlattr *tb, __be16 *ethertype, > return false; > } > > +#define CFM_MD_LEVEL_MASK 0x7 Can all those definitions be located in one place in some header file instead of being scattered across several C files? You'll need them one day and forget where you place them, some other developers won't know they are somewhere in C files and decide they're not defined in the kernel. > +static int fl_set_key_cfm_md_level(struct nlattr **tb, > + struct fl_flow_key *key, > + struct fl_flow_key *mask, > + struct netlink_ext_ack *extack) > +{ > + u8 level; > + > + if (!tb[TCA_FLOWER_KEY_CFM_MD_LEVEL]) > + return 0; > + > + level = nla_get_u8(tb[TCA_FLOWER_KEY_CFM_MD_LEVEL]); > + if (level & ~CFM_MD_LEVEL_MASK) { > + NL_SET_ERR_MSG_ATTR(extack, > + tb[TCA_FLOWER_KEY_CFM_MD_LEVEL], > + "cfm md level must be 0-7"); > + return -EINVAL; > + } > + key->cfm.mdl = level; > + mask->cfm.mdl = CFM_MD_LEVEL_MASK; > + > + return 0; > +} > + > +static void fl_set_key_cfm_opcode(struct nlattr **tb, > + struct fl_flow_key *key, > + struct fl_flow_key *mask, > + struct netlink_ext_ack *extack) > +{ > + if (!tb[TCA_FLOWER_KEY_CFM_OPCODE]) > + return; > + > + fl_set_key_val(tb, &key->cfm.opcode, > + TCA_FLOWER_KEY_CFM_OPCODE, > + &mask->cfm.opcode, > + TCA_FLOWER_UNSPEC, > + sizeof(key->cfm.opcode)); I think at least some of these fit into the previous line, there's no need to break lines just to break them or have one argument per line. > +} > + > +static int fl_set_key_cfm(struct nlattr **tb, > + struct fl_flow_key *key, > + struct fl_flow_key *mask, > + struct netlink_ext_ack *extack) > +{ > + struct nlattr *nla_cfm_opt[TCA_FLOWER_KEY_CFM_OPT_MAX + 1]; > + int err; > + > + if (!tb[TCA_FLOWER_KEY_CFM]) > + return 0; > + > + err = nla_parse_nested(nla_cfm_opt, TCA_FLOWER_KEY_CFM_OPT_MAX, > + tb[TCA_FLOWER_KEY_CFM], > + cfm_opt_policy, extack); > + if (err < 0) > + return err; > + > + fl_set_key_cfm_opcode(nla_cfm_opt, key, mask, extack); > + > + return fl_set_key_cfm_md_level(nla_cfm_opt, key, mask, extack); > +} > + > static int fl_set_key(struct net *net, struct nlattr **tb, > struct fl_flow_key *key, struct fl_flow_key *mask, > struct netlink_ext_ack *extack) > @@ -1794,6 +1862,10 @@ static int fl_set_key(struct net *net, struct nlattr **tb, > TCA_FLOWER_KEY_L2TPV3_SID, > &mask->l2tpv3.session_id, TCA_FLOWER_UNSPEC, > sizeof(key->l2tpv3.session_id)); > + } else if (key->basic.n_proto == htons(ETH_P_CFM)) { > + ret = fl_set_key_cfm(tb, key, mask, extack); > + if (ret) > + return ret; > } > > if (key->basic.ip_proto == IPPROTO_TCP || > @@ -1976,6 +2048,8 @@ static void fl_init_dissector(struct flow_dissector *dissector, > FLOW_DISSECTOR_KEY_PPPOE, pppoe); > FL_KEY_SET_IF_MASKED(mask, keys, cnt, > FLOW_DISSECTOR_KEY_L2TPV3, l2tpv3); > + FL_KEY_SET_IF_MASKED(mask, keys, cnt, > + FLOW_DISSECTOR_KEY_CFM, cfm); > > skb_flow_dissector_init(dissector, keys, cnt); > } > @@ -2984,6 +3058,45 @@ static int fl_dump_key_ct(struct sk_buff *skb, > return -EMSGSIZE; > } > > +static int fl_dump_key_cfm(struct sk_buff *skb, > + struct fl_flow_key *key, > + struct fl_flow_key *mask) > +{ > + struct nlattr *opts; > + int err; > + > + if (!memchr_inv(&mask->cfm, 0, sizeof(mask->cfm))) > + return 0; > + > + opts = nla_nest_start(skb, TCA_FLOWER_KEY_CFM); > + if (!opts) > + return -EMSGSIZE; > + > + if (mask->cfm.mdl && > + nla_put_u8(skb, > + TCA_FLOWER_KEY_CFM_MD_LEVEL, > + key->cfm.mdl)) { Also weird linewrapping. > + err = -EMSGSIZE; > + goto err_cfm_opts; > + } > + > + if (mask->cfm.opcode && > + nla_put_u8(skb, > + TCA_FLOWER_KEY_CFM_OPCODE, > + key->cfm.opcode)) { (same) > + err = -EMSGSIZE; > + goto err_cfm_opts; > + } > + > + nla_nest_end(skb, opts); > + > + return 0; > + > +err_cfm_opts: > + nla_nest_cancel(skb, opts); > + return err; > +} > + > static int fl_dump_key_options(struct sk_buff *skb, int enc_opt_type, > struct flow_dissector_key_enc_opts *enc_opts) > { > @@ -3266,6 +3379,9 @@ static int fl_dump_key(struct sk_buff *skb, struct net *net, > sizeof(key->hash.hash))) > goto nla_put_failure; > > + if (fl_dump_key_cfm(skb, key, mask)) > + goto nla_put_failure; > + > return 0; > > nla_put_failure: Thanks, Olek
[...] > > +/** > > + * struct flow_dissector_key_cfm > > + * > > + */ > > ??? > > Without a proper kernel-doc, this makes no sense. So either remove this > comment or make a kernel-doc from it, describing the structure and each > its member (I'd go for kernel-doc :P). > I will fix this. > > +struct flow_dissector_key_cfm { > > + u8 mdl:3, > > + ver:5; > > + u8 opcode; > > +}; > > + > > enum flow_dissector_key_id { > > FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */ > > FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */ > > @@ -329,6 +339,7 @@ enum flow_dissector_key_id { > > FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct flow_dissector_key_num_of_vlans */ > > FLOW_DISSECTOR_KEY_PPPOE, /* struct flow_dissector_key_pppoe */ > > FLOW_DISSECTOR_KEY_L2TPV3, /* struct flow_dissector_key_l2tpv3 */ > > + FLOW_DISSECTOR_KEY_CFM, /* struct flow_dissector_key_cfm */ > > > > FLOW_DISSECTOR_KEY_MAX, > > }; > > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h > > index 648a82f32666..d55f70ccfe3c 100644 > > --- a/include/uapi/linux/pkt_cls.h > > +++ b/include/uapi/linux/pkt_cls.h > > @@ -594,6 +594,8 @@ enum { > > > > TCA_FLOWER_KEY_L2TPV3_SID, /* be32 */ > > > > + TCA_FLOWER_KEY_CFM, > > Each existing definitions within this enum have a comment mentioning the > corresponding type (__be32, __u8 and so on), why doesn't this one? > I was following the other nest option attributes which don't have a comment but sure I can add one or probably change the name to include the opts prefix. > > + > > __TCA_FLOWER_MAX, > > }; > > > > @@ -702,6 +704,16 @@ enum { > > TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST = (1 << 1), > > }; > > > > +enum { > > + TCA_FLOWER_KEY_CFM_OPT_UNSPEC, > > + TCA_FLOWER_KEY_CFM_MD_LEVEL, > > + TCA_FLOWER_KEY_CFM_OPCODE, > > + __TCA_FLOWER_KEY_CFM_OPT_MAX, > > +}; > > + > > +#define TCA_FLOWER_KEY_CFM_OPT_MAX \ > > + (__TCA_FLOWER_KEY_CFM_OPT_MAX - 1) > > This fits into one line... > Can't we put it into the enum itself? > I can fix this but putting it into the enum makes it different from the other defintions. So I am not quire sure on that. > > + > > #define TCA_FLOWER_MASK_FLAGS_RANGE (1 << 0) /* Range-based match */ > > > > /* Match-all classifier */ > > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c > > index 25fb0bbc310f..adb23d31f199 100644 > > --- a/net/core/flow_dissector.c > > +++ b/net/core/flow_dissector.c > > @@ -547,6 +547,41 @@ __skb_flow_dissect_arp(const struct sk_buff *skb, > > return FLOW_DISSECT_RET_OUT_GOOD; > > } > > > > +static enum flow_dissect_ret > > +__skb_flow_dissect_cfm(const struct sk_buff *skb, > > + struct flow_dissector *flow_dissector, > > + void *target_container, const void *data, > > + int nhoff, int hlen) > > +{ > > + struct flow_dissector_key_cfm *key_cfm; > > + struct cfm_common_hdr { > > I don't see this type used anywhere else in the code, so you can leave > it anonymous. noted. > > > + __u8 mdlevel_version; > > + __u8 opcode; > > + __u8 flags; > > + __u8 tlv_offset; > > This is a purely-kernel-side structure, so use simply `u8` here for each > of them. I will fix it. > > > + } *hdr, _hdr; > > +#define CFM_MD_LEVEL_SHIFT 5 > > +#define CFM_MD_VERSION_MASK 0x1f > > + > > + if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_CFM)) > > + return FLOW_DISSECT_RET_OUT_GOOD; > > + > > + hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, > > + hlen, &_hdr); > > + if (!hdr) > > + return FLOW_DISSECT_RET_OUT_BAD; > > + > > + key_cfm = skb_flow_dissector_target(flow_dissector, > > + FLOW_DISSECTOR_KEY_CFM, > > + target_container); > > + > > + key_cfm->mdl = hdr->mdlevel_version >> CFM_MD_LEVEL_SHIFT; > > + key_cfm->ver = hdr->mdlevel_version & CFM_MD_VERSION_MASK; > > I'd highly recommend using FIELD_GET() here. > > Or wait, why can't you just use one structure for both FD and the actual > header? You only need two fields going next to each other, so you could > save some cycles by just directly assigning them (I mean, just define > the fields you need, not the whole header since you use only first two > fields). > I am not sure if get this completely. I understand we can reduce the struct size be removing the not needed fields but we still need to use the FIELD_GET here. Please correct me if my understanding is wrong. > > + key_cfm->opcode = hdr->opcode; > > + > > + return FLOW_DISSECT_RET_OUT_GOOD; > > +} > > + > > static enum flow_dissect_ret > > __skb_flow_dissect_gre(const struct sk_buff *skb, > > struct flow_dissector_key_control *key_control, > > @@ -1390,6 +1425,12 @@ bool __skb_flow_dissect(const struct net *net, > > break; > > } > > > > + case htons(ETH_P_CFM): { > > + fdret = __skb_flow_dissect_cfm(skb, flow_dissector, > > + target_container, data, > > + nhoff, hlen); > > + break; > > + } > > default: > > fdret = FLOW_DISSECT_RET_OUT_BAD; > > break; > > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c > > index 885c95191ccf..91f2268e1577 100644 > > --- a/net/sched/cls_flower.c > > +++ b/net/sched/cls_flower.c > > @@ -71,6 +71,7 @@ struct fl_flow_key { > > struct flow_dissector_key_num_of_vlans num_of_vlans; > > struct flow_dissector_key_pppoe pppoe; > > struct flow_dissector_key_l2tpv3 l2tpv3; > > + struct flow_dissector_key_cfm cfm; > > } __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */ > > > > struct fl_flow_mask_range { > > @@ -711,7 +712,7 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = { > > [TCA_FLOWER_KEY_PPPOE_SID] = { .type = NLA_U16 }, > > [TCA_FLOWER_KEY_PPP_PROTO] = { .type = NLA_U16 }, > > [TCA_FLOWER_KEY_L2TPV3_SID] = { .type = NLA_U32 }, > > - > > + [TCA_FLOWER_KEY_CFM] = { .type = NLA_NESTED }, > > }; > > > > static const struct nla_policy > > @@ -760,6 +761,12 @@ mpls_stack_entry_policy[TCA_FLOWER_KEY_MPLS_OPT_LSE_MAX + 1] = { > > [TCA_FLOWER_KEY_MPLS_OPT_LSE_LABEL] = { .type = NLA_U32 }, > > }; > > > > +static const struct nla_policy > > +cfm_opt_policy[TCA_FLOWER_KEY_CFM_OPT_MAX + 1] = { > > Why not just use %__TCA_FLOWER_KEY_CFM_OPT_MAX here which is the same? I > know it's not how it's been done historically, but anyway. I was looking again at the other definitions here. > > > + [TCA_FLOWER_KEY_CFM_MD_LEVEL] = { .type = NLA_U8 }, > > + [TCA_FLOWER_KEY_CFM_OPCODE] = { .type = NLA_U8 }, > > +}; > > + > > static void fl_set_key_val(struct nlattr **tb, > > void *val, int val_type, > > void *mask, int mask_type, int len) > > @@ -1644,6 +1651,67 @@ static bool is_vlan_key(struct nlattr *tb, __be16 *ethertype, > > return false; > > } > > > > +#define CFM_MD_LEVEL_MASK 0x7 > > Can all those definitions be located in one place in some header file > instead of being scattered across several C files? You'll need them one > day and forget where you place them, some other developers won't know > they are somewhere in C files and decide they're not defined in the kernel. > I can try to move them to a header file. I see some attributes have their own header with defines only. > > +static int fl_set_key_cfm_md_level(struct nlattr **tb, > > + struct fl_flow_key *key, > > + struct fl_flow_key *mask, > > + struct netlink_ext_ack *extack) > > +{ > > + u8 level; > > + > > + if (!tb[TCA_FLOWER_KEY_CFM_MD_LEVEL]) > > + return 0; > > + > > + level = nla_get_u8(tb[TCA_FLOWER_KEY_CFM_MD_LEVEL]); > > + if (level & ~CFM_MD_LEVEL_MASK) { > > + NL_SET_ERR_MSG_ATTR(extack, > > + tb[TCA_FLOWER_KEY_CFM_MD_LEVEL], > > + "cfm md level must be 0-7"); > > + return -EINVAL; > > + } > > + key->cfm.mdl = level; > > + mask->cfm.mdl = CFM_MD_LEVEL_MASK; > > + > > + return 0; > > +} > > + > > +static void fl_set_key_cfm_opcode(struct nlattr **tb, > > + struct fl_flow_key *key, > > + struct fl_flow_key *mask, > > + struct netlink_ext_ack *extack) > > +{ > > + if (!tb[TCA_FLOWER_KEY_CFM_OPCODE]) > > + return; > > + > > + fl_set_key_val(tb, &key->cfm.opcode, > > + TCA_FLOWER_KEY_CFM_OPCODE, > > + &mask->cfm.opcode, > > + TCA_FLOWER_UNSPEC, > > + sizeof(key->cfm.opcode)); > > I think at least some of these fit into the previous line, there's no > need to break lines just to break them or have one argument per line. > I will improve this. [...] > + if (mask->cfm.mdl && > > + nla_put_u8(skb, > > + TCA_FLOWER_KEY_CFM_MD_LEVEL, > > + key->cfm.mdl)) { > > Also weird linewrapping. > > > + err = -EMSGSIZE; > > + goto err_cfm_opts; > > + } > > + > > + if (mask->cfm.opcode && > > + nla_put_u8(skb, > > + TCA_FLOWER_KEY_CFM_OPCODE, > > + key->cfm.opcode)) { > > (same) > I will fix both lines as well. Many thanks Zahari > > + err = -EMSGSIZE; > > + goto err_cfm_opts; > > + } > > + > > + nla_nest_end(skb, opts); > > + > > + return 0; > > + > > +err_cfm_opts: > > + nla_nest_cancel(skb, opts); > > + return err; > > +} > > + > > static int fl_dump_key_options(struct sk_buff *skb, int enc_opt_type, > > struct flow_dissector_key_enc_opts *enc_opts) > > { > > @@ -3266,6 +3379,9 @@ static int fl_dump_key(struct sk_buff *skb, struct net *net, > > sizeof(key->hash.hash))) > > goto nla_put_failure; > > > > + if (fl_dump_key_cfm(skb, key, mask)) > > + goto nla_put_failure; > > + > > return 0; > > > > nla_put_failure: > > Thanks, > Olek
From: Zahari Doychev <zahari.doychev@linux.com> Date: Fri, 17 Feb 2023 17:19:20 +0100 > > [...] > >>> +/** >>> + * struct flow_dissector_key_cfm >>> + * >>> + */ >> >> ??? >> >> Without a proper kernel-doc, this makes no sense. So either remove this >> comment or make a kernel-doc from it, describing the structure and each >> its member (I'd go for kernel-doc :P). >> > > I will fix this. > >>> +struct flow_dissector_key_cfm { >>> + u8 mdl:3, >>> + ver:5; >>> + u8 opcode; >>> +}; >>> + >>> enum flow_dissector_key_id { >>> FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */ >>> FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */ >>> @@ -329,6 +339,7 @@ enum flow_dissector_key_id { >>> FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct flow_dissector_key_num_of_vlans */ >>> FLOW_DISSECTOR_KEY_PPPOE, /* struct flow_dissector_key_pppoe */ >>> FLOW_DISSECTOR_KEY_L2TPV3, /* struct flow_dissector_key_l2tpv3 */ >>> + FLOW_DISSECTOR_KEY_CFM, /* struct flow_dissector_key_cfm */ >>> >>> FLOW_DISSECTOR_KEY_MAX, >>> }; >>> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h >>> index 648a82f32666..d55f70ccfe3c 100644 >>> --- a/include/uapi/linux/pkt_cls.h >>> +++ b/include/uapi/linux/pkt_cls.h >>> @@ -594,6 +594,8 @@ enum { >>> >>> TCA_FLOWER_KEY_L2TPV3_SID, /* be32 */ >>> >>> + TCA_FLOWER_KEY_CFM, >> >> Each existing definitions within this enum have a comment mentioning the >> corresponding type (__be32, __u8 and so on), why doesn't this one? >> > > I was following the other nest option attributes which don't have > a comment but sure I can add one or probably change the name to > include the opts prefix. Ah it's nested. You can put a comment with the single word "nested" there. > >>> + >>> __TCA_FLOWER_MAX, >>> }; >>> >>> @@ -702,6 +704,16 @@ enum { >>> TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST = (1 << 1), >>> }; >>> >>> +enum { >>> + TCA_FLOWER_KEY_CFM_OPT_UNSPEC, >>> + TCA_FLOWER_KEY_CFM_MD_LEVEL, >>> + TCA_FLOWER_KEY_CFM_OPCODE, >>> + __TCA_FLOWER_KEY_CFM_OPT_MAX, >>> +}; >>> + >>> +#define TCA_FLOWER_KEY_CFM_OPT_MAX \ >>> + (__TCA_FLOWER_KEY_CFM_OPT_MAX - 1) >> >> This fits into one line... >> Can't we put it into the enum itself? >> > > I can fix this but putting it into the enum makes it different from the > other defintions. So I am not quire sure on that. Nothing present in the kernel automatically means it's good or approved or you should go only that route. Write the code the way you feel it looks the best and will see what others think. > >>> + >>> #define TCA_FLOWER_MASK_FLAGS_RANGE (1 << 0) /* Range-based match */ [...] >>> + key_cfm = skb_flow_dissector_target(flow_dissector, >>> + FLOW_DISSECTOR_KEY_CFM, >>> + target_container); >>> + >>> + key_cfm->mdl = hdr->mdlevel_version >> CFM_MD_LEVEL_SHIFT; >>> + key_cfm->ver = hdr->mdlevel_version & CFM_MD_VERSION_MASK; >> >> I'd highly recommend using FIELD_GET() here. >> >> Or wait, why can't you just use one structure for both FD and the actual >> header? You only need two fields going next to each other, so you could >> save some cycles by just directly assigning them (I mean, just define >> the fields you need, not the whole header since you use only first two >> fields). >> > > I am not sure if get this completely. I understand we can reduce the > struct size be removing the not needed fields but we still need to > use the FIELD_GET here. Please correct me if my understanding is wrong. If both packet header and kernel-side container will have the same layout, you could assign the fields directly. key_cfm->mdlevel_version = hdr->mdlevel_version; key_cfm->opcode = hdr->opcode; > >>> + key_cfm->opcode = hdr->opcode; >>> + >>> + return FLOW_DISSECT_RET_OUT_GOOD; >>> +} [...] Thanks, Olek
diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h index 5ccf52ef8809..a70497f96bed 100644 --- a/include/net/flow_dissector.h +++ b/include/net/flow_dissector.h @@ -297,6 +297,16 @@ struct flow_dissector_key_l2tpv3 { __be32 session_id; }; +/** + * struct flow_dissector_key_cfm + * + */ +struct flow_dissector_key_cfm { + u8 mdl:3, + ver:5; + u8 opcode; +}; + enum flow_dissector_key_id { FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */ FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */ @@ -329,6 +339,7 @@ enum flow_dissector_key_id { FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct flow_dissector_key_num_of_vlans */ FLOW_DISSECTOR_KEY_PPPOE, /* struct flow_dissector_key_pppoe */ FLOW_DISSECTOR_KEY_L2TPV3, /* struct flow_dissector_key_l2tpv3 */ + FLOW_DISSECTOR_KEY_CFM, /* struct flow_dissector_key_cfm */ FLOW_DISSECTOR_KEY_MAX, }; diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h index 648a82f32666..d55f70ccfe3c 100644 --- a/include/uapi/linux/pkt_cls.h +++ b/include/uapi/linux/pkt_cls.h @@ -594,6 +594,8 @@ enum { TCA_FLOWER_KEY_L2TPV3_SID, /* be32 */ + TCA_FLOWER_KEY_CFM, + __TCA_FLOWER_MAX, }; @@ -702,6 +704,16 @@ enum { TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST = (1 << 1), }; +enum { + TCA_FLOWER_KEY_CFM_OPT_UNSPEC, + TCA_FLOWER_KEY_CFM_MD_LEVEL, + TCA_FLOWER_KEY_CFM_OPCODE, + __TCA_FLOWER_KEY_CFM_OPT_MAX, +}; + +#define TCA_FLOWER_KEY_CFM_OPT_MAX \ + (__TCA_FLOWER_KEY_CFM_OPT_MAX - 1) + #define TCA_FLOWER_MASK_FLAGS_RANGE (1 << 0) /* Range-based match */ /* Match-all classifier */ diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c index 25fb0bbc310f..adb23d31f199 100644 --- a/net/core/flow_dissector.c +++ b/net/core/flow_dissector.c @@ -547,6 +547,41 @@ __skb_flow_dissect_arp(const struct sk_buff *skb, return FLOW_DISSECT_RET_OUT_GOOD; } +static enum flow_dissect_ret +__skb_flow_dissect_cfm(const struct sk_buff *skb, + struct flow_dissector *flow_dissector, + void *target_container, const void *data, + int nhoff, int hlen) +{ + struct flow_dissector_key_cfm *key_cfm; + struct cfm_common_hdr { + __u8 mdlevel_version; + __u8 opcode; + __u8 flags; + __u8 tlv_offset; + } *hdr, _hdr; +#define CFM_MD_LEVEL_SHIFT 5 +#define CFM_MD_VERSION_MASK 0x1f + + if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_CFM)) + return FLOW_DISSECT_RET_OUT_GOOD; + + hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, + hlen, &_hdr); + if (!hdr) + return FLOW_DISSECT_RET_OUT_BAD; + + key_cfm = skb_flow_dissector_target(flow_dissector, + FLOW_DISSECTOR_KEY_CFM, + target_container); + + key_cfm->mdl = hdr->mdlevel_version >> CFM_MD_LEVEL_SHIFT; + key_cfm->ver = hdr->mdlevel_version & CFM_MD_VERSION_MASK; + key_cfm->opcode = hdr->opcode; + + return FLOW_DISSECT_RET_OUT_GOOD; +} + static enum flow_dissect_ret __skb_flow_dissect_gre(const struct sk_buff *skb, struct flow_dissector_key_control *key_control, @@ -1390,6 +1425,12 @@ bool __skb_flow_dissect(const struct net *net, break; } + case htons(ETH_P_CFM): { + fdret = __skb_flow_dissect_cfm(skb, flow_dissector, + target_container, data, + nhoff, hlen); + break; + } default: fdret = FLOW_DISSECT_RET_OUT_BAD; break; diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index 885c95191ccf..91f2268e1577 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -71,6 +71,7 @@ struct fl_flow_key { struct flow_dissector_key_num_of_vlans num_of_vlans; struct flow_dissector_key_pppoe pppoe; struct flow_dissector_key_l2tpv3 l2tpv3; + struct flow_dissector_key_cfm cfm; } __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */ struct fl_flow_mask_range { @@ -711,7 +712,7 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = { [TCA_FLOWER_KEY_PPPOE_SID] = { .type = NLA_U16 }, [TCA_FLOWER_KEY_PPP_PROTO] = { .type = NLA_U16 }, [TCA_FLOWER_KEY_L2TPV3_SID] = { .type = NLA_U32 }, - + [TCA_FLOWER_KEY_CFM] = { .type = NLA_NESTED }, }; static const struct nla_policy @@ -760,6 +761,12 @@ mpls_stack_entry_policy[TCA_FLOWER_KEY_MPLS_OPT_LSE_MAX + 1] = { [TCA_FLOWER_KEY_MPLS_OPT_LSE_LABEL] = { .type = NLA_U32 }, }; +static const struct nla_policy +cfm_opt_policy[TCA_FLOWER_KEY_CFM_OPT_MAX + 1] = { + [TCA_FLOWER_KEY_CFM_MD_LEVEL] = { .type = NLA_U8 }, + [TCA_FLOWER_KEY_CFM_OPCODE] = { .type = NLA_U8 }, +}; + static void fl_set_key_val(struct nlattr **tb, void *val, int val_type, void *mask, int mask_type, int len) @@ -1644,6 +1651,67 @@ static bool is_vlan_key(struct nlattr *tb, __be16 *ethertype, return false; } +#define CFM_MD_LEVEL_MASK 0x7 +static int fl_set_key_cfm_md_level(struct nlattr **tb, + struct fl_flow_key *key, + struct fl_flow_key *mask, + struct netlink_ext_ack *extack) +{ + u8 level; + + if (!tb[TCA_FLOWER_KEY_CFM_MD_LEVEL]) + return 0; + + level = nla_get_u8(tb[TCA_FLOWER_KEY_CFM_MD_LEVEL]); + if (level & ~CFM_MD_LEVEL_MASK) { + NL_SET_ERR_MSG_ATTR(extack, + tb[TCA_FLOWER_KEY_CFM_MD_LEVEL], + "cfm md level must be 0-7"); + return -EINVAL; + } + key->cfm.mdl = level; + mask->cfm.mdl = CFM_MD_LEVEL_MASK; + + return 0; +} + +static void fl_set_key_cfm_opcode(struct nlattr **tb, + struct fl_flow_key *key, + struct fl_flow_key *mask, + struct netlink_ext_ack *extack) +{ + if (!tb[TCA_FLOWER_KEY_CFM_OPCODE]) + return; + + fl_set_key_val(tb, &key->cfm.opcode, + TCA_FLOWER_KEY_CFM_OPCODE, + &mask->cfm.opcode, + TCA_FLOWER_UNSPEC, + sizeof(key->cfm.opcode)); +} + +static int fl_set_key_cfm(struct nlattr **tb, + struct fl_flow_key *key, + struct fl_flow_key *mask, + struct netlink_ext_ack *extack) +{ + struct nlattr *nla_cfm_opt[TCA_FLOWER_KEY_CFM_OPT_MAX + 1]; + int err; + + if (!tb[TCA_FLOWER_KEY_CFM]) + return 0; + + err = nla_parse_nested(nla_cfm_opt, TCA_FLOWER_KEY_CFM_OPT_MAX, + tb[TCA_FLOWER_KEY_CFM], + cfm_opt_policy, extack); + if (err < 0) + return err; + + fl_set_key_cfm_opcode(nla_cfm_opt, key, mask, extack); + + return fl_set_key_cfm_md_level(nla_cfm_opt, key, mask, extack); +} + static int fl_set_key(struct net *net, struct nlattr **tb, struct fl_flow_key *key, struct fl_flow_key *mask, struct netlink_ext_ack *extack) @@ -1794,6 +1862,10 @@ static int fl_set_key(struct net *net, struct nlattr **tb, TCA_FLOWER_KEY_L2TPV3_SID, &mask->l2tpv3.session_id, TCA_FLOWER_UNSPEC, sizeof(key->l2tpv3.session_id)); + } else if (key->basic.n_proto == htons(ETH_P_CFM)) { + ret = fl_set_key_cfm(tb, key, mask, extack); + if (ret) + return ret; } if (key->basic.ip_proto == IPPROTO_TCP || @@ -1976,6 +2048,8 @@ static void fl_init_dissector(struct flow_dissector *dissector, FLOW_DISSECTOR_KEY_PPPOE, pppoe); FL_KEY_SET_IF_MASKED(mask, keys, cnt, FLOW_DISSECTOR_KEY_L2TPV3, l2tpv3); + FL_KEY_SET_IF_MASKED(mask, keys, cnt, + FLOW_DISSECTOR_KEY_CFM, cfm); skb_flow_dissector_init(dissector, keys, cnt); } @@ -2984,6 +3058,45 @@ static int fl_dump_key_ct(struct sk_buff *skb, return -EMSGSIZE; } +static int fl_dump_key_cfm(struct sk_buff *skb, + struct fl_flow_key *key, + struct fl_flow_key *mask) +{ + struct nlattr *opts; + int err; + + if (!memchr_inv(&mask->cfm, 0, sizeof(mask->cfm))) + return 0; + + opts = nla_nest_start(skb, TCA_FLOWER_KEY_CFM); + if (!opts) + return -EMSGSIZE; + + if (mask->cfm.mdl && + nla_put_u8(skb, + TCA_FLOWER_KEY_CFM_MD_LEVEL, + key->cfm.mdl)) { + err = -EMSGSIZE; + goto err_cfm_opts; + } + + if (mask->cfm.opcode && + nla_put_u8(skb, + TCA_FLOWER_KEY_CFM_OPCODE, + key->cfm.opcode)) { + err = -EMSGSIZE; + goto err_cfm_opts; + } + + nla_nest_end(skb, opts); + + return 0; + +err_cfm_opts: + nla_nest_cancel(skb, opts); + return err; +} + static int fl_dump_key_options(struct sk_buff *skb, int enc_opt_type, struct flow_dissector_key_enc_opts *enc_opts) { @@ -3266,6 +3379,9 @@ static int fl_dump_key(struct sk_buff *skb, struct net *net, sizeof(key->hash.hash))) goto nla_put_failure; + if (fl_dump_key_cfm(skb, key, mask)) + goto nla_put_failure; + return 0; nla_put_failure: