Message-ID: <e4863729-4ddc-f6cc-85f2-333bd996fc6a@intel.com>
Date:   Thu, 16 Feb 2023 19:19:48 +0100
From:   Alexander Lobakin <aleksander.lobakin@...el.com>
To:     Zahari Doychev <zahari.doychev@...ux.com>
CC:     <netdev@...r.kernel.org>, <jhs@...atatu.com>,
        <xiyou.wangcong@...il.com>, <jiri@...nulli.us>,
        <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <hmehrtens@...linear.com>,
        "Zahari Doychev" <zdoychev@...linear.com>
Subject: Re: [PATCH net-next 1/2] net: flower: add support for matching cfm
 fields

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

> From: Zahari Doychev <zdoychev@...linear.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@...linear.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

Powered by blists - more mailing lists