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

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

Hi,

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

> [1]: https://elixir.bootlin.com/linux/v6.3-rc5/source/net/netfilter/nf_conntrack_core.c#L1257
> [2]: https://elixir.bootlin.com/linux/v6.3-rc5/source/include/net/netfilter/nf_conntrack_core.h#L92
> 
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
>  include/net/netfilter/nf_conntrack.h      | 27 +++++++++++++++++++----
>  include/net/netfilter/nf_conntrack_core.h |  2 +-
>  net/netfilter/nf_conntrack_core.c         | 11 ++++-----
>  net/netfilter/nf_conntrack_proto_tcp.c    |  4 ++--
>  net/netfilter/nf_flow_table_core.c        |  4 ++--
>  5 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
> index a72028dbef0c..48e020db3fb3 100644
> --- a/include/net/netfilter/nf_conntrack.h
> +++ b/include/net/netfilter/nf_conntrack.h
> @@ -290,17 +290,36 @@ static inline bool nf_is_loopback_packet(const struct sk_buff *skb)
>  
>  #define nfct_time_stamp ((u32)(jiffies))
>  
> +static inline s32 nf_ct_read_timeout(const struct nf_conn *ct)
> +{
> +	s32 timeout;
> +
> +	if (nf_ct_is_confirmed(ct))
> +		timeout = READ_ONCE(ct->timeout) - nfct_time_stamp;
> +	else
> +		timeout = READ_ONCE(ct->timeout);
> +
> +	return timeout;
> +}
> +
> +static inline void nf_ct_write_timeout(struct nf_conn *ct, s32 timeout)
> +{
> +	if (nf_ct_is_confirmed(ct))
> +		WRITE_ONCE(ct->timeout, timeout + nfct_time_stamp);
> +	else
> +		WRITE_ONCE(ct->timeout, timeout);
> +}
> +
>  /* jiffies until ct expires, 0 if already expired */
>  static inline unsigned long nf_ct_expires(const struct nf_conn *ct)
>  {
> -	s32 timeout = READ_ONCE(ct->timeout) - nfct_time_stamp;
> -
> +	s32 timeout = nf_ct_read_timeout(ct);
>  	return max(timeout, 0);
>  }
>  
>  static inline bool nf_ct_is_expired(const struct nf_conn *ct)
>  {
> -	return (__s32)(READ_ONCE(ct->timeout) - nfct_time_stamp) <= 0;
> +	return nf_ct_expires(ct) == 0;
>  }
>  
>  /* use after obtaining a reference count */
> @@ -319,7 +338,7 @@ static inline bool nf_ct_should_gc(const struct nf_conn *ct)
>  static inline void nf_ct_offload_timeout(struct nf_conn *ct)
>  {
>  	if (nf_ct_expires(ct) < NF_CT_DAY / 2)
> -		WRITE_ONCE(ct->timeout, nfct_time_stamp + NF_CT_DAY);
> +		nf_ct_write_timeout(ct, NF_CT_DAY);
>  }
>  
>  struct kernel_param;
> diff --git a/include/net/netfilter/nf_conntrack_core.h b/include/net/netfilter/nf_conntrack_core.h
> index 71d1269fe4d4..7a6e49a66d20 100644
> --- a/include/net/netfilter/nf_conntrack_core.h
> +++ b/include/net/netfilter/nf_conntrack_core.h
> @@ -89,7 +89,7 @@ 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);
> +	nf_ct_write_timeout(ct, (u32)timeout);
>  }
>  
>  int __nf_ct_change_timeout(struct nf_conn *ct, u64 cta_timeout);
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index db1ea361f2da..47166576c195 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -657,7 +657,7 @@ bool nf_ct_delete(struct nf_conn *ct, u32 portid, int report)
>  
>  	tstamp = nf_conn_tstamp_find(ct);
>  	if (tstamp) {
> -		s32 timeout = READ_ONCE(ct->timeout) - nfct_time_stamp;
> +		s32 timeout = nf_ct_read_timeout(ct);
>  
>  		tstamp->stop = ktime_get_real_ns();
>  		if (timeout < 0)
> @@ -1063,7 +1063,7 @@ static int nf_ct_resolve_clash_harder(struct sk_buff *skb, u32 repl_idx)
>  	}
>  
>  	/* We want the clashing entry to go away real soon: 1 second timeout. */
> -	WRITE_ONCE(loser_ct->timeout, nfct_time_stamp + HZ);
> +	nf_ct_write_timeout(loser_ct, HZ);
>  
>  	/* IPS_NAT_CLASH removes the entry automatically on the first
>  	 * reply.  Also prevents UDP tracker from moving the entry to
> @@ -2079,11 +2079,8 @@ void __nf_ct_refresh_acct(struct nf_conn *ct,
>  		goto acct;
>  
>  	/* If not in hash table, timer will not be active yet */
> -	if (nf_ct_is_confirmed(ct))
> -		extra_jiffies += nfct_time_stamp;
> -
> -	if (READ_ONCE(ct->timeout) != extra_jiffies)
> -		WRITE_ONCE(ct->timeout, extra_jiffies);
> +	if (nf_ct_read_timeout(ct) != extra_jiffies)
> +		nf_ct_write_timeout(ct, extra_jiffies);
>  acct:
>  	if (do_acct)
>  		nf_ct_acct_update(ct, CTINFO2DIR(ctinfo), skb->len);
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> index 4018acb1d674..a2797d026943 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -766,7 +766,7 @@ static void __cold nf_tcp_handle_invalid(struct nf_conn *ct,
>  					  "packet (index %d, dir %d) response for index %d lower timeout to %u",
>  					  index, dir, ct->proto.tcp.last_index, timeout);
>  
> -			WRITE_ONCE(ct->timeout, timeout + nfct_time_stamp);
> +			nf_ct_write_timeout(ct, timeout);
>  		}
>  	} else {
>  		ct->proto.tcp.last_index = index;
> @@ -939,7 +939,7 @@ void nf_conntrack_tcp_set_closing(struct nf_conn *ct)
>  	}
>  
>  	timeout = timeouts[TCP_CONNTRACK_CLOSE];
> -	WRITE_ONCE(ct->timeout, timeout + nfct_time_stamp);
> +	nf_ct_write_timeout(ct, timeout);
>  
>  	spin_unlock_bh(&ct->lock);
>  
> diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
> index 04bd0ed4d2ae..113bd361e537 100644
> --- a/net/netfilter/nf_flow_table_core.c
> +++ b/net/netfilter/nf_flow_table_core.c
> @@ -206,8 +206,8 @@ static void flow_offload_fixup_ct(struct nf_conn *ct)
>  	if (timeout < 0)
>  		timeout = 0;
>  
> -	if (nf_flow_timeout_delta(READ_ONCE(ct->timeout)) > (__s32)timeout)
> -		WRITE_ONCE(ct->timeout, nfct_time_stamp + timeout);
> +	if (nf_flow_timeout_delta(nf_ct_read_timeout(ct)) > (__s32)timeout)
> +		nf_ct_write_timeout(ct, timeout);
>  }
>  
>  static void flow_offload_route_release(struct flow_offload *flow)
> -- 
> 2.40.0.577.gac1e443424-goog
>