[net-next] netfiler: conntrack: Add the option to set ct tcp flag - BE_LIBERAL per-ct basis.

[net-next] netfiler: conntrack: Add the option to set ct tcp flag - BE_LIBERAL per-ct basis.

[nusiddiq]

From: Numan Siddique <nusiddiq@redhat.com>

Before calling nf_conntrack_in(), caller can set this flag in the
connection template for a tcp packet and any errors in the
tcp_in_window() will be ignored.

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.

openvswitch makes use of this feature so that any out of window tcp
packets are not marked invalid. Prior to this there was no easy way
to distinguish if conntracked packet is marked invalid because of
tcp_in_window() check error or because it doesn't belong to an
existing connection.

An earlier attempt (see the link) tried to solve this problem for
openvswitch in a different way. Florian Westphal instead suggested
to be liberal in openvswitch for tcp packets.

Link: https://patchwork.ozlabs.org/project/netdev/patch/20201006083355.121018-1-nusiddiq@redhat.com/

Suggested-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
---
 include/net/netfilter/nf_conntrack_l4proto.h |  6 ++++++
 net/netfilter/nf_conntrack_core.c            | 13 +++++++++++--
 net/openvswitch/conntrack.c                  |  1 +
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_l4proto.h b/include/net/netfilter/nf_conntrack_l4proto.h
index 88186b95b3c2..572ae8d2a622 100644
--- a/include/net/netfilter/nf_conntrack_l4proto.h
+++ b/include/net/netfilter/nf_conntrack_l4proto.h
@@ -203,6 +203,12 @@ static inline struct nf_icmp_net *nf_icmpv6_pernet(struct net *net)
 {
 	return &net->ct.nf_ct_proto.icmpv6;
 }
+
+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;
+}
 #endif
 
 #ifdef CONFIG_NF_CT_PROTO_DCCP
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 234b7cab37c3..8290c5b04e88 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1748,10 +1748,18 @@ static int nf_conntrack_handle_packet(struct nf_conn *ct,
 				      struct sk_buff *skb,
 				      unsigned int dataoff,
 				      enum ip_conntrack_info ctinfo,
-				      const struct nf_hook_state *state)
+				      const struct nf_hook_state *state,
+				      union nf_conntrack_proto *tmpl_proto)
 {
 	switch (nf_ct_protonum(ct)) {
 	case IPPROTO_TCP:
+		if (tmpl_proto) {
+			if (tmpl_proto->tcp.seen[0].flags & IP_CT_TCP_FLAG_BE_LIBERAL)
+				ct->proto.tcp.seen[0].flags |= IP_CT_TCP_FLAG_BE_LIBERAL;
+
+			if (tmpl_proto->tcp.seen[1].flags & IP_CT_TCP_FLAG_BE_LIBERAL)
+				ct->proto.tcp.seen[1].flags |= IP_CT_TCP_FLAG_BE_LIBERAL;
+		}
 		return nf_conntrack_tcp_packet(ct, skb, dataoff,
 					       ctinfo, state);
 	case IPPROTO_UDP:
@@ -1843,7 +1851,8 @@ nf_conntrack_in(struct sk_buff *skb, const struct nf_hook_state *state)
 		goto out;
 	}
 
-	ret = nf_conntrack_handle_packet(ct, skb, dataoff, ctinfo, state);
+	ret = nf_conntrack_handle_packet(ct, skb, dataoff, ctinfo, state,
+					 tmpl ? &tmpl->proto : NULL);
 	if (ret <= 0) {
 		/* Invalid: inverse of the return code tells
 		 * the netfilter core what to do */
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 4beb96139d77..64247be2b1d7 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -969,6 +969,7 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
 			if (skb_nfct(skb))
 				nf_conntrack_put(skb_nfct(skb));
 			nf_conntrack_get(&tmpl->ct_general);
+			nf_ct_set_tcp_be_liberal(tmpl);
 			nf_ct_set(skb, tmpl, IP_CT_NEW);
 		}
 
-- 
2.28.0

Re: [net-next] netfiler: conntrack: Add the option to set ct tcp flag - BE_LIBERAL per-ct basis.

[Jakub Kicinski]

On Mon,  9 Nov 2020 12:59:30 +0530 nusiddiq@redhat.com wrote:
> From: Numan Siddique <nusiddiq@redhat.com>
> 
> Before calling nf_conntrack_in(), caller can set this flag in the
> connection template for a tcp packet and any errors in the
> tcp_in_window() will be ignored.
> 
> 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.
> 
> openvswitch makes use of this feature so that any out of window tcp
> packets are not marked invalid. Prior to this there was no easy way
> to distinguish if conntracked packet is marked invalid because of
> tcp_in_window() check error or because it doesn't belong to an
> existing connection.
> 
> An earlier attempt (see the link) tried to solve this problem for
> openvswitch in a different way. Florian Westphal instead suggested
> to be liberal in openvswitch for tcp packets.
> 
> Link: https://patchwork.ozlabs.org/project/netdev/patch/20201006083355.121018-1-nusiddiq@redhat.com/
> 
> Suggested-by: Florian Westphal <fw@strlen.de>
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>

Please repost Ccing Pablo & netfilter-devel.

Re: [net-next] netfiler: conntrack: Add the option to set ct tcp flag - BE_LIBERAL per-ct basis.

[Numan Siddique]

On Tue, Nov 10, 2020 at 1:25 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon,  9 Nov 2020 12:59:30 +0530 nusiddiq@redhat.com wrote:
> > From: Numan Siddique <nusiddiq@redhat.com>
> >
> > Before calling nf_conntrack_in(), caller can set this flag in the
> > connection template for a tcp packet and any errors in the
> > tcp_in_window() will be ignored.
> >
> > 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.
> >
> > openvswitch makes use of this feature so that any out of window tcp
> > packets are not marked invalid. Prior to this there was no easy way
> > to distinguish if conntracked packet is marked invalid because of
> > tcp_in_window() check error or because it doesn't belong to an
> > existing connection.
> >
> > An earlier attempt (see the link) tried to solve this problem for
> > openvswitch in a different way. Florian Westphal instead suggested
> > to be liberal in openvswitch for tcp packets.
> >
> > Link: https://patchwork.ozlabs.org/project/netdev/patch/20201006083355.121018-1-nusiddiq@redhat.com/
> >
> > Suggested-by: Florian Westphal <fw@strlen.de>
> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
>
> Please repost Ccing Pablo & netfilter-devel.

Thanks. I will repost.

Numan

>

Re: [net-next] netfiler: conntrack: Add the option to set ct tcp flag - BE_LIBERAL per-ct basis.

[Florian Westphal]

nusiddiq@redhat.com <nusiddiq@redhat.com> wrote:
> From: Numan Siddique <nusiddiq@redhat.com>
> 
> Before calling nf_conntrack_in(), caller can set this flag in the
> connection template for a tcp packet and any errors in the
> tcp_in_window() will be ignored.
> 
> 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.
> 
> openvswitch makes use of this feature so that any out of window tcp
> packets are not marked invalid. Prior to this there was no easy way
> to distinguish if conntracked packet is marked invalid because of
> tcp_in_window() check error or because it doesn't belong to an
> existing connection.
> 
> An earlier attempt (see the link) tried to solve this problem for
> openvswitch in a different way. Florian Westphal instead suggested
> to be liberal in openvswitch for tcp packets.
> 
> Link: https://patchwork.ozlabs.org/project/netdev/patch/20201006083355.121018-1-nusiddiq@redhat.com/
> 
> Suggested-by: Florian Westphal <fw@strlen.de>
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> ---
>  include/net/netfilter/nf_conntrack_l4proto.h |  6 ++++++
>  net/netfilter/nf_conntrack_core.c            | 13 +++++++++++--
>  net/openvswitch/conntrack.c                  |  1 +
>  3 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_conntrack_l4proto.h b/include/net/netfilter/nf_conntrack_l4proto.h
> index 88186b95b3c2..572ae8d2a622 100644
> --- a/include/net/netfilter/nf_conntrack_l4proto.h
> +++ b/include/net/netfilter/nf_conntrack_l4proto.h
> @@ -203,6 +203,12 @@ static inline struct nf_icmp_net *nf_icmpv6_pernet(struct net *net)
>  {
>  	return &net->ct.nf_ct_proto.icmpv6;
>  }
> +
> +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;
> +}
>  #endif
>  
>  #ifdef CONFIG_NF_CT_PROTO_DCCP
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 234b7cab37c3..8290c5b04e88 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -1748,10 +1748,18 @@ static int nf_conntrack_handle_packet(struct nf_conn *ct,
>  				      struct sk_buff *skb,
>  				      unsigned int dataoff,
>  				      enum ip_conntrack_info ctinfo,
> -				      const struct nf_hook_state *state)
> +				      const struct nf_hook_state *state,
> +				      union nf_conntrack_proto *tmpl_proto)

I would prefer if we could avoid adding yet another function argument.

AFAICS its enough to call the new nf_ct_set_tcp_be_liberal() helper
before nf_conntrack_confirm() in ovs_ct_commit(), e.g.:

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1235,10 +1235,13 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
        if (!nf_ct_is_confirmed(ct)) {
                err = ovs_ct_init_labels(ct, key, &info->labels.value,
                                         &info->labels.mask);
                if (err)
                        return err;
+
+               if (nf_ct_protonum(ct) == IPPROTO_TCP)
+                       nf_ct_set_tcp_be_liberal(ct);

Re: [net-next] netfiler: conntrack: Add the option to set ct tcp flag - BE_LIBERAL per-ct basis.

[Numan Siddique]

On Tue, Nov 10, 2020 at 3:06 AM Florian Westphal <fw@strlen.de> wrote:
>
> nusiddiq@redhat.com <nusiddiq@redhat.com> wrote:
> > From: Numan Siddique <nusiddiq@redhat.com>
> >
> > Before calling nf_conntrack_in(), caller can set this flag in the
> > connection template for a tcp packet and any errors in the
> > tcp_in_window() will be ignored.
> >
> > 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.
> >
> > openvswitch makes use of this feature so that any out of window tcp
> > packets are not marked invalid. Prior to this there was no easy way
> > to distinguish if conntracked packet is marked invalid because of
> > tcp_in_window() check error or because it doesn't belong to an
> > existing connection.
> >
> > An earlier attempt (see the link) tried to solve this problem for
> > openvswitch in a different way. Florian Westphal instead suggested
> > to be liberal in openvswitch for tcp packets.
> >
> > Link: https://patchwork.ozlabs.org/project/netdev/patch/20201006083355.121018-1-nusiddiq@redhat.com/
> >
> > Suggested-by: Florian Westphal <fw@strlen.de>
> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> > ---
> >  include/net/netfilter/nf_conntrack_l4proto.h |  6 ++++++
> >  net/netfilter/nf_conntrack_core.c            | 13 +++++++++++--
> >  net/openvswitch/conntrack.c                  |  1 +
> >  3 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/net/netfilter/nf_conntrack_l4proto.h b/include/net/netfilter/nf_conntrack_l4proto.h
> > index 88186b95b3c2..572ae8d2a622 100644
> > --- a/include/net/netfilter/nf_conntrack_l4proto.h
> > +++ b/include/net/netfilter/nf_conntrack_l4proto.h
> > @@ -203,6 +203,12 @@ static inline struct nf_icmp_net *nf_icmpv6_pernet(struct net *net)
> >  {
> >       return &net->ct.nf_ct_proto.icmpv6;
> >  }
> > +
> > +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;
> > +}
> >  #endif
> >
> >  #ifdef CONFIG_NF_CT_PROTO_DCCP
> > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> > index 234b7cab37c3..8290c5b04e88 100644
> > --- a/net/netfilter/nf_conntrack_core.c
> > +++ b/net/netfilter/nf_conntrack_core.c
> > @@ -1748,10 +1748,18 @@ static int nf_conntrack_handle_packet(struct nf_conn *ct,
> >                                     struct sk_buff *skb,
> >                                     unsigned int dataoff,
> >                                     enum ip_conntrack_info ctinfo,
> > -                                   const struct nf_hook_state *state)
> > +                                   const struct nf_hook_state *state,
> > +                                   union nf_conntrack_proto *tmpl_proto)
>
> I would prefer if we could avoid adding yet another function argument.
>
> AFAICS its enough to call the new nf_ct_set_tcp_be_liberal() helper
> before nf_conntrack_confirm() in ovs_ct_commit(), e.g.:

Thanks for the comments. I actually tried this approach first, but it
doesn't seem to work.
I noticed that for the committed connections, the ct tcp flag -
IP_CT_TCP_FLAG_BE_LIBERAL is
not set when nf_conntrack_in() calls resolve_normal_ct().

Would you expect that the tcp ct flags should have been preserved once
the connection is committed ?

Thanks
Numan

>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -1235,10 +1235,13 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
>         if (!nf_ct_is_confirmed(ct)) {
>                 err = ovs_ct_init_labels(ct, key, &info->labels.value,
>                                          &info->labels.mask);
>                 if (err)
>                         return err;
> +
> +               if (nf_ct_protonum(ct) == IPPROTO_TCP)
> +                       nf_ct_set_tcp_be_liberal(ct);
>

Re: [net-next] netfiler: conntrack: Add the option to set ct tcp flag - BE_LIBERAL per-ct basis.

[Florian Westphal]

Numan Siddique <nusiddiq@redhat.com> wrote:
> On Tue, Nov 10, 2020 at 3:06 AM Florian Westphal <fw@strlen.de> wrote:
> Thanks for the comments. I actually tried this approach first, but it
> doesn't seem to work.
> I noticed that for the committed connections, the ct tcp flag -
> IP_CT_TCP_FLAG_BE_LIBERAL is
> not set when nf_conntrack_in() calls resolve_normal_ct().

Yes, it won't be set during nf_conntrack_in, thats why I suggested
to do it before confirming the connection.

> Would you expect that the tcp ct flags should have been preserved once
> the connection is committed ?

Yes, they are preserved when you set them after nf_conntrack_in(), else
we would already have trouble with hw flow offloading which needs to
turn off window checks as well.

Re: [net-next] netfiler: conntrack: Add the option to set ct tcp flag - BE_LIBERAL per-ct basis.

[Numan Siddique]

On Tue, Nov 10, 2020 at 5:55 PM Florian Westphal <fw@strlen.de> wrote:
>
> Numan Siddique <nusiddiq@redhat.com> wrote:
> > On Tue, Nov 10, 2020 at 3:06 AM Florian Westphal <fw@strlen.de> wrote:
> > Thanks for the comments. I actually tried this approach first, but it
> > doesn't seem to work.
> > I noticed that for the committed connections, the ct tcp flag -
> > IP_CT_TCP_FLAG_BE_LIBERAL is
> > not set when nf_conntrack_in() calls resolve_normal_ct().
>
> Yes, it won't be set during nf_conntrack_in, thats why I suggested
> to do it before confirming the connection.

Sorry for the confusion. What I mean is - I tested  your suggestion - i.e called
nf_ct_set_tcp_be_liberal()  before calling nf_conntrack_confirm().

 Once the connection is established, for subsequent packets, openvswitch
 calls nf_conntrack_in() [1] to see if the packet is part of the
existing connection or not (i.e ct.new or ct.est )
and if the packet happens to be out-of-window then skb->_nfct is set
to NULL. And the tcp
be flags set during confirmation are not reflected when
nf_conntrack_in() calls resolve_normal_ct().


>
> > Would you expect that the tcp ct flags should have been preserved once
> > the connection is committed ?
>
> Yes, they are preserved when you set them after nf_conntrack_in(), else
> we would already have trouble with hw flow offloading which needs to
> turn off window checks as well.

Looks they are not preserved for the openvswitch case. Probably
openvswitch is doing something wrong.
I will debug further and see what is going on.

Please let me know if you have any further comments.

Thanks
Numan

>

Re: [net-next] netfiler: conntrack: Add the option to set ct tcp flag - BE_LIBERAL per-ct basis.

[Florian Westphal]

Numan Siddique <nusiddiq@redhat.com> wrote:
> On Tue, Nov 10, 2020 at 5:55 PM Florian Westphal <fw@strlen.de> wrote:
> >
> > Numan Siddique <nusiddiq@redhat.com> wrote:
> > > On Tue, Nov 10, 2020 at 3:06 AM Florian Westphal <fw@strlen.de> wrote:
> > > Thanks for the comments. I actually tried this approach first, but it
> > > doesn't seem to work.
> > > I noticed that for the committed connections, the ct tcp flag -
> > > IP_CT_TCP_FLAG_BE_LIBERAL is
> > > not set when nf_conntrack_in() calls resolve_normal_ct().
> >
> > Yes, it won't be set during nf_conntrack_in, thats why I suggested
> > to do it before confirming the connection.
> 
> Sorry for the confusion. What I mean is - I tested  your suggestion - i.e called
> nf_ct_set_tcp_be_liberal()  before calling nf_conntrack_confirm().
> 
>  Once the connection is established, for subsequent packets, openvswitch
>  calls nf_conntrack_in() [1] to see if the packet is part of the
> existing connection or not (i.e ct.new or ct.est )
> and if the packet happens to be out-of-window then skb->_nfct is set
> to NULL. And the tcp
> be flags set during confirmation are not reflected when
> nf_conntrack_in() calls resolve_normal_ct().

Can you debug where this happens?  This looks very very wrong.
resolve_normal_ct() has no business to check any of those flags
(and I don't see where it uses them, it should only deal with the
 tuples).

The flags come into play when nf_conntrack_handle_packet() gets called
after resolve_normal_ct has found an entry, since that will end up
calling the tcp conntrack part.

The entry found/returned by resolve_normal_ct should be the same
nf_conn entry that was confirmed earlier, i.e. it should be in "liberal"
mode.

Re: [net-next] netfiler: conntrack: Add the option to set ct tcp flag - BE_LIBERAL per-ct basis.

[Numan Siddique]

On Tue, Nov 10, 2020 at 6:41 PM Florian Westphal <fw@strlen.de> wrote:
>
> Numan Siddique <nusiddiq@redhat.com> wrote:
> > On Tue, Nov 10, 2020 at 5:55 PM Florian Westphal <fw@strlen.de> wrote:
> > >
> > > Numan Siddique <nusiddiq@redhat.com> wrote:
> > > > On Tue, Nov 10, 2020 at 3:06 AM Florian Westphal <fw@strlen.de> wrote:
> > > > Thanks for the comments. I actually tried this approach first, but it
> > > > doesn't seem to work.
> > > > I noticed that for the committed connections, the ct tcp flag -
> > > > IP_CT_TCP_FLAG_BE_LIBERAL is
> > > > not set when nf_conntrack_in() calls resolve_normal_ct().
> > >
> > > Yes, it won't be set during nf_conntrack_in, thats why I suggested
> > > to do it before confirming the connection.
> >
> > Sorry for the confusion. What I mean is - I tested  your suggestion - i.e called
> > nf_ct_set_tcp_be_liberal()  before calling nf_conntrack_confirm().
> >
> >  Once the connection is established, for subsequent packets, openvswitch
> >  calls nf_conntrack_in() [1] to see if the packet is part of the
> > existing connection or not (i.e ct.new or ct.est )
> > and if the packet happens to be out-of-window then skb->_nfct is set
> > to NULL. And the tcp
> > be flags set during confirmation are not reflected when
> > nf_conntrack_in() calls resolve_normal_ct().
>
> Can you debug where this happens?  This looks very very wrong.
> resolve_normal_ct() has no business to check any of those flags
> (and I don't see where it uses them, it should only deal with the
>  tuples).
>
> The flags come into play when nf_conntrack_handle_packet() gets called
> after resolve_normal_ct has found an entry, since that will end up
> calling the tcp conntrack part.
>
> The entry found/returned by resolve_normal_ct should be the same
> nf_conn entry that was confirmed earlier, i.e. it should be in "liberal"
> mode.

I debugged a bit. Calling nf_ct_set_tcp_be_liberal() in ct_commit
doesn't work because
  - the first SYN packet during connection establishment is committed
to the contract.
  - but tcp_in_window() calls tcp_options() which clears the tcp ct
flags for the SYN and SYN-ACK packets.
    And hence the flags get cleared.

So I think it should be enough if openvswitch calls
nf_ct_set_tcp_be_liberal() once the connection is established.


I posted v2. Request to take a look.

Thanks
Numan

>