Message-ID: <E12AC26C-AB1F-489C-9BC0-A35593FD03D5@redhat.com>
Date: Wed, 12 Nov 2025 14:28:58 +0100
From: Eelco Chaudron <echaudro@...hat.com>
To: Ilya Maximets <i.maximets@....org>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
 linux-kernel@...r.kernel.org, dev@...nvswitch.org,
 Aaron Conole <aconole@...hat.com>, Willy Tarreau <w@....eu>,
 LePremierHomme <kwqcheii@...ton.me>, Junvy Yang <zhuque@...cent.com>
Subject: Re: [PATCH net] net: openvswitch: remove never-working support for
 setting nsh fields



On 12 Nov 2025, at 12:14, Ilya Maximets wrote:

> The validation of the set(nsh(...)) action is completely wrong.
> It runs through the nsh_key_put_from_nlattr() function that is the
> same function that validates NSH keys for the flow match and the
> push_nsh() action.  However, the set(nsh(...)) has a very different
> memory layout.  Nested attributes in there are doubled in size in
> case of the masked set().  That makes proper validation impossible.
>
> There is also confusion in the code between the 'masked' flag, that
> says that the nested attributes are doubled in size containing both
> the value and the mask, and the 'is_mask' that says that the value
> we're parsing is the mask.  This is causing kernel crash on trying to
> write into mask part of the match with SW_FLOW_KEY_PUT() during
> validation, while validate_nsh() doesn't allocate any memory for it:
>
>   BUG: kernel NULL pointer dereference, address: 0000000000000018
>   #PF: supervisor read access in kernel mode
>   #PF: error_code(0x0000) - not-present page
>   PGD 1c2383067 P4D 1c2383067 PUD 20b703067 PMD 0
>   Oops: Oops: 0000 [#1] SMP NOPTI
>   CPU: 8 UID: 0 Kdump: loaded Not tainted 6.17.0-rc4+ #107 PREEMPT(voluntary)
>   RIP: 0010:nsh_key_put_from_nlattr+0x19d/0x610 [openvswitch]
>   Call Trace:
>    <TASK>
>    validate_nsh+0x60/0x90 [openvswitch]
>    validate_set.constprop.0+0x270/0x3c0 [openvswitch]
>    __ovs_nla_copy_actions+0x477/0x860 [openvswitch]
>    ovs_nla_copy_actions+0x8d/0x100 [openvswitch]
>    ovs_packet_cmd_execute+0x1cc/0x310 [openvswitch]
>    genl_family_rcv_msg_doit+0xdb/0x130
>    genl_family_rcv_msg+0x14b/0x220
>    genl_rcv_msg+0x47/0xa0
>    netlink_rcv_skb+0x53/0x100
>    genl_rcv+0x24/0x40
>    netlink_unicast+0x280/0x3b0
>    netlink_sendmsg+0x1f7/0x430
>    ____sys_sendmsg+0x36b/0x3a0
>    ___sys_sendmsg+0x87/0xd0
>    __sys_sendmsg+0x6d/0xd0
>    do_syscall_64+0x7b/0x2c0
>    entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> The third issue with this process is that while trying to convert
> the non-masked set into masked one, validate_set() copies and doubles
> the size of the OVS_KEY_ATTR_NSH as if it didn't have any nested
> attributes.  It should be copying each nested attribute and doubling
> them in size independently.  And the process must be properly reversed
> during the conversion back from masked to a non-masked variant during
> the flow dump.
>
> In the end, the only two outcomes of trying to use this action are
> either validation failure or a kernel crash.  And if somehow someone
> manages to install a flow with such an action, it will most definitely
> not do what it is supposed to, since all the keys and the masks are
> mixed up.
>
> Fixing all the issues is a complex task as it requires re-writing
> most of the validation code.
>
> Given that and the fact that this functionality never worked since
> introduction, let's just remove it altogether.  It's better to
> re-introduce it later with a proper implementation instead of trying
> to fix it in stable releases.
>
> Fixes: b2d0f5d5dc53 ("openvswitch: enable NSH support")
> Reported-by: Junvy Yang <zhuque@...cent.com>
> Signed-off-by: Ilya Maximets <i.maximets@....org>

Hi Ilya, thanks for taking the time to look into this issue.

Acked-by: Eelco Chaudron <echaudro@...hat.com>

Powered by blists - more mailing lists