Re: [RFC PATCH net-next v4 1/4] flow_dissector: Add PPPoE dissectors

[Guillaume Nault <gnault@redhat.com>]

On Mon, Jul 11, 2022 at 10:23:50AM +0000, Drewek, Wojciech wrote:
> > > diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> > > index a4c6057c7097..af0d429b9a26 100644
> > > --- a/include/net/flow_dissector.h
> > > +++ b/include/net/flow_dissector.h
> > > @@ -261,6 +261,18 @@ struct flow_dissector_key_num_of_vlans {
> > >  	u8 num_of_vlans;
> > >  };
> > >
> > > +/**
> > > + * struct flow_dissector_key_pppoe:
> > > + * @session_id: pppoe session id
> > > + * @ppp_proto: ppp protocol
> > > + * @type: pppoe eth type
> > > + */
> > > +struct flow_dissector_key_pppoe {
> > > +	__be16 session_id;
> > > +	__be16 ppp_proto;
> > > +	__be16 type;
> > 
> > I don't understand the need for the new 'type' field.
> 
> Let's say user want to add below filter with just protocol field:
> tc filter add dev ens6f0 ingress prio 1 protocol ppp_ses action drop
> 
> cls_flower would set basic.n_proto to ETH_P_PPP_SES, then PPPoE packet
> arrives with ppp_proto = PPP_IP, which means that in  __skb_flow_dissect basic.n_proto is going to
> be set to ETH_P_IP. We have a mismatch here cls_flower set basic.n_proto to ETH_P_PPP_SES and
> flow_dissector set it to ETH_P_IP. That's why in such example basic.n_proto has to be set to 0 (it works the same 
> with vlans) and key_pppoe::type has to be used. In other words basic.n_proto can't be used for storing
> ETH_P_PPP_SES because it will store encapsulated protocol.
> 
> We could also use it to match on ETH_P_PPP_DISC.

Thanks for the explanation. That makes sense.

> > > @@ -1214,26 +1250,60 @@ bool __skb_flow_dissect(const struct net *net,
> > >  			struct pppoe_hdr hdr;
> > >  			__be16 proto;
> > >  		} *hdr, _hdr;
> > > +		__be16 ppp_proto;
> > > +
> > >  		hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
> > >  		if (!hdr) {
> > >  			fdret = FLOW_DISSECT_RET_OUT_BAD;
> > >  			break;
> > >  		}
> > >
> > > -		nhoff += PPPOE_SES_HLEN;
> > > -		switch (hdr->proto) {
> > > -		case htons(PPP_IP):
> > > +		if (!is_pppoe_ses_hdr_valid(hdr->hdr)) {
> > > +			fdret = FLOW_DISSECT_RET_OUT_BAD;
> > > +			break;
> > > +		}
> > > +
> > > +		/* least significant bit of the first byte
> > > +		 * indicates if protocol field was compressed
> > > +		 */
> > > +		if (hdr->proto & 1) {
> > > +			ppp_proto = hdr->proto << 8;
> > 
> > This is little endian specific code. We can't make such assumptions.
> 
> Both ppp_proto and hdr->prot are stored in __be16 so left shift by 8 bits
> should always be ok, am I right?

Sorry, I don't understand. How could the test and the bit shift
operation give the correct result on a big endian machine?

Let's say we handle an IPv4 packet and the PPP protocol field isn't
compressed. That is, protocol is 0x0021.
On a big endian machine 'hdr->proto & 1' is true and the bit shift sets
ppp_proto to 0x2100, while the code should have left the original value
untouched.

> Should I use cpu_to_be16 on both 1 and 8. Is that what you mean?

I can't see how cpu_to_be16() could help here. I was thinking of simply
using ntohs(hdr->proto).