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@xxxxxxxxx>
> ---
> 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 */
