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

[Taehee Yoo <ap420073@gmail.com>]

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!