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@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?
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.
