On Sat, 27 Apr 2019 at 02:03, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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!
