netdev - Re: [PATCH net 14/14] netfilter: ipset: fix performance regression in swap operation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 18 Jan 2024 11:08:00 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Pablo Neira Ayuso <pablo@...filter.org>
Cc: netfilter-devel@...r.kernel.org, davem@...emloft.net, 
	netdev@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com, fw@...len.de
Subject: Re: [PATCH net 14/14] netfilter: ipset: fix performance regression in
 swap operation

On Wed, Jan 17, 2024 at 5:00 PM Pablo Neira Ayuso <pablo@...filter.org> wrote:
>
> From: Jozsef Kadlecsik <kadlec@...filter.org>
>
> The patch "netfilter: ipset: fix race condition between swap/destroy
> and kernel side add/del/test", commit 28628fa9 fixes a race condition.
> But the synchronize_rcu() added to the swap function unnecessarily slows
> it down: it can safely be moved to destroy and use call_rcu() instead.
> Thus we can get back the same performance and preventing the race condition
> at the same time.
>
> Fixes: 28628fa952fe ("netfilter: ipset: fix race condition between swap/destroy and kernel side add/del/test")
> Link: https://lore.kernel.org/lkml/C0829B10-EAA6-4809-874E-E1E9C05A8D84@automattic.com/
> Reported-by: Ale Crismani <ale.crismani@...omattic.com>
> Reported-by: David Wang <00107082@....com
> Tested-by: Ale Crismani <ale.crismani@...omattic.com>
> Tested-by: David Wang <00107082@....com
> Signed-off-by: Jozsef Kadlecsik <kadlec@...filter.org>
> Signed-off-by: Pablo Neira Ayuso <pablo@...filter.org>
> ---
>  include/linux/netfilter/ipset/ip_set.h |  2 ++
>  net/netfilter/ipset/ip_set_core.c      | 31 +++++++++++++++++++-------
>  2 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/netfilter/ipset/ip_set.h b/include/linux/netfilter/ipset/ip_set.h
> index e8c350a3ade1..912f750d0bea 100644
> --- a/include/linux/netfilter/ipset/ip_set.h
> +++ b/include/linux/netfilter/ipset/ip_set.h
> @@ -242,6 +242,8 @@ extern void ip_set_type_unregister(struct ip_set_type *set_type);
>
>  /* A generic IP set */
>  struct ip_set {
> +       /* For call_cru in destroy */
> +       struct rcu_head rcu;
>         /* The name of the set */
>         char name[IPSET_MAXNAMELEN];
>         /* Lock protecting the set data */
> diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
> index 4c133e06be1d..3bf9bb345809 100644
> --- a/net/netfilter/ipset/ip_set_core.c
> +++ b/net/netfilter/ipset/ip_set_core.c
> @@ -1182,6 +1182,14 @@ ip_set_destroy_set(struct ip_set *set)
>         kfree(set);
>  }
>
> +static void
> +ip_set_destroy_set_rcu(struct rcu_head *head)
> +{
> +       struct ip_set *set = container_of(head, struct ip_set, rcu);
> +
> +       ip_set_destroy_set(set);

Calling ip_set_destroy_set() from BH (rcu callbacks) is not working.

I think you should test your patch with LOCKDEP enabled, and/or
CONFIG_DEBUG_ATOMIC_SLEEP=y