Re: [PATCH] netfilter: conntrack: fix wrong ct->timeout value

[Tzung-Bi Shih <tzungbi@kernel.org>]

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