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

[Guillaume Nault <gnault@redhat.com>]

On Fri, Jul 01, 2022 at 10:53:51AM +0000, Drewek, Wojciech wrote:
> > > +/**
> > > + * struct flow_dissector_key_pppoe:
> > > + * @session_id: pppoe session id
> > > + * @ppp_proto: ppp protocol
> > > + */
> > > +struct flow_dissector_key_pppoe {
> > > +	u16 session_id;
> > > +	__be16 ppp_proto;
> > > +};
> > 
> > Why isn't session_id __be16 too?
> 
> I've got general impression that storing protocols values
> in big endian is a standard through out the code and other values like vlan_id
> don't have to be stored in big endian, but maybe it's just my illusion :)
> I can change that in v3.

I don't know of any written rule, but looking at other keys, every
protocol field is stored with the endianess used on the wire. Only
metadata are stored in host byte order. For flow_dissector_key_vlan,
vlan_id is a bit special since it's only 12 bits long, but other vlan
fields are stored in big endian (vlan_tci is __be16 for example). And
vlan ids are special for another reason too: they're also metadata
stored in skbuffs because of vlan hardware offload.

But PPPoE Session Id is clearly read from the packet header, where it's
stored in network byte order.

> > Also, I'm not sure I like mixing the PPPoE session ID and PPP protocol
> > fields in the same structure: they're part of two different protocols.
> > However, I can't anticipate any technical problem in doing so, and I
> > guess there's no easy way to let the flow dissector parse the PPP
> > header independently. So well, maybe we don't have choice...
> 
> We are not planning to match on other fields from PPP protocol so
> separate structure just for it is not needed I guess.

FTR, I believe it's okay to take this shortcut but for different
reasons:

 * When transported over PPPoE, PPP frames are not supposed to have
   address and control fields. Therefore, in this case, the PPP header
   is limitted to the protocol field, so the dissector key would never
   have to be extended.

 * It's unlikely enough that we'd ever have any other protocol
   transporting PPP frames to implement in the flow dissector.
   Therefore, independent PPP dissection code probably won't be needed
   (even if one wants to add support for L2TP or PPTP in the flow
   dissector, that probably should be done with tunnel metadata, like
   VXLAN).

 * We have gotos for jumping to "network" or "transport" header dissection
   (proto_again and ip_proto_again), but no place to restart at the "link"
   header and no way to tell what type of link layer header we're
   requesting to parse (Ethernet or PPP).

For all these reasons, I believe your approach is an acceptable
shortcut. But I don't buy the "let's limit the flow dissector to what
we plan to support in ice" argument.

> > > @@ -1221,19 +1254,29 @@ bool __skb_flow_dissect(const struct net *net,
> > >  		}
> > >
> > >  		nhoff += PPPOE_SES_HLEN;
> > > -		switch (hdr->proto) {
> > > -		case htons(PPP_IP):
> > > +		if (hdr->proto == htons(PPP_IP)) {
> > >  			proto = htons(ETH_P_IP);
> > >  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> > > -			break;
> > > -		case htons(PPP_IPV6):
> > > +		} else if (hdr->proto == htons(PPP_IPV6)) {
> > >  			proto = htons(ETH_P_IPV6);
> > >  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> > > -			break;
> > 
> > 1)
> > Looks like you could easily handle MPLS too. Did you skip it on
> > purpose? (not enough users to justify writing and maintaining
> > the code?).
> > 
> > I don't mean MPLS has to be supported; I'd just like to know if it was
> > considered.
> 
> Yes, exactly as you write, not enough users, but I can see thet MPLS should
> be easy to implement so I'll include it in the next version.

Okay.

> > 2)
> > Also this whole test is a bit weak: the version, type and code fields
> > must have precise values for the PPPoE Session packet to be valid.
> > If either version or type is different than 1, then the packet
> > advertises a new version of the protocol that we don't know how to parse
> > (or most probably the packet was forged or corrupted). A non-zero code
> > is also invalid.
> > 
> > I know the code was already like this before your patch, but it's
> > probably better to fix it before implementing hardware offload.
> 
> Sure, I'll add packet validation in next version.

Great!

> > 3)
> > Finally, the PPP protocol could be compressed and stored in 1 byte
> > instead of 2. This case wasn't handled before your patch, but I think
> > that should be fixed too before implementing hardware offload.
> 
> We faced that issue but we couldn't find out what indicates
> when ppp protocol is stored in 1 byte instead of 2.

That depends on the least significant bit of the first byte. If it's 0
then the next byte is also part of the protocol field. If it's one,
the protocol is "compressed" (that is the high order 0x00 byte has been
stripped and we're left with only the least significant byte).

This is explained more formally in RFC 1661 section 2 (PPP Encapsulation):
  https://datatracker.ietf.org/doc/html/rfc1661#section-2

and section 6.5 (Protocol-Field-Compression (PFC)):
  https://datatracker.ietf.org/doc/html/rfc1661#section-6.5

There should be no reason to use this old PPP feature with PPPoE, but
it's still valid (even though it breaks IP header alignment).