[PATCH nf 1/2] netfilter: nft_last: avoid possible false sharing

[PATCH nf 1/2] netfilter: nft_last: avoid possible false sharing

[Pablo Neira Ayuso]

Use the idiom described in:

https://github.com/google/ktsan/wiki/READ_ONCE-and-WRITE_ONCE#it-may-improve-performance

Moreover, prevent a compiler optimization.

Fixes: 836382dc2471 ("netfilter: nf_tables: add last expression")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nft_last.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/net/netfilter/nft_last.c b/net/netfilter/nft_last.c
index 8088b99f2ee3..f29f205e9992 100644
--- a/net/netfilter/nft_last.c
+++ b/net/netfilter/nft_last.c
@@ -48,24 +48,30 @@ static void nft_last_eval(const struct nft_expr *expr,
 {
 	struct nft_last_priv *priv = nft_expr_priv(expr);
 
-	priv->last_jiffies = jiffies;
-	priv->last_set = 1;
+	if (READ_ONCE(priv->last_set) == 0)
+		WRITE_ONCE(priv->last_set, 1);
+	if (READ_ONCE(priv->last_jiffies) != jiffies)
+		WRITE_ONCE(priv->last_jiffies, jiffies);
 }
 
 static int nft_last_dump(struct sk_buff *skb, const struct nft_expr *expr)
 {
 	struct nft_last_priv *priv = nft_expr_priv(expr);
+	unsigned long last_jiffies = READ_ONCE(priv->last_jiffies);
+	u32 last_set = READ_ONCE(priv->last_set);
 	__be64 msecs;
 
-	if (time_before(jiffies, priv->last_jiffies))
-		priv->last_set = 0;
+	if (time_before(jiffies, last_jiffies)) {
+		WRITE_ONCE(priv->last_set, 0);
+		last_set = 0;
+	}
 
-	if (priv->last_set)
-		msecs = nf_jiffies64_to_msecs(jiffies - priv->last_jiffies);
+	if (last_set)
+		msecs = nf_jiffies64_to_msecs(jiffies - last_jiffies);
 	else
 		msecs = 0;
 
-	if (nla_put_be32(skb, NFTA_LAST_SET, htonl(priv->last_set)) ||
+	if (nla_put_be32(skb, NFTA_LAST_SET, htonl(last_set)) ||
 	    nla_put_be64(skb, NFTA_LAST_MSECS, msecs, NFTA_LAST_PAD))
 		goto nla_put_failure;
 
-- 
2.30.2

[PATCH nf 2/2] netfilter: flowtable: remove nf_ct_l4proto_find() call

[Pablo Neira Ayuso]

TCP and UDP are built-in conntrack protocol trackers and the flowtable
only supports for TCP and UDP, remove this call.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_flow_table_core.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index 551976e4284c..19f4c4343b7d 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -180,15 +180,10 @@ static void flow_offload_fixup_tcp(struct ip_ct_tcp *tcp)
 
 static void flow_offload_fixup_ct_timeout(struct nf_conn *ct)
 {
-	const struct nf_conntrack_l4proto *l4proto;
 	struct net *net = nf_ct_net(ct);
 	int l4num = nf_ct_protonum(ct);
 	unsigned int timeout;
 
-	l4proto = nf_ct_l4proto_find(l4num);
-	if (!l4proto)
-		return;
-
 	if (l4num == IPPROTO_TCP) {
 		struct nf_tcp_net *tn = nf_tcp_pernet(net);
 
@@ -197,8 +192,6 @@ static void flow_offload_fixup_ct_timeout(struct nf_conn *ct)
 		struct nf_udp_net *tn = nf_udp_pernet(net);
 
 		timeout = tn->offload_pickup;
-	} else {
-		return;
 	}
 
 	if (nf_flow_timeout_delta(ct->timeout) > (__s32)timeout)
@@ -273,15 +266,10 @@ static const struct rhashtable_params nf_flow_offload_rhash_params = {
 
 unsigned long flow_offload_get_timeout(struct flow_offload *flow)
 {
-	const struct nf_conntrack_l4proto *l4proto;
 	unsigned long timeout = NF_FLOW_TIMEOUT;
 	struct net *net = nf_ct_net(flow->ct);
 	int l4num = nf_ct_protonum(flow->ct);
 
-	l4proto = nf_ct_l4proto_find(l4num);
-	if (!l4proto)
-		return timeout;
-
 	if (l4num == IPPROTO_TCP) {
 		struct nf_tcp_net *tn = nf_tcp_pernet(net);
 
-- 
2.30.2

Re: [PATCH nf 2/2] netfilter: flowtable: remove nf_ct_l4proto_find() call

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3980 bytes --]

Hi Pablo,

I love your patch! Perhaps something to improve:

[auto build test WARNING on nf/master]

url:    https://github.com/0day-ci/linux/commits/Pablo-Neira-Ayuso/netfilter-nft_last-avoid-possible-false-sharing/20210718-102117
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git master
config: mips-randconfig-r032-20210718 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 5d5b08761f944d5b9822d582378333cc4b36a0a7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/0day-ci/linux/commit/5f2c0c949c4707c91d270de9993cf889ece6261a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Pablo-Neira-Ayuso/netfilter-nft_last-avoid-possible-false-sharing/20210718-102117
        git checkout 5f2c0c949c4707c91d270de9993cf889ece6261a
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/netfilter/nf_flow_table_core.c:191:13: warning: variable 'timeout' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
           } else if (l4num == IPPROTO_UDP) {
                      ^~~~~~~~~~~~~~~~~~~~
   net/netfilter/nf_flow_table_core.c:197:50: note: uninitialized use occurs here
           if (nf_flow_timeout_delta(ct->timeout) > (__s32)timeout)
                                                           ^~~~~~~
   net/netfilter/nf_flow_table_core.c:191:9: note: remove the 'if' if its condition is always true
           } else if (l4num == IPPROTO_UDP) {
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~
   net/netfilter/nf_flow_table_core.c:185:22: note: initialize the variable 'timeout' to silence this warning
           unsigned int timeout;
                               ^
                                = 0
   1 warning generated.


vim +191 net/netfilter/nf_flow_table_core.c

da5984e51063a2 Felix Fietkau     2018-02-26  180  
1e5b2471bcc483 Pablo Neira Ayuso 2019-08-09  181  static void flow_offload_fixup_ct_timeout(struct nf_conn *ct)
da5984e51063a2 Felix Fietkau     2018-02-26  182  {
1d91d2e1a7f767 Oz Shlomo         2021-06-03  183  	struct net *net = nf_ct_net(ct);
1e5b2471bcc483 Pablo Neira Ayuso 2019-08-09  184  	int l4num = nf_ct_protonum(ct);
da5984e51063a2 Felix Fietkau     2018-02-26  185  	unsigned int timeout;
da5984e51063a2 Felix Fietkau     2018-02-26  186  
1d91d2e1a7f767 Oz Shlomo         2021-06-03  187  	if (l4num == IPPROTO_TCP) {
1d91d2e1a7f767 Oz Shlomo         2021-06-03  188  		struct nf_tcp_net *tn = nf_tcp_pernet(net);
1d91d2e1a7f767 Oz Shlomo         2021-06-03  189  
1d91d2e1a7f767 Oz Shlomo         2021-06-03  190  		timeout = tn->offload_pickup;
1d91d2e1a7f767 Oz Shlomo         2021-06-03 @191  	} else if (l4num == IPPROTO_UDP) {
1d91d2e1a7f767 Oz Shlomo         2021-06-03  192  		struct nf_udp_net *tn = nf_udp_pernet(net);
1d91d2e1a7f767 Oz Shlomo         2021-06-03  193  
1d91d2e1a7f767 Oz Shlomo         2021-06-03  194  		timeout = tn->offload_pickup;
1d91d2e1a7f767 Oz Shlomo         2021-06-03  195  	}
da5984e51063a2 Felix Fietkau     2018-02-26  196  
1e5b2471bcc483 Pablo Neira Ayuso 2019-08-09  197  	if (nf_flow_timeout_delta(ct->timeout) > (__s32)timeout)
da5984e51063a2 Felix Fietkau     2018-02-26  198  		ct->timeout = nfct_time_stamp + timeout;
da5984e51063a2 Felix Fietkau     2018-02-26  199  }
da5984e51063a2 Felix Fietkau     2018-02-26  200  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37641 bytes --]