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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 3 Apr 2023 17:36:22 +0200
From:   Zahari Doychev <zahari.doychev@...ux.com>
To:     Simon Horman <simon.horman@...igine.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,
        aleksander.lobakin@...el.com,
        Zahari Doychev <zdoychev@...linear.com>
Subject: Re: [PATCH net-next v2 1/2] net: flower: add support for matching
 cfm fields

On Mon, Apr 03, 2023 at 04:54:12PM +0200, Simon Horman wrote:
> On Sun, Apr 02, 2023 at 05:10:30PM +0200, Zahari Doychev wrote:
> > 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>
> 
> 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.

I think it makes sense so I would do it.

> 
> > 
> > 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.

It was proposed in the previous version for different part but it applies here
as well so I would change it.

> 
> ...
> 
> > 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.

I was wondering about this so I would use the fields here.

> 
> > +
> > +	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.

I would prefer to have an own function here as I find __skb_flow_dissect
long enough already. But if you insist I am okay to change it.

Thanks
Zahari

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