Message-ID: <a492bec9-8eaf-4884-bb4b-c8487d84de73@ovn.org>
Date: Wed, 21 May 2025 13:50:10 +0200
From: Ilya Maximets <i.maximets@....org>
To: Faicker Mo <faicker.mo@...layer.com>, Eelco Chaudron <echaudro@...hat.com>
Cc: i.maximets@....org, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "ovs-dev@...nvswitch.org" <ovs-dev@...nvswitch.org>,
 "aconole@...hat.com" <aconole@...hat.com>,
 "davem@...emloft.net" <davem@...emloft.net>,
 "edumazet@...gle.com" <edumazet@...gle.com>,
 "kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com"
 <pabeni@...hat.com>, "horms@...nel.org" <horms@...nel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "dev@...nvswitch.org" <dev@...nvswitch.org>
Subject: Re: [PATCH] net: openvswitch: Fix the dead loop of MPLS parse

On 5/21/25 6:10 AM, Faicker Mo wrote:
> 
> On 2025/5/20, 18:38, "Ilya Maximets" <i.maximets@....org <mailto:i.maximets@....org>> wrote:
>> The idea of not failing the parsing is to allow forwarding the packet
>> based on parsed ethernet header.  So, we shouldn't fail here.
>> We're also keeping num_labels_mask at zero in this case, so it'll be
>> an MPLS packet with zero labels and it should not be parsed further,
>> but can still be forwarded.
> 
> num_labels_mask should keep the first max MPLS_LABEL_DEPTH labels.

If the packet is not properly formatted (doesn't have BOS bit set anywhere),
then it should be fine to treat it as packet without MPLS headers.

> This is a MPLS packet with max MPLS_LABEL_DEPTH labels to continue forwarding.
> 
>> But also, there is another overflow here that is actually causing an
>> infinite loop - the label_count * MPLS_HLEN easily overflows u8, so
>> the check_header() a few lines above doesn't work properly starting
>> at 32 labels and doesn't break the loop. We need to switch the
>> label_count back to size_t or other sufficiently large type to avoid
>> this overflow and make the parsing end naturally when we hit the end
>> of the packet.
> 
> No overflow with check_header()?

Yes, if we change the type from u8 to size_t, then check_header() will
fail once we get to the end of the packet.  And that will end the loop.

> 
>> With the type change we may still consider returning early, though it's
>> not clear what the value we should aim for in this case. And we need to
>> figure out what the skb_inner_network_header() should be in this case.
> 
> We may parse until the packet end to set the inner network header?set to 0 if fail.

I think, for now we can just parse til the end of a packet.  The inner
header pointer will also point somewhere to the end of a packet in this
case and so there should be no way to actually perform MPLS GSO.  No need
to set it to zero.  We may consider setting it to zero in the future,
if necessary.

So, AFAIU, we just need to change the type in this function and that
should resolve the infinite loop issue, because check_header() will
eventually fail.

> 
>> One other thing,
> 
>> For some reason the patch was not delivered to lore.kernel.org
>> and is not available in netdev+bpf patchwork and not in lkml.org.
>> Both of our replies are available in list archives.  The original
>> email is available only via mail-archive, but it is ovs-dev and
>> not the netdev list:
>>  https://www.mail-archive.com/ovs-dev@openvswitch.org/msg94895.html>7C0d27725cb11d49f0b479a26ae758f26d%7C1%7C0%7C638833450887452972%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=1DXkGXqyAYVUf9BxH45MGy4BGSNozuUQwrU0IP8t%2FLI%3D&reserved=0
>> Same for v2.
> 
>> Is kernel.org blocking the sender somehow?  Does anyone know?
> 
> Sorry. This is my outlook web problem with html after the plain text body.

Yeah, you need to find a different mail client, since your patches are
not delivered to netdev@, and that's the actual place you want them to
be delivered to.

Best regards, Ilya Maximets.

Powered by blists - more mailing lists