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

["Drewek, Wojciech" <wojciech.drewek@intel.com>]

> -----Original Message-----
> From: Guillaume Nault <gnault@redhat.com>
> Sent: piątek, 1 lipca 2022 14:42
> To: Drewek, Wojciech <wojciech.drewek@intel.com>
> Cc: Marcin Szycik <marcin.szycik@linux.intel.com>; netdev@vger.kernel.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
> davem@davemloft.net; xiyou.wangcong@gmail.com; Brandeburg, Jesse <jesse.brandeburg@intel.com>; gustavoars@kernel.org;
> baowen.zheng@corigine.com; boris.sukholitko@broadcom.com; edumazet@google.com; kuba@kernel.org; jhs@mojatatu.com;
> jiri@resnulli.us; kurt@linutronix.de; pablo@netfilter.org; pabeni@redhat.com; paulb@nvidia.com; simon.horman@corigine.com;
> komachi.yoshiki@gmail.com; zhangkaiheb@126.com; intel-wired-lan@lists.osuosl.org; michal.swiatkowski@linux.intel.com; Lobakin,
> Alexandr <alexandr.lobakin@intel.com>; mostrows@earthlink.net; paulus@samba.org
> Subject: Re: [RFC PATCH net-next v3 1/4] flow_dissector: Add PPPoE dissectors
> 
> 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.

Thanks for explanation! We'll use __be16 for session_id since now.

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

Again thanks for explanation. Sorry, I didn't want to suggest that flow_dissector
should be designed based only on our needs. We are happy to change our
implementation if requested.

We will stay with the current approach if this is the conclusion.

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

Thanks for explanation! From the next version we will support both options.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan