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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>