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 for Android: free password hash cracker in your pocket

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

Date: Thu, 18 Jan 2024 15:24:41 +0100 (CET)
From: Jozsef Kadlecsik <kadlec@...filter.org>
To: Eric Dumazet <edumazet@...gle.com>
cc: Pablo Neira Ayuso <pablo@...filter.org>, netfilter-devel@...r.kernel.org, 
    David Miller <davem@...emloft.net>, netdev@...r.kernel.org, 
    kuba@...nel.org, pabeni@...hat.com, Florian Westphal <fw@...len.de>
Subject: Re: [PATCH net 14/14] netfilter: ipset: fix performance regression
 in swap operation

Hi,

Please drop the patch from the batch, it needs more work (see below).

On Thu, 18 Jan 2024, Eric Dumazet wrote:

> On Wed, Jan 17, 2024 at 5:00 PM Pablo Neira Ayuso <pablo@...filter.org> wrote:
> >
> > 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.

Yeah, it calls cancel_delayed_work_sync() to handle the garbage collector 
and that can wait. The call can be moved into the main destroy function 
and let the rcu callback do just the minimal job, however it needs a 
restructuring. So please skip this patch now.

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