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

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

On Mon, Apr 10, 2023 at 11:31:21AM +0200, Pablo Neira Ayuso wrote:
> On Mon, Apr 10, 2023 at 10:33:32AM +0200, Pablo Neira Ayuso wrote:
> > On Mon, Apr 10, 2023 at 02:09:35PM +0800, Tzung-Bi Shih wrote:
> > > (struct nf_conn)->timeout is an interval before the conntrack
> > > confirmed.  After confirmed, it becomes a timestamp[1].
> > > 
> > > It is observed that timeout of an unconfirmed conntrack have been
> > > altered by calling ctnetlink_change_timeout().  As a result,
> > > `nfct_time_stamp` was wrongly added to `ct->timeout` twice[2].
> > > 
> > > Differentiate the 2 cases in all `ct->timeout` accesses.
> > 
> > You can just skip refreshing the timeout for unconfirmed conntrack
> > entries in ctnetlink_change_timeout().
> 
> Something like this patch probably?

Pardon me, I sent a v2[3] before seeing the message.

[3]: https://lore.kernel.org/netfilter-devel/20230410093454.853575-1-tzungbi@kernel.org/T/#u

> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index bfc3aaa2c872..6556f5f30844 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -2466,7 +2466,8 @@ static int ctnetlink_new_conntrack(struct sk_buff *skb,
>  
>  	err = -EEXIST;
>  	ct = nf_ct_tuplehash_to_ctrack(h);
> -	if (!(info->nlh->nlmsg_flags & NLM_F_EXCL)) {
> +	if (!(info->nlh->nlmsg_flags & NLM_F_EXCL) &&
> +	    nf_ct_is_confirmed(ct)) {
>  		err = ctnetlink_change_conntrack(ct, cda);
>  		if (err == 0) {
>  			nf_conntrack_eventmask_report((1 << IPCT_REPLY) |

The patch can't fix the issue we observed.

Here is the calling stack:
  ctnetlink_glue_parse
  [...]
  __sys_sendto
  __x64_sys_sendto
  [...]

It was on another path:
ctnetlink_glue_parse_ct() -> ctnetlink_change_timeout().

I guess we should skip it in ctnetlink_change_timeout().  Something like v2.