Re: [PATCH] net: openvswitch: Fix the dead loop of MPLS parse

Re: [PATCH] net: openvswitch: Fix the dead loop of MPLS parse

[Eelco Chaudron]

On 20 May 2025, at 6:13, Faicker Mo wrote:

> The unexpected MPLS packet may not end with the bottom label stack.
> When there are many stacks, The label count value has wrapped around.
> A dead loop occurs, soft lockup/CPU stuck finally.
>
> stack backtrace:
> UBSAN: array-index-out-of-bounds in /build/linux-0Pa0xK/linux-5.15.0/net/openvswitch/flow.c:662:26
> index -1 is out of range for type '__be32 [3]'
> CPU: 34 PID: 0 Comm: swapper/34 Kdump: loaded Tainted: G           OE   5.15.0-121-generic #131-Ubuntu
> Hardware name: Dell Inc. PowerEdge C6420/0JP9TF, BIOS 2.12.2 07/14/2021
> Call Trace:
>  <IRQ>
>  show_stack+0x52/0x5c
>  dump_stack_lvl+0x4a/0x63
>  dump_stack+0x10/0x16
>  ubsan_epilogue+0x9/0x36
>  __ubsan_handle_out_of_bounds.cold+0x44/0x49
>  key_extract_l3l4+0x82a/0x840 [openvswitch]
>  ? kfree_skbmem+0x52/0xa0
>  key_extract+0x9c/0x2b0 [openvswitch]
>  ovs_flow_key_extract+0x124/0x350 [openvswitch]
>  ovs_vport_receive+0x61/0xd0 [openvswitch]
>  ? kernel_init_free_pages.part.0+0x4a/0x70
>  ? get_page_from_freelist+0x353/0x540
>  netdev_port_receive+0xc4/0x180 [openvswitch]
>  ? netdev_port_receive+0x180/0x180 [openvswitch]
>  netdev_frame_hook+0x1f/0x40 [openvswitch]
>  __netif_receive_skb_core.constprop.0+0x23a/0xf00
>  __netif_receive_skb_list_core+0xfa/0x240
>  netif_receive_skb_list_internal+0x18e/0x2a0
>  napi_complete_done+0x7a/0x1c0
>  bnxt_poll+0x155/0x1c0 [bnxt_en]
>  __napi_poll+0x30/0x180
>  net_rx_action+0x126/0x280
>  ? bnxt_msix+0x67/0x80 [bnxt_en]
>  handle_softirqs+0xda/0x2d0
>  irq_exit_rcu+0x96/0xc0
>  common_interrupt+0x8e/0xa0
>  </IRQ>
>
> Signed-off-by: Faicker Mo <faicker.mo@zenlayer.com>
> ---
>  net/openvswitch/flow.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index 8a848ce72e29..834b1b9110ac 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -805,6 +805,8 @@ static int key_extract_l3l4(struct sk_buff *skb, struct sw_flow_key *key)
>                         if (label_count <= MPLS_LABEL_DEPTH)
>                                 memcpy(&key->mpls.lse[label_count - 1], &lse,
>                                        MPLS_HLEN);
> +                       else if (unlikely(label_count == 255))
> +                               return 0;

Thanks for finding and solving this issue, Faicker. I think that if we hit 255 stack labels, it's safe to say the packet is invalid, and we should probably return -EINVAL. This also makes sense because the inner_network_header is not correct based on the terminated decode.

>
>                         skb_set_inner_network_header(skb, skb->mac_len +
>                                                      label_count * MPLS_HLEN);
> --
> 2.34.1

Re: [PATCH] net: openvswitch: Fix the dead loop of MPLS parse

[Ilya Maximets]

On 5/20/25 9:09 AM, Eelco Chaudron wrote:
> 
> 
> On 20 May 2025, at 6:13, Faicker Mo wrote:
> 
>> The unexpected MPLS packet may not end with the bottom label stack.
>> When there are many stacks, The label count value has wrapped around.
>> A dead loop occurs, soft lockup/CPU stuck finally.
>>
>> stack backtrace:
>> UBSAN: array-index-out-of-bounds in /build/linux-0Pa0xK/linux-5.15.0/net/openvswitch/flow.c:662:26
>> index -1 is out of range for type '__be32 [3]'
>> CPU: 34 PID: 0 Comm: swapper/34 Kdump: loaded Tainted: G           OE   5.15.0-121-generic #131-Ubuntu
>> Hardware name: Dell Inc. PowerEdge C6420/0JP9TF, BIOS 2.12.2 07/14/2021
>> Call Trace:
>>  <IRQ>
>>  show_stack+0x52/0x5c
>>  dump_stack_lvl+0x4a/0x63
>>  dump_stack+0x10/0x16
>>  ubsan_epilogue+0x9/0x36
>>  __ubsan_handle_out_of_bounds.cold+0x44/0x49
>>  key_extract_l3l4+0x82a/0x840 [openvswitch]
>>  ? kfree_skbmem+0x52/0xa0
>>  key_extract+0x9c/0x2b0 [openvswitch]
>>  ovs_flow_key_extract+0x124/0x350 [openvswitch]
>>  ovs_vport_receive+0x61/0xd0 [openvswitch]
>>  ? kernel_init_free_pages.part.0+0x4a/0x70
>>  ? get_page_from_freelist+0x353/0x540
>>  netdev_port_receive+0xc4/0x180 [openvswitch]
>>  ? netdev_port_receive+0x180/0x180 [openvswitch]
>>  netdev_frame_hook+0x1f/0x40 [openvswitch]
>>  __netif_receive_skb_core.constprop.0+0x23a/0xf00
>>  __netif_receive_skb_list_core+0xfa/0x240
>>  netif_receive_skb_list_internal+0x18e/0x2a0
>>  napi_complete_done+0x7a/0x1c0
>>  bnxt_poll+0x155/0x1c0 [bnxt_en]
>>  __napi_poll+0x30/0x180
>>  net_rx_action+0x126/0x280
>>  ? bnxt_msix+0x67/0x80 [bnxt_en]
>>  handle_softirqs+0xda/0x2d0
>>  irq_exit_rcu+0x96/0xc0
>>  common_interrupt+0x8e/0xa0
>>  </IRQ>
>>
>> Signed-off-by: Faicker Mo <faicker.mo@zenlayer.com>
>> ---
>>  net/openvswitch/flow.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
>> index 8a848ce72e29..834b1b9110ac 100644
>> --- a/net/openvswitch/flow.c
>> +++ b/net/openvswitch/flow.c
>> @@ -805,6 +805,8 @@ static int key_extract_l3l4(struct sk_buff *skb, struct sw_flow_key *key)
>>                         if (label_count <= MPLS_LABEL_DEPTH)
>>                                 memcpy(&key->mpls.lse[label_count - 1], &lse,
>>                                        MPLS_HLEN);
>> +                       else if (unlikely(label_count == 255))
>> +                               return 0;
> 
> Thanks for finding and solving this issue, Faicker. I think that if we hit 255
> stack labels, it's safe to say the packet is invalid, and we should probably
> return -EINVAL. This also makes sense because the inner_network_header is not
> correct based on the terminated decode.

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.

I'm not sure how the skb_inner_network_header() should be set in this
case though.  It's used in the MPLS GSO code, but since we don't know
the good value, we may need to set it to zero.

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.

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.

Thoughts?

Best regards, Ilya Maximets.

> 
>>
>>                         skb_set_inner_network_header(skb, skb->mac_len +
>>                                                      label_count * MPLS_HLEN);
>> --
>> 2.34.1
> 

Re: [PATCH] net: openvswitch: Fix the dead loop of MPLS parse

[Ilya Maximets]

On 5/20/25 6:13 AM, Faicker Mo wrote:
> The unexpected MPLS packet may not end with the bottom label stack.
> When there are many stacks, The label count value has wrapped around.
> A dead loop occurs, soft lockup/CPU stuck finally.
> 
> stack backtrace:
> UBSAN: array-index-out-of-bounds in /build/linux-0Pa0xK/linux-5.15.0/net/openvswitch/flow.c:662:26
> index -1 is out of range for type '__be32 [3]'
> CPU: 34 PID: 0 Comm: swapper/34 Kdump: loaded Tainted: G           OE   5.15.0-121-generic #131-Ubuntu
> Hardware name: Dell Inc. PowerEdge C6420/0JP9TF, BIOS 2.12.2 07/14/2021
> Call Trace:
>  <IRQ>
>  show_stack+0x52/0x5c
>  dump_stack_lvl+0x4a/0x63
>  dump_stack+0x10/0x16
>  ubsan_epilogue+0x9/0x36
>  __ubsan_handle_out_of_bounds.cold+0x44/0x49
>  key_extract_l3l4+0x82a/0x840 [openvswitch]
>  ? kfree_skbmem+0x52/0xa0
>  key_extract+0x9c/0x2b0 [openvswitch]
>  ovs_flow_key_extract+0x124/0x350 [openvswitch]
>  ovs_vport_receive+0x61/0xd0 [openvswitch]
>  ? kernel_init_free_pages.part.0+0x4a/0x70
>  ? get_page_from_freelist+0x353/0x540
>  netdev_port_receive+0xc4/0x180 [openvswitch]
>  ? netdev_port_receive+0x180/0x180 [openvswitch]
>  netdev_frame_hook+0x1f/0x40 [openvswitch]
>  __netif_receive_skb_core.constprop.0+0x23a/0xf00
>  __netif_receive_skb_list_core+0xfa/0x240
>  netif_receive_skb_list_internal+0x18e/0x2a0
>  napi_complete_done+0x7a/0x1c0
>  bnxt_poll+0x155/0x1c0 [bnxt_en]
>  __napi_poll+0x30/0x180
>  net_rx_action+0x126/0x280
>  ? bnxt_msix+0x67/0x80 [bnxt_en]
>  handle_softirqs+0xda/0x2d0
>  irq_exit_rcu+0x96/0xc0
>  common_interrupt+0x8e/0xa0
>  </IRQ>
> 
> Signed-off-by: Faicker Mo <faicker.mo@zenlayer.com>
> ---
>  net/openvswitch/flow.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index 8a848ce72e29..834b1b9110ac 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -805,6 +805,8 @@ static int key_extract_l3l4(struct sk_buff *skb, struct sw_flow_key *key)
>                         if (label_count <= MPLS_LABEL_DEPTH)
>                                 memcpy(&key->mpls.lse[label_count - 1], &lse,
>                                        MPLS_HLEN);
> +                       else if (unlikely(label_count == 255))
> +                               return 0;
> 
>                         skb_set_inner_network_header(skb, skb->mac_len +
>                                                      label_count * MPLS_HLEN);
> --
> 2.34.1
> 

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
Same for v2.

Is kernel.org blocking the sender somehow?  Does anyone know?

Best regards, Ilya Maximets.

Re: [PATCH] net: openvswitch: Fix the dead loop of MPLS parse

[Jakub Kicinski]

On Tue, 20 May 2025 15:37:53 +0200 Ilya Maximets wrote:
> Is kernel.org blocking the sender somehow?  Does anyone know?

The patch was submitted with an HTML attachment :(
Same with the v2 BTW. vger drops all emails with HTML parts.

Re: [PATCH] net: openvswitch: Fix the dead loop of MPLS parse

[Ilya Maximets]

On 5/21/25 1:44 AM, Jakub Kicinski wrote:
> On Tue, 20 May 2025 15:37:53 +0200 Ilya Maximets wrote:
>> Is kernel.org blocking the sender somehow?  Does anyone know?
> 
> The patch was submitted with an HTML attachment :(
> Same with the v2 BTW. vger drops all emails with HTML parts.

Hrm, I see.  It's "Content-Type: multipart/alternative;" indeed.
Gmail and apparently outlook(?) likes to send the same message as
both plain text and html.  ovs-dev just drops the html alternative
keeping the plain text, that's why we have the patch there.

@Faicker, you'll need to re-configure your email client to avoid
inclusion of the html alternative parts, I suppose.  Or someone
else will need to re-send your patches, if not possible.  AFAIK,
it's not possible to turn off the multipart/alternative in gmail
web interface, not sure about outlook.

Some useful information and links are available here:
  https://docs.kernel.org/process/email-clients.html
Ideally, git send-email is the preferred client for sending patches.

Best regards, Ilya Maximets.

Re: [PATCH] net: openvswitch: Fix the dead loop of MPLS parse

[Ilya Maximets]

On 5/21/25 6:10 AM, Faicker Mo wrote:
> 
> On 2025/5/20, 18:38, "Ilya Maximets" <i.maximets@ovn.org <mailto:i.maximets@ovn.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.