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

[Simon Horman <simon.horman@corigine.com>]

On Sun, Apr 02, 2023 at 05:10:30PM +0200, Zahari Doychev wrote:
> 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>

Hi Zahari,

thanks for your patch.
Some initial feedback from my side follows.

> ---
>  include/net/flow_dissector.h |  21 +++++++
>  include/uapi/linux/pkt_cls.h |   9 +++
>  net/core/flow_dissector.c    |  29 ++++++++++
>  net/sched/cls_flower.c       | 108 ++++++++++++++++++++++++++++++++++-
>  4 files changed, 166 insertions(+), 1 deletion(-)

FWIIW I would have split the flow dissector and cls flower
changes into separate patches.

> 
> diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> index 5ccf52ef8809..e1e7e51db88f 100644
> --- a/include/net/flow_dissector.h
> +++ b/include/net/flow_dissector.h
> @@ -297,6 +297,26 @@ struct flow_dissector_key_l2tpv3 {
>  	__be32 session_id;
>  };
>  
> +/**
> + * struct flow_dissector_key_cfm
> + * @mdl_ver: maintenance domain level(mdl) and cfm protocol version
> + * @opcode: code specifying a type of cfm protocol packet
> + *
> + * See 802.1ag, ITU-T G.8013/Y.1731
> + *         1               2
> + * |7 6 5 4 3 2 1 0|7 6 5 4 3 2 1 0|
> + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> + * | mdl | version |     opcode    |
> + * +-----+---------+-+-+-+-+-+-+-+-+
> + */
> +struct flow_dissector_key_cfm {
> +	u8	mdl_ver;
> +	u8	opcode;
> +};
> +
> +#define FLOW_DIS_CFM_MDL_MASK	 7
> +#define FLOW_DIS_CFM_MDL_SHIFT	 5

I think that if you used GENMASK to create the mask,
and then FIELD_PREP/FIELD_GET to use the mask you
could avoid _SHIFT entirely. Which might be cleaner.

...

> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 25fb0bbc310f..7c694e7b9917 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -547,6 +547,29 @@ __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, *hdr, _hdr;
> +
> +	if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_CFM))
> +		return FLOW_DISSECT_RET_OUT_GOOD;
> +
> +	hdr = __skb_header_pointer(skb, nhoff, sizeof(*key), data, hlen, &_hdr);
> +	if (!hdr)
> +		return FLOW_DISSECT_RET_OUT_BAD;
> +
> +	key = skb_flow_dissector_target(flow_dissector, FLOW_DISSECTOR_KEY_CFM,
> +					target_container);
> +
> +	*key = *hdr;

It is unusual to just copy the header directly to the key.
But as both are two u8 values I guess it is fine.

> +
> +	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 +1413,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);

I do like that you moved the handling into it's own function.
But I do also note that this style differs from adjacent code in this
file.

> +		break;
> +	}
>  	default:
>  		fdret = FLOW_DISSECT_RET_OUT_BAD;
>  		break;

...