[PATCH net] net: openvswitch: fix parsing of nw_proto for IPv6 fragments

[PATCH net] net: openvswitch: fix parsing of nw_proto for IPv6 fragments

[Rosemarie O'Riorden]

When a packet enters the OVS datapath and does not match any existing
flows installed in the kernel flow cache, the packet will be sent to
userspace to be parsed, and a new flow will be created. The kernel and
OVS rely on each other to parse packet fields in the same way so that
packets will be handled properly.

As per the design document linked below, OVS expects all later IPv6
fragments to have nw_proto=44 in the flow key, so they can be correctly
matched on OpenFlow rules. OpenFlow controllers create pipelines based
on this design.

This behavior was changed by the commit in the Fixes tag so that
nw_proto equals the next_header field of the last extension header.
However, there is no counterpart for this change in OVS userspace,
meaning that this field is parsed differently between OVS and the
kernel. This is a problem because OVS creates actions based on what is
parsed in userspace, but the kernel-provided flow key is used as a match
criteria, as described in Documentation/networking/openvswitch.rst. This
leads to issues such as packets incorrectly matching on a flow and thus
the wrong list of actions being applied to the packet. Such changes in
packet parsing cannot be implemented without breaking the userspace.

The offending commit is partially reverted to restore the expected
behavior.

The change technically made sense and there is a good reason that it was
implemented, but it does not comply with the original design of OVS.
If in the future someone wants to implement such a change, then it must
be user-configurable and disabled by default to preserve backwards
compatibility with existing OVS versions.

Cc: stable@vger.kernel.org
Fixes: fa642f08839b ("openvswitch: Derive IP protocol number for IPv6 later frags")
Link: https://docs.openvswitch.org/en/latest/topics/design/#fragments
Signed-off-by: Rosemarie O'Riorden <roriorden@redhat.com>
---
 net/openvswitch/flow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 372bf54a0ca9..e20d1a973417 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -407,7 +407,7 @@ static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
 	if (flags & IP6_FH_F_FRAG) {
 		if (frag_off) {
 			key->ip.frag = OVS_FRAG_TYPE_LATER;
-			key->ip.proto = nexthdr;
+			key->ip.proto = NEXTHDR_FRAGMENT;
 			return 0;
 		}
 		key->ip.frag = OVS_FRAG_TYPE_FIRST;
-- 
2.35.3

Re: [ovs-dev] [PATCH net] net: openvswitch: fix parsing of nw_proto for IPv6 fragments

[Eelco Chaudron]

On 21 Jun 2022, at 22:48, Rosemarie O'Riorden wrote:

> When a packet enters the OVS datapath and does not match any existing
> flows installed in the kernel flow cache, the packet will be sent to
> userspace to be parsed, and a new flow will be created. The kernel and
> OVS rely on each other to parse packet fields in the same way so that
> packets will be handled properly.
>
> As per the design document linked below, OVS expects all later IPv6
> fragments to have nw_proto=44 in the flow key, so they can be correctly
> matched on OpenFlow rules. OpenFlow controllers create pipelines based
> on this design.
>
> This behavior was changed by the commit in the Fixes tag so that
> nw_proto equals the next_header field of the last extension header.
> However, there is no counterpart for this change in OVS userspace,
> meaning that this field is parsed differently between OVS and the
> kernel. This is a problem because OVS creates actions based on what is
> parsed in userspace, but the kernel-provided flow key is used as a match
> criteria, as described in Documentation/networking/openvswitch.rst. This
> leads to issues such as packets incorrectly matching on a flow and thus
> the wrong list of actions being applied to the packet. Such changes in
> packet parsing cannot be implemented without breaking the userspace.
>
> The offending commit is partially reverted to restore the expected
> behavior.
>
> The change technically made sense and there is a good reason that it was
> implemented, but it does not comply with the original design of OVS.
> If in the future someone wants to implement such a change, then it must
> be user-configurable and disabled by default to preserve backwards
> compatibility with existing OVS versions.
>
> Cc: stable@vger.kernel.org
> Fixes: fa642f08839b ("openvswitch: Derive IP protocol number for IPv6 later frags")
> Link: https://docs.openvswitch.org/en/latest/topics/design/#fragments
> Signed-off-by: Rosemarie O'Riorden <roriorden@redhat.com>
> ---
>  net/openvswitch/flow.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index 372bf54a0ca9..e20d1a973417 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -407,7 +407,7 @@ static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
>  	if (flags & IP6_FH_F_FRAG) {
>  		if (frag_off) {
>  			key->ip.frag = OVS_FRAG_TYPE_LATER;
> -			key->ip.proto = nexthdr;
> +			key->ip.proto = NEXTHDR_FRAGMENT;
>  			return 0;
>  		}
>  		key->ip.frag = OVS_FRAG_TYPE_FIRST;
> -- 
> 2.35.3

Thanks Rosemarie, for fixing this. The change looks good to me!

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

Re: [PATCH net] net: openvswitch: fix parsing of nw_proto for IPv6 fragments

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Paolo Abeni <pabeni@redhat.com>:

On Tue, 21 Jun 2022 16:48:45 -0400 you wrote:
> When a packet enters the OVS datapath and does not match any existing
> flows installed in the kernel flow cache, the packet will be sent to
> userspace to be parsed, and a new flow will be created. The kernel and
> OVS rely on each other to parse packet fields in the same way so that
> packets will be handled properly.
> 
> As per the design document linked below, OVS expects all later IPv6
> fragments to have nw_proto=44 in the flow key, so they can be correctly
> matched on OpenFlow rules. OpenFlow controllers create pipelines based
> on this design.
> 
> [...]

Here is the summary with links:
  - [net] net: openvswitch: fix parsing of nw_proto for IPv6 fragments
    https://git.kernel.org/netdev/net/c/12378a5a75e3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html