[PATCH nf 2/4] netfilter: nft_flow_offload: do not make un-established tcp flow_offload session.

[PATCH nf 2/4] netfilter: nft_flow_offload: do not make un-established tcp flow_offload session.

[Taehee Yoo]

nft_flow_offload_eval() makes flow_offload session but it doesn't check
tcp state.
So, it can make un-ESTABLISHED tcp flow offload session such as SYN-RECV.
But, this is not a normal case.

Fixes: a3c90f7a2323 ("netfilter: nf_tables: flow offload expression")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
 net/netfilter/nft_flow_offload.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
index ff50bc1b144f..8538ddf9c6bf 100644
--- a/net/netfilter/nft_flow_offload.c
+++ b/net/netfilter/nft_flow_offload.c
@@ -84,6 +84,9 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
 
 	switch (ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.protonum) {
 	case IPPROTO_TCP:
+		if (ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED)
+			goto out;
+		break;
 	case IPPROTO_UDP:
 		break;
 	default:
-- 
2.17.1

Re: [PATCH nf 2/4] netfilter: nft_flow_offload: do not make un-established tcp flow_offload session.

[Pablo Neira Ayuso]

On Sat, Apr 27, 2019 at 01:53:27AM +0900, Taehee Yoo wrote:
> nft_flow_offload_eval() makes flow_offload session but it doesn't check
> tcp state.
> So, it can make un-ESTABLISHED tcp flow offload session such as SYN-RECV.
> But, this is not a normal case.
> 
> Fixes: a3c90f7a2323 ("netfilter: nf_tables: flow offload expression")
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> ---
>  net/netfilter/nft_flow_offload.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
> index ff50bc1b144f..8538ddf9c6bf 100644
> --- a/net/netfilter/nft_flow_offload.c
> +++ b/net/netfilter/nft_flow_offload.c
> @@ -84,6 +84,9 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
>  
>  	switch (ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.protonum) {
>  	case IPPROTO_TCP:
> +		if (ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED)
> +			goto out;

You can restrict this via policy:

        ct status assured flow add @x

I've been considering to remove this defensive check

diff --git a/net/netfilter/nft_flow_offload.c
b/net/netfilter/nft_flow_offload.c
index 6e6b9adf7d38..bb8ea4cefc34 100644
--- a/net/netfilter/nft_flow_offload.c
+++ b/net/netfilter/nft_flow_offload.c
@@ -94,10 +94,6 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
        if (help)
                goto out;
 
-       if (ctinfo == IP_CT_NEW ||
-           ctinfo == IP_CT_RELATED)
-               goto out;
-
        if (test_and_set_bit(IPS_OFFLOAD_BIT, &ct->status))
                goto out;

Given that we can restrict this via policy, ie.

        ct state established flow add @x

And this would fix the flowtable infrastructure UDP traffic that only
goes in one direction.

We can document in the manpage a few examples for this.

Re: [PATCH nf 2/4] netfilter: nft_flow_offload: do not make un-established tcp flow_offload session.

[Taehee Yoo]

On Sat, 27 Apr 2019 at 02:03, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>

Hi Pablo!
Thank you for the review!

> On Sat, Apr 27, 2019 at 01:53:27AM +0900, Taehee Yoo wrote:
> > nft_flow_offload_eval() makes flow_offload session but it doesn't check
> > tcp state.
> > So, it can make un-ESTABLISHED tcp flow offload session such as SYN-RECV.
> > But, this is not a normal case.
> >
> > Fixes: a3c90f7a2323 ("netfilter: nf_tables: flow offload expression")
> > Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> > ---
> >  net/netfilter/nft_flow_offload.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
> > index ff50bc1b144f..8538ddf9c6bf 100644
> > --- a/net/netfilter/nft_flow_offload.c
> > +++ b/net/netfilter/nft_flow_offload.c
> > @@ -84,6 +84,9 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
> >
> >       switch (ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.protonum) {
> >       case IPPROTO_TCP:
> > +             if (ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED)
> > +                     goto out;
>
> You can restrict this via policy:
>
>         ct status assured flow add @x
>

I think If we allow to make non-ESTABLISHED flow offload,
teardown code should be changed because teardown changes TCP status to
ESTABLISHED.
How do you think about it?

> I've been considering to remove this defensive check
>
> diff --git a/net/netfilter/nft_flow_offload.c
> b/net/netfilter/nft_flow_offload.c
> index 6e6b9adf7d38..bb8ea4cefc34 100644
> --- a/net/netfilter/nft_flow_offload.c
> +++ b/net/netfilter/nft_flow_offload.c
> @@ -94,10 +94,6 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
>         if (help)
>                 goto out;
>
> -       if (ctinfo == IP_CT_NEW ||
> -           ctinfo == IP_CT_RELATED)
> -               goto out;
> -
>         if (test_and_set_bit(IPS_OFFLOAD_BIT, &ct->status))
>                 goto out;
>
> Given that we can restrict this via policy, ie.
>
>         ct state established flow add @x
>
> And this would fix the flowtable infrastructure UDP traffic that only
> goes in one direction.
>
> We can document in the manpage a few examples for this.

I agree that flowtable infrastructure provides UDP one direction flow
offloading.

Thanks!

Re: [PATCH nf 2/4] netfilter: nft_flow_offload: do not make un-established tcp flow_offload session.

[Pablo Neira Ayuso]

On Sat, Apr 27, 2019 at 02:48:15AM +0900, Taehee Yoo wrote:
> On Sat, 27 Apr 2019 at 02:03, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> 
> Hi Pablo!
> Thank you for the review!
> 
> > On Sat, Apr 27, 2019 at 01:53:27AM +0900, Taehee Yoo wrote:
> > > nft_flow_offload_eval() makes flow_offload session but it doesn't check
> > > tcp state.
> > > So, it can make un-ESTABLISHED tcp flow offload session such as SYN-RECV.
> > > But, this is not a normal case.
> > >
> > > Fixes: a3c90f7a2323 ("netfilter: nf_tables: flow offload expression")
> > > Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> > > ---
> > >  net/netfilter/nft_flow_offload.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
> > > index ff50bc1b144f..8538ddf9c6bf 100644
> > > --- a/net/netfilter/nft_flow_offload.c
> > > +++ b/net/netfilter/nft_flow_offload.c
> > > @@ -84,6 +84,9 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
> > >
> > >       switch (ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.protonum) {
> > >       case IPPROTO_TCP:
> > > +             if (ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED)
> > > +                     goto out;
> >
> > You can restrict this via policy:
> >
> >         ct status assured flow add @x
> >
> 
> I think If we allow to make non-ESTABLISHED flow offload,
> teardown code should be changed because teardown changes TCP status to
> ESTABLISHED. How do you think about it?

Hm, right, the teardown fixup routine cannot then bogusly assumes the
TCP established state if we create a TCP entry from the first packet,
ie. the SYN_SENT state...

[...]
> > And this would fix the flowtable infrastructure UDP traffic that only
> > goes in one direction.
> >
> > We can document in the manpage a few examples for this.
> 
> I agree that flowtable infrastructure provides UDP one direction flow
> offloading.

OK, probably I can collapse your patch to mine. Let me think over the
weekend.

Re: [PATCH nf 2/4] netfilter: nft_flow_offload: do not make un-established tcp flow_offload session.

[Taehee Yoo]

On Sat, 27 Apr 2019 at 04:02, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Sat, Apr 27, 2019 at 02:48:15AM +0900, Taehee Yoo wrote:
> > On Sat, 27 Apr 2019 at 02:03, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >
> >
> > Hi Pablo!
> > Thank you for the review!
> >
> > > On Sat, Apr 27, 2019 at 01:53:27AM +0900, Taehee Yoo wrote:
> > > > nft_flow_offload_eval() makes flow_offload session but it doesn't check
> > > > tcp state.
> > > > So, it can make un-ESTABLISHED tcp flow offload session such as SYN-RECV.
> > > > But, this is not a normal case.
> > > >
> > > > Fixes: a3c90f7a2323 ("netfilter: nf_tables: flow offload expression")
> > > > Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> > > > ---
> > > >  net/netfilter/nft_flow_offload.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
> > > > index ff50bc1b144f..8538ddf9c6bf 100644
> > > > --- a/net/netfilter/nft_flow_offload.c
> > > > +++ b/net/netfilter/nft_flow_offload.c
> > > > @@ -84,6 +84,9 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
> > > >
> > > >       switch (ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.protonum) {
> > > >       case IPPROTO_TCP:
> > > > +             if (ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED)
> > > > +                     goto out;
> > >
> > > You can restrict this via policy:
> > >
> > >         ct status assured flow add @x
> > >
> >
> > I think If we allow to make non-ESTABLISHED flow offload,
> > teardown code should be changed because teardown changes TCP status to
> > ESTABLISHED. How do you think about it?
>
> Hm, right, the teardown fixup routine cannot then bogusly assumes the
> TCP established state if we create a TCP entry from the first packet,
> ie. the SYN_SENT state...
>

Thank you for sharing opinion.

> [...]
> > > And this would fix the flowtable infrastructure UDP traffic that only
> > > goes in one direction.
> > >
> > > We can document in the manpage a few examples for this.
> >
> > I agree that flowtable infrastructure provides UDP one direction flow
> > offloading.
>
> OK, probably I can collapse your patch to mine. Let me think over the
> weekend.

Okay, I will wait for your response!