From: Boris Sukholitko <boris.sukholitko@broadcom.com>
To: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Vladimir Oltean <olteanv@gmail.com>,
Vadym Kochan <vadym.kochan@plvision.eu>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Cong Wang <xiyou.wangcong@gmail.com>,
Andrew Lunn <andrew@lunn.ch>,
Serhiy Boiko <serhiy.boiko@plvision.eu>,
Volodymyr Mytnyk <volodymyr.mytnyk@plvision.eu>,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
jiri@resnulli.us, idosch@idosch.org, ilya.lifshits@broadcom.com
Subject: Re: [PATCH net-next] net/sched: cls_flower: fix resetting of ether proto mask
Date: Mon, 28 Jun 2021 09:24:34 +0300 [thread overview]
Message-ID: <20210628062434.GA13594@noodle> (raw)
In-Reply-To: <7d0367ab-22e4-522a-11ef-8fb376672b54@mojatatu.com>
[-- Attachment #1: Type: text/plain, Size: 3619 bytes --]
Hi Jamal,
On Thu, Jun 24, 2021 at 04:07:56PM -0400, Jamal Hadi Salim wrote:
> Hi Boris,
>
> Apologies for the latency.
>
Apparently mine is not great either. :)
> On 2021-06-22 11:22 a.m., Boris Sukholitko wrote:
> > On Tue, Jun 22, 2021 at 10:17:45AM -0400, Jamal Hadi Salim wrote:
>
> [..]
>
> > > > Do you by any chance have some naming suggestion? Does
> > > > vlan_pure_ethtype sound ok? What about vlan_{orig, pkt, raw, hdr}_ethtype?
> > > >
> > >
> > > The distinction is in getting the inner vs outer proto, correct?
> >
> > Yes. To be more explicit: the outer protocol (ETH_P_PPP_SES in this case) is
> > invisible to the user due to __skb_flow_dissect drilling down
> > to find the inner protocol.
>
> Ok, seems this is going to be problematic for flower for more than
> just ETH_P_PPP_SES, no? i.e anything that has an inner proto.
> IIUC, basically what you end up seeing in fl_classify() is
> the PPP protocol that is extracted by the dissector?
>
Yes. It looks like the problem for all of tunnel protocols.
> > Yes. Talking specifically about flower's fl_classify and the following
> > rule (0x8864 is ETH_P_PPP_SES):
> >
> > tc filter add dev eth0 ingress protocol 0x8864 flower action simple sdata hi6
> >
> > skb_flow_dissect sets skb_key.basic.n_proto to the inner protocol
> > contained inside the PPP tunnel. fl_mask_lookup will fail finding the
> > outer protocol configured by the user.
> >
>
> For vlans it seems that flower tries to "rectify" the situation
> with skb_protocol() (that why i pointed to that function) - but the
> situation in this case cant be rectified inside fl_classify().
>
> Just quick glance of the dissector code though seems to indicate
> skb->protocol is untouched, no? i.e if you have a simple pppoe with
> ppp protocol == ipv4, skb->protocol should still be 0x8864 on it
> (even when skb_key.basic.n_proto has ipv4).
>
The skb->protocol is probably untouched. Unfortunately I don't see
how this may help us...
>
> > It looks to me that there is no way to match on outer protocol such as
> > ETH_P_PPP_SES at least in flower. Although other filters (e.g. matchall)
> > will work, VLAN packets containing ETH_P_PPP_SES will require flower and
> > still will not match.
>
> This is a consequence of flower using flow_dissector and flow
> dissector loosing information..
>
Yes.
> > > This is because when vlan offloading was merged it skewed
> > > things a little and we had to live with that.
> > >
> > > Flower is unique in its use of the dissector which other classifiers
> > > dont. Is this resolvable by having the fix in the dissector?
> >
> > Yes, the solution suggested by Vladimir and elaborated by myself
> > involves extending the dissector to keep the outer protocol and having
> > flower eth_type match on it. This is the "plan" being quoted above.
> >
> >
> > I believe this is the solution for the non-vlan tagged traffic. For the
> > vlans we already have [c]vlan_ethtype keys taken. Therefore we'll need
> > new [c]vlan_outer_ethtype keys.
> >
>
> I think that would work in the short term but what happens when you
> have vlan in vlan carrying pppoe? i.e how much max nesting can you
> assume and what would you call these fields?
>
Currently flower matches on 2 level of vlans: e.g. it has vlan_prio for
the outer vlan and cvlan_prio for the inner vlan. I have no ambition to
go beyond that. Thus only two keys will be needed: vlan_outer_ethtype
and cvlan_outer_ethtype, I think...
In your vlan in vlan ppoe example, I hope that
cvlan_outer_ethtype == ETH_P_PPP_SES will match.
Thanks,
Boris.
> cheers,
> jamal
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]
prev parent reply other threads:[~2021-06-28 6:24 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-17 16:14 [PATCH net-next] net/sched: cls_flower: fix resetting of ether proto mask Vadym Kochan
2021-06-17 16:41 ` Vladimir Oltean
2021-06-17 19:51 ` Vladimir Oltean
2021-06-21 8:32 ` Boris Sukholitko
2021-06-21 10:09 ` Vladimir Oltean
2021-06-21 14:04 ` Jamal Hadi Salim
2021-06-22 13:13 ` Boris Sukholitko
2021-06-22 14:17 ` Jamal Hadi Salim
2021-06-22 15:22 ` Boris Sukholitko
2021-06-24 20:07 ` Jamal Hadi Salim
2021-06-28 6:24 ` Boris Sukholitko [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210628062434.GA13594@noodle \
--to=boris.sukholitko@broadcom.com \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=idosch@idosch.org \
--cc=ilya.lifshits@broadcom.com \
--cc=jhs@mojatatu.com \
--cc=jiri@resnulli.us \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=olteanv@gmail.com \
--cc=serhiy.boiko@plvision.eu \
--cc=vadym.kochan@plvision.eu \
--cc=volodymyr.mytnyk@plvision.eu \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).