NAK/cmnt: [SRU][F:linux-bluefield][PATCH V1 1/3] net: openvswitch: Be liberal in tcp conntrack.

[Bodong Wang]

Hi Kleber,

Could you be more specific for the misalignment? I can only tell Paul added his own signoff which is expected as he did the backport.

Sorry for not bottom posting...

Boodng

-----Original Message-----
From: Kleber Souza <kleber.souza at canonical.com> 
Sent: Friday, February 18, 2022 9:39 AM
To: Bodong Wang <bodong at nvidia.com>; kernel-team at lists.ubuntu.com
Cc: Vladimir Sokolovsky <vlad at nvidia.com>; Paul Blakey <paulb at nvidia.com>; Maor Dickman <maord at nvidia.com>
Subject: NAK/cmnt: [SRU][F:linux-bluefield][PATCH V1 1/3] net: openvswitch: Be liberal in tcp conntrack.

Hi Bodong,

On 11.02.22 16:52, Bodong Wang wrote:
> From: Numan Siddique <nusiddiq at redhat.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1960575
> 
> There is no easy way to distinguish if a conntracked tcp packet is 
> marked invalid because of tcp_in_window() check error or because it 
> doesn't belong to an existing connection. With this patch, openvswitch 
> sets liberal tcp flag for the established sessions so that out of 
> window packets are not marked invalid.
> 
> A helper function - nf_ct_set_tcp_be_liberal(nf_conn) is added which 
> sets this flag for both the directions of the nf_conn.
> 
> Suggested-by: Florian Westphal <fw at strlen.de>
> Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
> Acked-by: Florian Westphal <fw at strlen.de>
> Link: 
> https://lore.kernel.org/r/20201116130126.3065077-1-nusiddiq@redhat.com
> Signed-off-by: Jakub Kicinski <kuba at kernel.org>
> Signed-off-by: Paul Blakey <paulb at nvidia.com> (cherry picked from 
> commit e2ef5203c817a60bfb591343ffd851b6537370ff)

These patches are not matching the mainline provenance block. For this patch, the original provenance is:

Suggested-by: Florian Westphal <fw at strlen.de>

Signed-off-by: Numan Siddique <nusiddiq at redhat.com>

Acked-by: Florian Westphal <fw at strlen.de>

Link: https://lore.kernel.org/r/20201116130126.3065077-1-nusiddiq@redhat.com

Signed-off-by: Jakub Kicinski <kuba at kernel.org>


The other two patches of this set seem to be missing the Signed-off-by of the upstream maintainer.

When taking a patch from upstream/mainline the original block should be kept, only adding additional information below it.

Could you please re-submit these patches with the block fixed?


Thank you,
Kleber

> Signed-off-by: Bodong Wang <bodong at nvidia.com>
> ---
>   include/net/netfilter/nf_conntrack_l4proto.h | 14 ++++++++++++++
>   net/netfilter/nf_conntrack_proto_tcp.c       |  6 ------
>   net/openvswitch/conntrack.c                  |  8 ++++++++
>   3 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_conntrack_l4proto.h 
> b/include/net/netfilter/nf_conntrack_l4proto.h
> index 4cad1f0..dc9bba6 100644
> --- a/include/net/netfilter/nf_conntrack_l4proto.h
> +++ b/include/net/netfilter/nf_conntrack_l4proto.h
> @@ -201,6 +201,20 @@ static inline struct nf_icmp_net *nf_icmpv6_pernet(struct net *net)
>   {
>   	return &net->ct.nf_ct_proto.icmpv6;
>   }
> +
> +/* Caller must check nf_ct_protonum(ct) is IPPROTO_TCP before 
> +calling. */ static inline void nf_ct_set_tcp_be_liberal(struct 
> +nf_conn *ct) {
> +	ct->proto.tcp.seen[0].flags |= IP_CT_TCP_FLAG_BE_LIBERAL;
> +	ct->proto.tcp.seen[1].flags |= IP_CT_TCP_FLAG_BE_LIBERAL; }
> +
> +/* Caller must check nf_ct_protonum(ct) is IPPROTO_TCP before 
> +calling. */ static inline bool nf_conntrack_tcp_established(const 
> +struct nf_conn *ct) {
> +	return ct->proto.tcp.state == TCP_CONNTRACK_ESTABLISHED &&
> +	       test_bit(IPS_ASSURED_BIT, &ct->status); }
>   #endif
>   
>   #ifdef CONFIG_NF_CT_PROTO_DCCP
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c 
> b/net/netfilter/nf_conntrack_proto_tcp.c
> index dc0fe06..d8e554c 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -834,12 +834,6 @@ static noinline bool tcp_new(struct nf_conn *ct, const struct sk_buff *skb,
>   	return true;
>   }
>   
> -static bool nf_conntrack_tcp_established(const struct nf_conn *ct) -{
> -	return ct->proto.tcp.state == TCP_CONNTRACK_ESTABLISHED &&
> -	       test_bit(IPS_ASSURED_BIT, &ct->status);
> -}
> -
>   /* Returns verdict for packet, or -1 for invalid. */
>   int nf_conntrack_tcp_packet(struct nf_conn *ct,
>   			    struct sk_buff *skb,
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c 
> index 45e9a59..4a08ebf 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -1034,6 +1034,14 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
>   		    ovs_ct_helper(skb, info->family) != NF_ACCEPT) {
>   			return -EINVAL;
>   		}
> +
> +		if (nf_ct_protonum(ct) == IPPROTO_TCP &&
> +		    nf_ct_is_confirmed(ct) && nf_conntrack_tcp_established(ct)) {
> +			/* Be liberal for tcp packets so that out-of-window
> +			 * packets are not marked invalid.
> +			 */
> +			nf_ct_set_tcp_be_liberal(ct);
> +		}
>   	}
>   
>   	return 0;