[PATCH] netfilter: ipset: wait for xt_recseq on all cpus

[PATCH] netfilter: ipset: wait for xt_recseq on all cpus

[xiaolinkui]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

Before destroying the ipset, take a check on sequence to ensure that the
ip_set_test operation of this ipset has been completed.

The code of set_match_v4 is protected by addend=xt_write_recseq_begin() and
xt_write_recseq_end(addend). So we can ensure that the test operation is
completed by reading seqcount.

Otherwise, there will be a low probability of use-after-free problems
occurring:

 PC: ffff0000033c0168  [hash_net4_kadt+56]
 LR: ffff000002b811bc  [ip_set_test+188]
 SP: ffff8003fff3f8d0  PSTATE: 60400005
X29: ffff8003fff3f8d0  X28: ffff8003ab915c4e  X27: ffff8003b0c7a000
X26: ffff8003b9780040  X25: ffff000000c70600  X24: ffff8003ac2c0200
X23: ffff000002f70fcc  X22: 0000000000000002  X21: ffff8003ac2c0200
X20: ffff8003be8e2800  X19: ffff8003fff3f9c8  X18: 0000000000000000
X17: 0000000000000000  X16: 0000000000000000  X15: 0000000000000000
X14: 970000002d494600  X13: 0000000000000000  X12: c5d405f139e6e418
X11: ffff000000c70600  X10: ffff8003b0c7a000   X9: 0000000000000001
 X8: 0000000000000000   X7: 000000000000005f   X6: 0000000000000000
 X5: ffff0000033c0130   X4: ffff8003fff3f9c8   X3: 0000000000000002
 X2: ffff0000033d01d8   X1: 00000000ffffffff   X0: 0000000000000000
[ffff8003fff3f8d0] hash_net4_kadt at ffff0000033c0164 [ip_set_hash_net]
[ffff8003fff3f940] ip_set_test at ffff000002b811b8 [ip_set]
[ffff8003fff3f990] set_match_v4 at ffff000002f70fc8 [xt_set]
[ffff8003fff3fa20] ipt_do_table at ffff000000c504e0 [ip_tables]
[ffff8003fff3fb60] iptable_filter_hook at ffff00000266006c [iptable_filter]
[ffff8003fff3fb80] nf_hook_slow at ffff000008ac7a84
[ffff8003fff3fbc0] ip_local_deliver at ffff000008ad5d88
[ffff8003fff3fc10] ip_rcv_finish at ffff000008ad59b4
[ffff8003fff3fc40] ip_rcv at ffff000008ad5dec
[ffff8003fff3fca0] __netif_receive_skb_one_core at ffff000008a6c344
[ffff8003fff3fce0] __netif_receive_skb at ffff000008a6c3ac
[ffff8003fff3fd00] netif_receive_skb_internal at ffff000008a6c440
[ffff8003fff3fd30] napi_gro_receive at ffff000008a6d3ec
[ffff8003fff3fd60] receive_buf at ffff000001c934d8 [virtio_net]
[ffff8003fff3fe20] virtnet_poll at ffff000001c953e8 [virtio_net]
[ffff8003fff3fec0] net_rx_action at ffff000008a6c9ec
[ffff8003fff3ff60] __softirqentry_text_start at ffff0000080819f0
[ffff8003fff3fff0] irq_exit at ffff0000080f1228
[ffff8003fff40010] __handle_domain_irq at ffff000008162a10

Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
 net/netfilter/ipset/ip_set_core.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index 46f4f47e29e4..53561176162f 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -1187,6 +1187,24 @@ ip_set_destroy_set(struct ip_set *set)
 	kfree(set);
 }
 
+static void wait_xt_recseq(void)
+{
+	unsigned int cpu;
+
+	/* wait for even xt_recseq on all cpus */
+	for_each_possible_cpu(cpu) {
+		seqcount_t *s = &per_cpu(xt_recseq, cpu);
+		u32 seq = raw_read_seqcount(s);
+
+		if (seq & 1) {
+			do {
+				cond_resched();
+				cpu_relax();
+			} while (seq == raw_read_seqcount(s));
+		}
+	}
+}
+
 static int ip_set_destroy(struct sk_buff *skb, const struct nfnl_info *info,
 			  const struct nlattr * const attr[])
 {
@@ -1225,6 +1243,7 @@ static int ip_set_destroy(struct sk_buff *skb, const struct nfnl_info *info,
 		for (i = 0; i < inst->ip_set_max; i++) {
 			s = ip_set(inst, i);
 			if (s) {
+				wait_xt_recseq();
 				ip_set(inst, i) = NULL;
 				ip_set_destroy_set(s);
 			}
@@ -1243,6 +1262,7 @@ static int ip_set_destroy(struct sk_buff *skb, const struct nfnl_info *info,
 			ret = -IPSET_ERR_BUSY;
 			goto out;
 		}
+		wait_xt_recseq();
 		ip_set(inst, i) = NULL;
 		read_unlock_bh(&ip_set_ref_lock);
 
-- 
2.17.1

Re: [PATCH] netfilter: ipset: wait for xt_recseq on all cpus

[Florian Westphal]

xiaolinkui <xiaolinkui@126.com> wrote:
> From: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> Before destroying the ipset, take a check on sequence to ensure that the
> ip_set_test operation of this ipset has been completed.
> 
> The code of set_match_v4 is protected by addend=xt_write_recseq_begin() and
> xt_write_recseq_end(addend). So we can ensure that the test operation is
> completed by reading seqcount.

Nope, please don't do this, the xt_set can also be used from nft_compat
which doesn't use the xtables packet traversers.

I'd rather use synchonize_rcu() once in ip_set_destroy(), that will
make sure all concurrent traversers are gone.

That said, I still do not understand this fix, the
match / target destroy hooks are called after the table has
been completely replaced, i.e., while packets can still be in flight
no packets should be within the ipset lookup functions when
this happens, and no more packets should be able to enter them.

AFAICS the request to delete the set will fail if its still referenced
via any rule. xt_set holds references to the sets.

So:
1. set have dropped all references
2. userspace *can* delete the set
3. we get crash because xt_set was still within a sets eval
  function.

I don't see how 3) can happen, xt table replace isn't supposed
to call the xt_set destroy functions until after table replace.

We even release the entire x_table blob right afterwards.

Re: [PATCH] netfilter: ipset: wait for xt_recseq on all cpus

[Jozsef Kadlecsik]

On Thu, 5 Oct 2023, Florian Westphal wrote:

> xiaolinkui <xiaolinkui@126.com> wrote:
> > From: Linkui Xiao <xiaolinkui@kylinos.cn>
> > 
> > Before destroying the ipset, take a check on sequence to ensure that the
> > ip_set_test operation of this ipset has been completed.
> > 
> > The code of set_match_v4 is protected by addend=xt_write_recseq_begin() and
> > xt_write_recseq_end(addend). So we can ensure that the test operation is
> > completed by reading seqcount.
> 
> Nope, please don't do this, the xt_set can also be used from nft_compat
> which doesn't use the xtables packet traversers.
> 
> I'd rather use synchonize_rcu() once in ip_set_destroy(), that will
> make sure all concurrent traversers are gone.

But ip_set_destroy() can be called only when there's no reference to the 
set in the kernel and thus there's no ipset function whatsoever in the 
packet path which would access it.

> That said, I still do not understand this fix, the
> match / target destroy hooks are called after the table has
> been completely replaced, i.e., while packets can still be in flight
> no packets should be within the ipset lookup functions when
> this happens, and no more packets should be able to enter them.
> 
> AFAICS the request to delete the set will fail if its still referenced
> via any rule. xt_set holds references to the sets.
> 
> So:
> 1. set have dropped all references
> 2. userspace *can* delete the set
> 3. we get crash because xt_set was still within a sets eval
>   function.
> 
> I don't see how 3) can happen, xt table replace isn't supposed
> to call the xt_set destroy functions until after table replace.
> 
> We even release the entire x_table blob right afterwards.

I'd expect the author to send patches to netfilter-devel@vger.kernel.org 
first in order to review netfilter and ipset related patches there.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH] netfilter: ipset: wait for xt_recseq on all cpus

[kernel test robot]

Hi xiaolinkui,

kernel test robot noticed the following build errors:

[auto build test ERROR on netfilter-nf/main]
[also build test ERROR on nf-next/master horms-ipvs/master linus/master v6.6-rc6 next-20231016]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/xiaolinkui/netfilter-ipset-wait-for-xt_recseq-on-all-cpus/20231005-234042
base:   git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git main
patch link:    https://lore.kernel.org/r/20231005115022.12902-1-xiaolinkui%40126.com
patch subject: [PATCH] netfilter: ipset: wait for xt_recseq on all cpus
config: x86_64-buildonly-randconfig-006-20231016 (https://download.01.org/0day-ci/archive/20231016/202310161625.GDDBP8SZ-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231016/202310161625.GDDBP8SZ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310161625.GDDBP8SZ-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: vmlinux.o: in function `wait_xt_recseq':
>> ip_set_core.c:(.text+0x1c561ac): undefined reference to `xt_recseq'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] netfilter: ipset: wait for xt_recseq on all cpus

[kernel test robot]

Hi xiaolinkui,

kernel test robot noticed the following build errors:

[auto build test ERROR on netfilter-nf/main]
[also build test ERROR on nf-next/master horms-ipvs/master linus/master v6.6-rc6 next-20231016]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/xiaolinkui/netfilter-ipset-wait-for-xt_recseq-on-all-cpus/20231005-234042
base:   git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git main
patch link:    https://lore.kernel.org/r/20231005115022.12902-1-xiaolinkui%40126.com
patch subject: [PATCH] netfilter: ipset: wait for xt_recseq on all cpus
config: i386-randconfig-016-20231016 (https://download.01.org/0day-ci/archive/20231016/202310161728.mW3lt1Jl-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231016/202310161728.mW3lt1Jl-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310161728.mW3lt1Jl-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: net/netfilter/ipset/ip_set_core.o: in function `wait_xt_recseq':
>> net/netfilter/ipset/ip_set_core.c:1194: undefined reference to `xt_recseq'


vim +1194 net/netfilter/ipset/ip_set_core.c

  1187	
  1188	static void wait_xt_recseq(void)
  1189	{
  1190		unsigned int cpu;
  1191	
  1192		/* wait for even xt_recseq on all cpus */
  1193		for_each_possible_cpu(cpu) {
> 1194			seqcount_t *s = &per_cpu(xt_recseq, cpu);
  1195			u32 seq = raw_read_seqcount(s);
  1196	
  1197			if (seq & 1) {
  1198				do {
  1199					cond_resched();
  1200					cpu_relax();
  1201				} while (seq == raw_read_seqcount(s));
  1202			}
  1203		}
  1204	}
  1205	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] netfilter: ipset: wait for xt_recseq on all cpus

[Jozsef Kadlecsik]

Hello,

Besides the broken patch, the description simply cannot be true:

"Before destroying the ipset, take a check on sequence to ensure that the 
ip_set_test operation of this ipset has been completed."

Set can only be destroyed when there is no iptables rule (match/target) 
which refers to it. If this condition is not true, then the real reason 
must be fixed.

How can one reproduce the issue?

Best regards,
Jozsef

On Mon, 16 Oct 2023, kernel test robot wrote:

> Hi xiaolinkui,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on netfilter-nf/main]
> [also build test ERROR on nf-next/master horms-ipvs/master linus/master v6.6-rc6 next-20231016]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/xiaolinkui/netfilter-ipset-wait-for-xt_recseq-on-all-cpus/20231005-234042
> base:   git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git main
> patch link:    https://lore.kernel.org/r/20231005115022.12902-1-xiaolinkui%40126.com
> patch subject: [PATCH] netfilter: ipset: wait for xt_recseq on all cpus
> config: x86_64-buildonly-randconfig-006-20231016 (https://download.01.org/0day-ci/archive/20231016/202310161625.GDDBP8SZ-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231016/202310161625.GDDBP8SZ-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202310161625.GDDBP8SZ-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>    ld: vmlinux.o: in function `wait_xt_recseq':
> >> ip_set_core.c:(.text+0x1c561ac): undefined reference to `xt_recseq'
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
> 

-- 
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary