| Message ID | 20230118123208.17167-4-fw@strlen.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4883ec512c1715fc827557f0e2bfce76c6530757 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,1/9] netfilter: conntrack: sctp: use nf log infrastructure for invalid packets | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next, async |
| netdev/apply | success | Patch already applied to net-next |
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 */
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) { 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 */
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(-)