On Thu, Apr 13, 2023 at 12:56:01AM +0200, Pablo Neira Ayuso wrote:
> Maybe just do this special handling:
>
> + if (nf_ct_is_confirmed(ct))
> + WRITE_ONCE(ct->timeout, timeout + nfct_time_stamp);
> + else
> + WRITE_ONCE(ct->timeout, timeout);
>
> for ctnetlink_change_timeout().
>
> Just replace __nf_ct_set_timeout(), by this code above in
> nf_conntrack_netlink.c? I think the __nf_ct_set_timeout() helper is
> not very useful.
I don't quite understand the message above.
Calling path in v6.3-rc6:
ctnetlink_change_timeout() in net/netfilter/nf_conntrack_netlink.c
-> __nf_ct_change_timeout() in net/netfilter/nf_conntrack_core.c
-> __nf_ct_set_timeout() in include/net/netfilter/nf_conntrack_core.h
To clarify, which one did you mean:
Option 1: replace the __nf_ct_change_timeout() invocation to the special
handling in net/netfilter/nf_conntrack_netlink.c
Option 2: replace the __nf_ct_set_timeout() invocation to the special
handling in net/netfilter/nf_conntrack_core.c
Option 3: put the special handling in __nf_ct_set_timeout() in
include/net/netfilter/nf_conntrack_core.h
In either case, the fix would be a subset of v1.
I'm not sure other use cases. In our environment, we observed an
inconsistent state by a partial fix of v1. nf_ct_expires() got called
by userspace program. And the return value (which means the remaining
timeout) will be the parameter for the next ctnetlink_change_timeout().
As you can see in [4], if this happens on an unconfirmed conntrack, the
`nfct_time_stamp` would be wrongly invoved in the calculation again.
That's why we take care of all `ct->timeout` accesses in v1.
[4]: https://elixir.bootlin.com/linux/v6.3-rc6/source/include/net/netfilter/nf_conntrack.h#L296
