| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <MW4PR11MB5776F1388A0EFB83990120CDFDBD9@MW4PR11MB5776.namprd11.prod.outlook.com>
Date: Fri, 1 Jul 2022 10:53:51 +0000
From: "Drewek, Wojciech" <wojciech.drewek@...el.com>
To: Guillaume Nault <gnault@...hat.com>,
Marcin Szycik <marcin.szycik@...ux.intel.com>
CC: "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
Hi Guillaume,
Thanks for the review!
> -----Original Message-----
> From: Guillaume Nault <gnault@...hat.com>
> Sent: piątek, 1 lipca 2022 01:10
> To: Marcin Szycik <marcin.szycik@...ux.intel.com>
> Cc: 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; Drewek,
> Wojciech <wojciech.drewek@...el.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 Wed, Jun 29, 2022 at 04:38:56PM +0200, Marcin Szycik wrote:
> > From: Wojciech Drewek <wojciech.drewek@...el.com>
> >
> > Allow to dissect PPPoE specific fields which are:
> > - session ID (16 bits)
> > - ppp protocol (16 bits)
> >
> > The goal is to make the following TC command possible:
> >
> > # tc filter add dev ens6f0 ingress prio 1 protocol ppp_ses \
> > flower \
> > pppoe_sid 12 \
> > ppp_proto ip \
> > action drop
> >
> > Note that only PPPoE Session is supported.
> >
> > Signed-off-by: Wojciech Drewek <wojciech.drewek@...el.com>
> > ---
> > v3: revert byte order changes in is_ppp_proto_supported from previous
> > version, add kernel-doc for is_ppp_proto_supported
> > v2: use ntohs instead of htons in is_ppp_proto_supported
> >
> > include/net/flow_dissector.h | 11 ++++++++
> > net/core/flow_dissector.c | 55 ++++++++++++++++++++++++++++++++----
> > 2 files changed, 60 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> > index a4c6057c7097..8ff40c7c3f1c 100644
> > --- a/include/net/flow_dissector.h
> > +++ b/include/net/flow_dissector.h
> > @@ -261,6 +261,16 @@ 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
> > + */
> > +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.
>
> 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.
>
> > enum flow_dissector_key_id {
> > FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
> > FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
> > @@ -291,6 +301,7 @@ enum flow_dissector_key_id {
> > FLOW_DISSECTOR_KEY_CT, /* struct flow_dissector_key_ct */
> > FLOW_DISSECTOR_KEY_HASH, /* struct flow_dissector_key_hash */
> > FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct flow_dissector_key_num_of_vlans */
> > + FLOW_DISSECTOR_KEY_PPPOE, /* struct flow_dissector_key_pppoe */
> >
> > FLOW_DISSECTOR_KEY_MAX,
> > };
> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > index 6aee04f75e3e..42393af477a2 100644
> > --- a/net/core/flow_dissector.c
> > +++ b/net/core/flow_dissector.c
> > @@ -895,6 +895,39 @@ bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
> > return result == BPF_OK;
> > }
> >
> > +/**
> > + * is_ppp_proto_supported - checks if inner PPP protocol should be dissected
> > + * @proto: protocol type (PPP proto field)
> > + */
> > +static bool is_ppp_proto_supported(__be16 proto)
> > +{
> > + switch (proto) {
> > + case htons(PPP_AT):
> > + case htons(PPP_IPX):
> > + case htons(PPP_VJC_COMP):
> > + case htons(PPP_VJC_UNCOMP):
> > + case htons(PPP_MP):
> > + case htons(PPP_COMPFRAG):
> > + case htons(PPP_COMP):
> > + case htons(PPP_MPLS_UC):
> > + case htons(PPP_MPLS_MC):
> > + case htons(PPP_IPCP):
> > + case htons(PPP_ATCP):
> > + case htons(PPP_IPXCP):
> > + case htons(PPP_IPV6CP):
> > + case htons(PPP_CCPFRAG):
> > + case htons(PPP_MPLSCP):
> > + case htons(PPP_LCP):
> > + case htons(PPP_PAP):
> > + case htons(PPP_LQR):
> > + case htons(PPP_CHAP):
> > + case htons(PPP_CBCP):
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > /**
> > * __skb_flow_dissect - extract the flow_keys struct and return it
> > * @net: associated network namespace, derived from @skb if NULL
> > @@ -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.
>
> 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.
>
> 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.
>
> > - default:
> > + } else if (is_ppp_proto_supported(hdr->proto)) {
> > + fdret = FLOW_DISSECT_RET_OUT_GOOD;
> > + } else {
> > fdret = FLOW_DISSECT_RET_OUT_BAD;
> > break;
> > }
> > +
> > + if (dissector_uses_key(flow_dissector,
> > + FLOW_DISSECTOR_KEY_PPPOE)) {
> > + struct flow_dissector_key_pppoe *key_pppoe;
> > +
> > + key_pppoe = skb_flow_dissector_target(flow_dissector,
> > + FLOW_DISSECTOR_KEY_PPPOE,
> > + target_container);
> > + key_pppoe->session_id = ntohs(hdr->hdr.sid);
> > + key_pppoe->ppp_proto = hdr->proto;
> > + }
> > break;
> > }
> > case htons(ETH_P_TIPC): {
> > --
> > 2.35.1
> >
Powered by blists - more mailing lists