On Wed, Mar 20, 2024 at 10:20:29AM +0100, Sven Auhagen wrote:
> On Wed, Mar 20, 2024 at 10:07:24AM +0100, Pablo Neira Ayuso wrote:
> > On Wed, Mar 20, 2024 at 09:49:49AM +0100, Sven Auhagen wrote:
> > > On Wed, Mar 20, 2024 at 09:45:16AM +0100, Pablo Neira Ayuso wrote:
> > > > Hi Sven,
> > > >
> > > > On Wed, Mar 20, 2024 at 09:39:16AM +0100, Sven Auhagen wrote:
> > > > > On Mon, Mar 18, 2024 at 10:39:15AM +0100, Pablo Neira Ayuso wrote:
> > > > [...]
> > > > > > diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
> > > > > > index a0571339239c..481fe3d96bbc 100644
> > > > > > --- a/net/netfilter/nf_flow_table_core.c
> > > > > > +++ b/net/netfilter/nf_flow_table_core.c
> > > > > > @@ -165,10 +165,22 @@ void flow_offload_route_init(struct flow_offload *flow,
> > > > > > }
> > > > > > EXPORT_SYMBOL_GPL(flow_offload_route_init);
> > > > > >
> > > > > > -static void flow_offload_fixup_tcp(struct ip_ct_tcp *tcp)
> > > > > > +static s32 flow_offload_fixup_tcp(struct net *net, struct nf_conn *ct,
> > > > > > + enum tcp_conntrack tcp_state)
> > > > > > {
> > > > > > - tcp->seen[0].td_maxwin = 0;
> > > > > > - tcp->seen[1].td_maxwin = 0;
> > > > > > + struct nf_tcp_net *tn = nf_tcp_pernet(net);
> > > > > > +
> > > > > > + ct->proto.tcp.state = tcp_state;
> > > > > > + ct->proto.tcp.seen[0].td_maxwin = 0;
> > > > > > + ct->proto.tcp.seen[1].td_maxwin = 0;
> > > > > > +
> > > > > > + /* Similar to mid-connection pickup with loose=1.
> > > > > > + * Avoid large ESTABLISHED timeout.
> > > > > > + */
> > > > > > + if (tcp_state == TCP_CONNTRACK_ESTABLISHED)
> > > > > > + return tn->timeouts[TCP_CONNTRACK_UNACK];
> > > > >
> > > > > Hi Pablo,
> > > > >
> > > > > I tested the patch but the part that sets the timout to UNACK is not
> > > > > very practical.
> > > > > For example my long running SSH connections get killed off by the firewall
> > > > > regularly now while beeing ESTABLISHED:
> > > > >
> > > > > [NEW] tcp 6 120 SYN_SENT src=192.168.6.55 dst=192.168.10.22 sport=55582 dport=22 [UNREPLIED] src=192.168.10.22 dst=192.168.6.55 sport=22 dport=55582 mark=16777216
> > > > > [UPDATE] tcp 6 60 SYN_RECV src=192.168.6.55 dst=192.168.10.22 sport=55582 dport=22 src=192.168.10.22 dst=192.168.6.55 sport=22 dport=55582 mark=16777216
> > > > > [UPDATE] tcp 6 86400 ESTABLISHED src=192.168.6.55 dst=192.168.10.22 sport=55582 dport=22 src=192.168.10.22 dst=192.168.6.55 sport=22 dport=55582 [OFFLOAD] mark=16777216
> > > > >
> > > > > [DESTROY] tcp 6 ESTABLISHED src=192.168.6.55 dst=192.168.10.22 sport=54941 dport=22 packets=133 bytes=13033 src=192.168.10.22 dst=192.168.6.55 sport=22 dport=54941 packets=95 bytes=15004 [ASSURED] mark=16777216 delta-time=1036
> > > > >
> > > > > I would remove the if case here.
> > > >
> > > > OK, I remove it and post a v2. Thanks!
> > >
> > > Thanks and also the hardcoded TCP_CONNTRACK_ESTABLISHED in flow_offload_fixup_ct
> > > should be reverted to the real tcp state ct->proto.tcp.state.
> >
> > ct->proto.tcp.state contains the state that was observed before
> > handling over this flow to the flowtable, in most cases, this should
> > be TCP_CONNTRACK_ESTABLISHED.
> >
> > > This way we always set the current TCP timeout.
> >
> > I can keep it to ct->proto.tcp.state but I wonder if it is better to
> > use a well known state such as TCP_CONNTRACK_ESTABLISHED to pick up from.
>
> In case of a race condition or if something is off like my TCP_FIN
> that is beeing offloaded again setting to to ESTABLISHED hard coded
> will make the e.g. FIN or CLOSE a very long state.
> It is not guaranteed that we are still in ESTABLISHED when this code
> runs. Also for example we could have seen both FIN already before the
> flowtable gc runs.
OK, I just posted a v2, leave things as is. I agree it is better to
only address the issue you are observing at this time, it is possible
to revisit later.
Thanks!
