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

Guillaume Nault
2022-07-12T17:20:44Z

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