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

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Thu, Apr 13, 2023 at 11:23:11AM +0800, Tzung-Bi Shih wrote:
> 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.

Yes, I think this is Option 3:

diff --git a/include/net/netfilter/nf_conntrack_core.h b/include/net/netfilter/nf_conntrack_core.h
index 71d1269fe4d4..9c2cd69bbdc6 100644
--- a/include/net/netfilter/nf_conntrack_core.h
+++ b/include/net/netfilter/nf_conntrack_core.h
@@ -89,7 +89,11 @@ static inline void __nf_ct_set_timeout(struct nf_conn *ct, u64 timeout)
 {
        if (timeout > INT_MAX)
                timeout = INT_MAX;
-       WRITE_ONCE(ct->timeout, nfct_time_stamp + (u32)timeout);
+
+       if (nf_ct_is_confirmed(ct))
+               WRITE_ONCE(ct->timeout, nfct_time_stamp + (u32)timeout;
+       else
+               ct->timeout = (u32)timeout;
 }
 
 int __nf_ct_change_timeout(struct nf_conn *ct, u64 cta_timeout);

Note:

                WRITE_ONCE(ct->timeout, (u32)timeout);

is not required, because unconfirmed conntrack object is owned by the
packet (not yet in the hashes).


BTW, not related to this patch, but I would like to understand why
this __nf_ct_set_timeout() function is inline, but that is a different
issue.

> I'm not sure other use cases.  In our environment, we observed an
> inconsistent state by a partial fix of v1. 

Thanks for explaining, extending patch description would be good.

> 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().

Unconfirmed conntrack is owned by the packet that refers to it, it is
not yet in the hashes. I don't see how concurrent access to the
timeout might occur.

Or are you referring to a different scenario that triggers the partial
state?

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

If you are observing a partial state, that is a different issue and I
think it deserves a separated patch with a description? Probably
including KCSAN splat if this is what you used to catch the partial
state.

Thanks!

> [4]: https://elixir.bootlin.com/linux/v6.3-rc6/source/include/net/netfilter/nf_conntrack.h#L296