| Message ID | 1644594737-25324-2-git-send-email-bodong@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Pass originating device to drivers offloading ct connection | expand |
Hi Bodong, On 11.02.22 16:52, Bodong Wang wrote: > From: Numan Siddique <nusiddiq@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@strlen.de> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > Acked-by: Florian Westphal <fw@strlen.de> > Link: https://lore.kernel.org/r/20201116130126.3065077-1-nusiddiq@redhat.com > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Paul Blakey <paulb@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@strlen.de> Signed-off-by: Numan Siddique <nusiddiq@redhat.com> Acked-by: Florian Westphal <fw@strlen.de> Link: https://lore.kernel.org/r/20201116130126.3065077-1-nusiddiq@redhat.com Signed-off-by: Jakub Kicinski <kuba@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@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;
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@canonical.com> Sent: Friday, February 18, 2022 9:39 AM To: Bodong Wang <bodong@nvidia.com>; kernel-team@lists.ubuntu.com Cc: Vladimir Sokolovsky <vlad@nvidia.com>; Paul Blakey <paulb@nvidia.com>; Maor Dickman <maord@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@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@strlen.de> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > Acked-by: Florian Westphal <fw@strlen.de> > Link: > https://lore.kernel.org/r/20201116130126.3065077-1-nusiddiq@redhat.com > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Paul Blakey <paulb@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@strlen.de> Signed-off-by: Numan Siddique <nusiddiq@redhat.com> Acked-by: Florian Westphal <fw@strlen.de> Link: https://lore.kernel.org/r/20201116130126.3065077-1-nusiddiq@redhat.com Signed-off-by: Jakub Kicinski <kuba@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@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;
Hi Bodong, On 18.02.22 16:46, Bodong Wang wrote: > 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. OK, that's more clear to me now what process you are following. Thanks for the clarification. When we state that we are cherry-picking or backporting a patch from upstream/mainline, we assume that the patch was taken from that particular tree and the provenance block should be kept intact before the "(cherry picked|backported from ...") line. So if Paul Blakey did the cherry-pick/backport, the provenance block would look like: Suggested-by: Florian Westphal <fw@strlen.de> Signed-off-by: Numan Siddique <nusiddiq@redhat.com> Acked-by: Florian Westphal <fw@strlen.de> Link: https://lore.kernel.org/r/20201116130126.3065077-1-nusiddiq@redhat.com Signed-off-by: Jakub Kicinski <kuba@kernel.org> (cherry picked from commit e2ef5203c817a60bfb591343ffd851b6537370ff) Signed-off-by: Paul Blakey <paulb@nvidia.com> Signed-off-by: Bodong Wang <bodong@nvidia.com> Please note that when someone is part of the work of cherry-picking/backporting/ submitting a patch to an Ubuntu kernel, the signed-off-by should be added at the bottom even if the person is the original author of that patch. For patch 2/3 ("net/sched: act_ct: Fill offloading tuple iifidx"), this would be: Signed-off-by: Oz Shlomo <ozsh@nvidia.com> Signed-off-by: Paul Blakey <paulb@nvidia.com> Signed-off-by: David S. Miller <davem@davemloft.net> (cherry picked from commit 9795ded7f924b6486e54976619b1b094fcc1969d) Signed-off-by: Paul Blakey <paulb@nvidia.com> Signed-off-by: Bodong Wang <bodong@nvidia.com> Note that in the patch submitted with this thread it was missing the s-o-b of the upstream maintainer (David Miller). I hope this helps and sorry for the nitpicking, this is needed to keep the complete history and origin of the patch. Thanks, Kleber > > Sorry for not bottom posting... > > Boodng > > -----Original Message----- > From: Kleber Souza <kleber.souza@canonical.com> > Sent: Friday, February 18, 2022 9:39 AM > To: Bodong Wang <bodong@nvidia.com>; kernel-team@lists.ubuntu.com > Cc: Vladimir Sokolovsky <vlad@nvidia.com>; Paul Blakey <paulb@nvidia.com>; Maor Dickman <maord@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@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@strlen.de> >> Signed-off-by: Numan Siddique <nusiddiq@redhat.com> >> Acked-by: Florian Westphal <fw@strlen.de> >> Link: >> https://lore.kernel.org/r/20201116130126.3065077-1-nusiddiq@redhat.com >> Signed-off-by: Jakub Kicinski <kuba@kernel.org> >> Signed-off-by: Paul Blakey <paulb@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@strlen.de> > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > > Acked-by: Florian Westphal <fw@strlen.de> > > Link: https://lore.kernel.org/r/20201116130126.3065077-1-nusiddiq@redhat.com > > Signed-off-by: Jakub Kicinski <kuba@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@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; >
On 2/18/2022 10:56 AM, Kleber Souza wrote: > Hi Bodong, > > On 18.02.22 16:46, Bodong Wang wrote: >> 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. > > OK, that's more clear to me now what process you are following. Thanks > for the > clarification. > > When we state that we are cherry-picking or backporting a patch from > upstream/mainline, > we assume that the patch was taken from that particular tree and the > provenance block > should be kept intact before the "(cherry picked|backported from ...") > line. > > So if Paul Blakey did the cherry-pick/backport, the provenance block > would > look like: > > Suggested-by: Florian Westphal <fw@strlen.de> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > Acked-by: Florian Westphal <fw@strlen.de> > Link: > https://lore.kernel.org/r/20201116130126.3065077-1-nusiddiq@redhat.com > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > (cherry picked from commit e2ef5203c817a60bfb591343ffd851b6537370ff) > Signed-off-by: Paul Blakey <paulb@nvidia.com> > Signed-off-by: Bodong Wang <bodong@nvidia.com> > > Please note that when someone is part of the work of > cherry-picking/backporting/ > submitting a patch to an Ubuntu kernel, the signed-off-by should be > added at the bottom > even if the person is the original author of that patch. > > For patch 2/3 ("net/sched: act_ct: Fill offloading tuple iifidx"), > this would be: > > Signed-off-by: Oz Shlomo <ozsh@nvidia.com> > > Signed-off-by: Paul Blakey <paulb@nvidia.com> > > Signed-off-by: David S. Miller <davem@davemloft.net> > > (cherry picked from commit 9795ded7f924b6486e54976619b1b094fcc1969d) > Signed-off-by: Paul Blakey <paulb@nvidia.com> > Signed-off-by: Bodong Wang <bodong@nvidia.com> > > Note that in the patch submitted with this thread it was missing the > s-o-b of the > upstream maintainer (David Miller). > > I hope this helps and sorry for the nitpicking, this is needed to keep > the > complete history and origin of the patch. > > Thanks, > Kleber > Fixed and sent V2, please review.
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;