[PATCH net-next 0/2] flower add cfm support

[PATCH net-next 0/2] flower add cfm support

[Zahari Doychev]

The first patch adds cfm support to the flower classifier. 
The second adds a selftest for the flower cfm functionality.

iproute2 changes will come in follow up patches.

rfc->v1:
 - add selftest to the makefile TEST_PROGS.

Zahari Doychev (2):
  net: flower: add support for matching cfm fields
  selftests: net: add tc flower cfm test

 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 +++++++++++-
 .../testing/selftests/net/forwarding/Makefile |   1 +
 .../selftests/net/forwarding/tc_flower_cfm.sh | 168 ++++++++++++++++++
 6 files changed, 350 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/net/forwarding/tc_flower_cfm.sh

-- 
2.39.1

[PATCH net-next 1/2] net: flower: add support for matching cfm fields

[Zahari Doychev]

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
+ *
+ */
+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:
-- 
2.39.1

[PATCH net-next 2/2] selftests: net: add tc flower cfm test

[Zahari Doychev]

From: Zahari Doychev <zdoychev@maxlinear.com>

New cfm flower test case is added to the net forwarding selfttests.

Signed-off-by: Zahari Doychev <zdoychev@maxlinear.com>
---
 .../testing/selftests/net/forwarding/Makefile |   1 +
 .../selftests/net/forwarding/tc_flower_cfm.sh | 168 ++++++++++++++++++
 2 files changed, 169 insertions(+)
 create mode 100755 tools/testing/selftests/net/forwarding/tc_flower_cfm.sh

diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
index 91201ab3c4fc..72ed9b18ba28 100644
--- a/tools/testing/selftests/net/forwarding/Makefile
+++ b/tools/testing/selftests/net/forwarding/Makefile
@@ -82,6 +82,7 @@ TEST_PROGS = bridge_igmp.sh \
 	tc_chains.sh \
 	tc_flower_router.sh \
 	tc_flower.sh \
+	tc_flower_cfm.sh \
 	tc_mpls_l2vpn.sh \
 	tc_police.sh \
 	tc_shblocks.sh \
diff --git a/tools/testing/selftests/net/forwarding/tc_flower_cfm.sh b/tools/testing/selftests/net/forwarding/tc_flower_cfm.sh
new file mode 100755
index 000000000000..c536a3bba8e7
--- /dev/null
+++ b/tools/testing/selftests/net/forwarding/tc_flower_cfm.sh
@@ -0,0 +1,168 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+ALL_TESTS="match_cfm_opcode match_cfm_level match_cfm_level_and_opcode"
+
+NUM_NETIFS=2
+source tc_common.sh
+source lib.sh
+
+tcflags="skip_hw"
+
+h1_create()
+{
+	simple_if_init $h1 192.0.2.1/24 198.51.100.1/24
+}
+
+h1_destroy()
+{
+	simple_if_fini $h1 192.0.2.1/24 198.51.100.1/24
+}
+
+h2_create()
+{
+	simple_if_init $h2 192.0.2.2/24 198.51.100.2/24
+	tc qdisc add dev $h2 clsact
+}
+
+h2_destroy()
+{
+	tc qdisc del dev $h2 clsact
+	simple_if_fini $h2 192.0.2.2/24 198.51.100.2/24
+}
+
+cfm_mdl_opcode()
+{
+	local mdl=$1
+	local op=$2
+	local flags=$3
+	local tlv_offset=$4
+
+	printf "%02x %02x %02x %02x"    \
+		   $((mdl << 5))             \
+		   $((op & 0xff))             \
+		   $((flags & 0xff)) \
+		   $tlv_offset
+}
+
+match_cfm_opcode()
+{
+	local ethtype="89 02"; readonly ethtype
+	RET=0
+
+	tc filter add dev $h2 ingress protocol cfm pref 1 handle 101 \
+	   flower cfm op 47 action drop
+	tc filter add dev $h2 ingress protocol cfm pref 2 handle 102 \
+	   flower cfm op 43 action drop
+
+	pkt="$ethtype $(cfm_mdl_opcode 7 47 0 4)"
+	$MZ $h1 -c 1 -p 64 -a $h1mac -b $h2mac "$pkt" -q
+	pkt="$ethtype $(cfm_mdl_opcode 6 5 0 4)"
+	$MZ $h1 -c 1 -p 64 -a $h1mac -b $h2mac "$pkt" -q
+
+	tc_check_packets "dev $h2 ingress" 101 1
+	check_err $? "Did not match on correct opcode"
+
+	tc_check_packets "dev $h2 ingress" 102 0
+	check_err $? "Matched on the wrong opcode"
+
+	tc filter del dev $h2 ingress protocol cfm pref 1 handle 101 flower
+	tc filter del dev $h2 ingress protocol cfm pref 2 handle 102 flower
+
+	log_test "CFM opcode match test"
+}
+
+match_cfm_level()
+{
+	local ethtype="89 02"; readonly ethtype
+	RET=0
+
+	tc filter add dev $h2 ingress protocol cfm pref 1 handle 101 \
+	   flower cfm mdl 5 action drop
+	tc filter add dev $h2 ingress protocol cfm pref 2 handle 102 \
+	   flower cfm mdl 3 action drop
+
+	pkt="$ethtype $(cfm_mdl_opcode 5 42 0 4)"
+	$MZ $h1 -c 1 -p 64 -a $h1mac -b $h2mac "$pkt" -q
+	pkt="$ethtype $(cfm_mdl_opcode 6 1 0 4)"
+	$MZ $h1 -c 1 -p 64 -a $h1mac -b $h2mac "$pkt" -q
+
+	tc_check_packets "dev $h2 ingress" 101 1
+	check_err $? "Did not match on correct level"
+
+	tc_check_packets "dev $h2 ingress" 102 0
+	check_err $? "Matched on the wrong level"
+
+	tc filter del dev $h2 ingress protocol cfm pref 1 handle 101 flower
+	tc filter del dev $h2 ingress protocol cfm pref 2 handle 102 flower
+
+	log_test "CFM level match test"
+}
+
+match_cfm_level_and_opcode()
+{
+	local ethtype="89 02"; readonly ethtype
+	RET=0
+
+	tc filter add dev $h2 ingress protocol cfm pref 1 handle 101 \
+	   flower cfm mdl 5 op 41 action drop
+	tc filter add dev $h2 ingress protocol cfm pref 2 handle 102 \
+	   flower cfm mdl 7 op 42 action drop
+
+	pkt="$ethtype $(cfm_mdl_opcode 5 41 0 4)"
+	$MZ $h1 -c 1 -p 64 -a $h1mac -b $h2mac "$pkt" -q
+	pkt="$ethtype $(cfm_mdl_opcode 7 3 0 4)"
+	$MZ $h1 -c 1 -p 64 -a $h1mac -b $h2mac "$pkt" -q
+	pkt="$ethtype $(cfm_mdl_opcode 3 42 0 4)"
+	$MZ $h1 -c 1 -p 64 -a $h1mac -b $h2mac "$pkt" -q
+
+	tc_check_packets "dev $h2 ingress" 101 1
+	check_err $? "Did not match on correct level and opcode"
+	tc_check_packets "dev $h2 ingress" 102 0
+	check_err $? "Matched on the wrong level and opcode"
+
+	tc filter del dev $h2 ingress protocol cfm pref 1 handle 101 flower
+	tc filter del dev $h2 ingress protocol cfm pref 2 handle 102 flower
+
+	log_test "CFM opcode and level match test"
+}
+
+setup_prepare()
+{
+	h1=${NETIFS[p1]}
+	h2=${NETIFS[p2]}
+	h1mac=$(mac_get $h1)
+	h2mac=$(mac_get $h2)
+
+	vrf_prepare
+
+	h1_create
+	h2_create
+}
+
+cleanup()
+{
+	pre_cleanup
+
+	h2_destroy
+	h1_destroy
+
+	vrf_cleanup
+}
+
+trap cleanup EXIT
+
+setup_prepare
+setup_wait
+
+tests_run
+
+tc_offload_check
+if [[ $? -ne 0 ]]; then
+	log_info "Could not test offloaded functionality"
+else
+	tcflags="skip_sw"
+	tests_run
+fi
+
+exit $EXIT_STATUS
-- 
2.39.1

Re: [PATCH net-next 0/2] flower add cfm support

[Alexander Lobakin]

From: Zahari Doychev <zahari.doychev@linux.com>
Date: Wed, 15 Feb 2023 20:25:52 +0100

> [PATCH net-next 0/2] flower add cfm support

Some badly formed subject here. You likely need this prefix:

net/sched: flower:

and then "add CFM support". i.e. smth like

[PATCH net-next 0/2] net/sched: flower: add cfm support

> The first patch adds cfm support to the flower classifier. 
> The second adds a selftest for the flower cfm functionality.
> 
> iproute2 changes will come in follow up patches.
> 
> rfc->v1:
>  - add selftest to the makefile TEST_PROGS.
> 
> Zahari Doychev (2):
>   net: flower: add support for matching cfm fields
>   selftests: net: add tc flower cfm test
> 
>  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 +++++++++++-
>  .../testing/selftests/net/forwarding/Makefile |   1 +
>  .../selftests/net/forwarding/tc_flower_cfm.sh | 168 ++++++++++++++++++
>  6 files changed, 350 insertions(+), 1 deletion(-)
>  create mode 100755 tools/testing/selftests/net/forwarding/tc_flower_cfm.sh
> 

Thanks,
Olek

Re: [PATCH net-next 1/2] net: flower: add support for matching cfm fields

[Alexander Lobakin]

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

Re: [PATCH net-next 1/2] net: flower: add support for matching cfm fields

[Zahari Doychev]

[...]

> > +/**
> > + * 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

Re: [PATCH net-next 1/2] net: flower: add support for matching cfm fields

[Alexander Lobakin]

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