Re: [PATCH net-next 3/9] netfilter: conntrack: avoid reload of ct->status

[Roi Dayan <roid@nvidia.com>]

On 18/01/2023 14:32, Florian Westphal wrote:
> Compiler can't merge the two test_bit() calls, so load ct->status
> once and use non-atomic accesses.
> 
> This is fine because IPS_EXPECTED or NAT_CLASH are either set at ct
> creation time or not at all, but compiler can't know that.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  net/netfilter/nf_conntrack_core.c      |  9 +++++----
>  net/netfilter/nf_conntrack_proto_udp.c | 10 ++++++----
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 81ece117033a..9e12cade4e0f 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -1854,14 +1854,15 @@ resolve_normal_ct(struct nf_conn *tmpl,
>  	if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY) {
>  		ctinfo = IP_CT_ESTABLISHED_REPLY;
>  	} else {
> +		unsigned long status = READ_ONCE(ct->status);
> +
>  		/* Once we've had two way comms, always ESTABLISHED. */
> -		if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status)) {
> +		if (likely(status & IPS_SEEN_REPLY))
>  			ctinfo = IP_CT_ESTABLISHED;
> -		} else if (test_bit(IPS_EXPECTED_BIT, &ct->status)) {
> +		else if (status & IPS_EXPECTED)
>  			ctinfo = IP_CT_RELATED;
> -		} else {
> +		else
>  			ctinfo = IP_CT_NEW;
> -		}
>  	}
>  	nf_ct_set(skb, ct, ctinfo);
>  	return 0;
> diff --git a/net/netfilter/nf_conntrack_proto_udp.c b/net/netfilter/nf_conntrack_proto_udp.c
> index 3b516cffc779..6b9206635b24 100644
> --- a/net/netfilter/nf_conntrack_proto_udp.c
> +++ b/net/netfilter/nf_conntrack_proto_udp.c
> @@ -88,6 +88,7 @@ int nf_conntrack_udp_packet(struct nf_conn *ct,
>  			    const struct nf_hook_state *state)
>  {
>  	unsigned int *timeouts;
> +	unsigned long status;
>  
>  	if (udp_error(skb, dataoff, state))
>  		return -NF_ACCEPT;
> @@ -96,26 +97,27 @@ int nf_conntrack_udp_packet(struct nf_conn *ct,
>  	if (!timeouts)
>  		timeouts = udp_get_timeouts(nf_ct_net(ct));
>  
> -	if (!nf_ct_is_confirmed(ct))
> +	status = READ_ONCE(ct->status);
> +	if ((status & IPS_CONFIRMED) == 0)
>  		ct->proto.udp.stream_ts = 2 * HZ + jiffies;
>  
>  	/* If we've seen traffic both ways, this is some kind of UDP
>  	 * stream. Set Assured.
>  	 */
> -	if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status)) {
> +	if (status & IPS_SEEN_REPLY_BIT) {

Hi,

This change has a bug not matching reply status anymore.
Since you don't use test_bit() you should use IPS_SEEN_REPLY
instead of IPS_SEEN_REPLY_BIT.

Thanks,
Roi

>  		unsigned long extra = timeouts[UDP_CT_UNREPLIED];
>  		bool stream = false;
>  
>  		/* Still active after two seconds? Extend timeout. */
>  		if (time_after(jiffies, ct->proto.udp.stream_ts)) {
>  			extra = timeouts[UDP_CT_REPLIED];
> -			stream = true;
> +			stream = (status & IPS_ASSURED) == 0;
>  		}
>  
>  		nf_ct_refresh_acct(ct, ctinfo, skb, extra);
>  
>  		/* never set ASSURED for IPS_NAT_CLASH, they time out soon */
> -		if (unlikely((ct->status & IPS_NAT_CLASH)))
> +		if (unlikely((status & IPS_NAT_CLASH)))
>  			return NF_ACCEPT;
>  
>  		/* Also, more likely to be important, and not a probe */