Message-ID: <MW4PR11MB57765C3D4A7B8B95F4145BBEFDBD9@MW4PR11MB5776.namprd11.prod.outlook.com>
Date:   Fri, 1 Jul 2022 13:33:35 +0000
From:   "Drewek, Wojciech" <wojciech.drewek@...el.com>
To:     Guillaume Nault <gnault@...hat.com>
CC:     Marcin Szycik <marcin.szycik@...ux.intel.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>,
        "Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
        "gustavoars@...nel.org" <gustavoars@...nel.org>,
        "baowen.zheng@...igine.com" <baowen.zheng@...igine.com>,
        "boris.sukholitko@...adcom.com" <boris.sukholitko@...adcom.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "jhs@...atatu.com" <jhs@...atatu.com>,
        "jiri@...nulli.us" <jiri@...nulli.us>,
        "kurt@...utronix.de" <kurt@...utronix.de>,
        "pablo@...filter.org" <pablo@...filter.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "paulb@...dia.com" <paulb@...dia.com>,
        "simon.horman@...igine.com" <simon.horman@...igine.com>,
        "komachi.yoshiki@...il.com" <komachi.yoshiki@...il.com>,
        "zhangkaiheb@....com" <zhangkaiheb@....com>,
        "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
        "michal.swiatkowski@...ux.intel.com" 
        <michal.swiatkowski@...ux.intel.com>,
        "Lobakin, Alexandr" <alexandr.lobakin@...el.com>,
        "mostrows@...thlink.net" <mostrows@...thlink.net>,
        "paulus@...ba.org" <paulus@...ba.org>
Subject: RE: [RFC PATCH net-next v3 1/4] flow_dissector: Add PPPoE dissectors



> -----Original Message-----
> From: Guillaume Nault <gnault@...hat.com>
> Sent: piątek, 1 lipca 2022 14:42
> To: Drewek, Wojciech <wojciech.drewek@...el.com>
> Cc: Marcin Szycik <marcin.szycik@...ux.intel.com>; netdev@...r.kernel.org; Nguyen, Anthony L <anthony.l.nguyen@...el.com>;
> davem@...emloft.net; xiyou.wangcong@...il.com; Brandeburg, Jesse <jesse.brandeburg@...el.com>; gustavoars@...nel.org;
> baowen.zheng@...igine.com; boris.sukholitko@...adcom.com; edumazet@...gle.com; kuba@...nel.org; jhs@...atatu.com;
> jiri@...nulli.us; kurt@...utronix.de; pablo@...filter.org; pabeni@...hat.com; paulb@...dia.com; simon.horman@...igine.com;
> komachi.yoshiki@...il.com; zhangkaiheb@....com; intel-wired-lan@...ts.osuosl.org; michal.swiatkowski@...ux.intel.com; Lobakin,
> Alexandr <alexandr.lobakin@...el.com>; mostrows@...thlink.net; paulus@...ba.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.

Powered by blists - more mailing lists