On Wed, Mar 13, 2024 at 04:25:28PM +0100, Florian Westphal wrote:
> Sven Auhagen <sven.auhagen@xxxxxxxxxxxx> wrote:
> > On Wed, Mar 13, 2024 at 04:02:03PM +0100, Florian Westphal wrote:
> > > Florian Westphal <fw@xxxxxxxxx> wrote:
> > > > No idea, but it was intentional, see
> > > > b6f27d322a0a ("netfilter: nf_flow_table: tear down TCP flows if RST or FIN was seen")
> > >
> > > Maybe:
> > >
> > > diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
> > > --- a/net/netfilter/nf_flow_table_ip.c
> > > +++ b/net/netfilter/nf_flow_table_ip.c
> > > @@ -28,10 +28,8 @@ static int nf_flow_state_check(struct flow_offload *flow, int proto,
> > > return 0;
> > >
> > > tcph = (void *)(skb_network_header(skb) + thoff);
> > > - if (unlikely(tcph->fin || tcph->rst)) {
> > > - flow_offload_teardown(flow);
> > > + if (unlikely(tcph->fin || tcph->rst))
> > > return -1;
> > > - }
> > >
> > > return 0;
> > > }
> > >
> > > ?
> > >
> > > This will let gc step clean the entry from the flowtable.
> > Thanks for your answer.
> >
> > I double checked and the problem is that the timeout in flow_offload_fixup_ct is set to a very small value
> > and the state is deleted immediately afterwards.
>
> but from where is the call to flow_offload_fixup_ct() made?
It is coming from nf_flow_state_check so your patch is correct in that sense.
It might still lead to the timeout beeing set to 0 during flowtable gc though if the
state is transitioned to e.g. LAST_ACK before the gc is running even with
your patch applied.
I will also try out your patch and it makes sense that it is a race by design.
I think it should be applied as well.
I guess the question is is it save to send more packets throug the flowtable
even after we have seen a fin or rst?
>
> I don't think tearing down the flowtable entry on first fin or rst makes
> any sense, its racy by design.
