netdev - Re: [PATCH v2 nf-next 1/2] netfilter: x_tables: make xt_replace_table wait until old rules are not used anymore

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:   Wed, 11 Oct 2017 06:23:03 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     Florian Westphal <fw@...len.de>
Cc:     netdev <netdev@...r.kernel.org>, Dan Williams <dcbw@...hat.com>
Subject: Re: [PATCH v2 nf-next 1/2] netfilter: x_tables: make xt_replace_table
 wait until old rules are not used anymore

On Tue, Oct 10, 2017 at 2:39 PM, Florian Westphal <fw@...len.de> wrote:
> xt_replace_table relies on table replacement counter retrieval (which
> uses xt_recseq to synchronize pcpu counters).
>
> This is fine, however with large rule set get_counters() can take
> a very long time -- it needs to synchronize all counters because
> it has to assume concurrent modifications can occur.
>
> Make xt_replace_table synchronize by itself by waiting until all cpus
> had an even seqcount.
>
> This allows a followup patch to copy the counters of the old ruleset
> without any synchonization after xt_replace_table has completed.
>
> Cc: Dan Williams <dcbw@...hat.com>
> Cc: Eric Dumazet <edumazet@...gle.com>
> Signed-off-by: Florian Westphal <fw@...len.de>
> ---
>  v2: fix Erics email address
>
>  net/netfilter/x_tables.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
> index c83a3b5e1c6c..f2d4a365768f 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -1153,6 +1153,7 @@ xt_replace_table(struct xt_table *table,
>               int *error)
>  {
>         struct xt_table_info *private;
> +       unsigned int cpu;
>         int ret;
>
>         ret = xt_jumpstack_alloc(newinfo);
> @@ -1184,12 +1185,20 @@ xt_replace_table(struct xt_table *table,
>
>         /*
>          * Even though table entries have now been swapped, other CPU's
> -        * may still be using the old entries. This is okay, because
> -        * resynchronization happens because of the locking done
> -        * during the get_counters() routine.
> +        * may still be using the old entries...
>          */
>         local_bh_enable();
>
> +       /* ... so wait for even xt_recseq on all cpus */
> +       for_each_possible_cpu(cpu) {
> +               seqcount_t *s = &per_cpu(xt_recseq, cpu);
> +
> +               while (raw_read_seqcount(s) & 1)
> +                       cpu_relax();
> +
> +               cond_resched();
> +       }

It seems that we could also check :

1) If low order bit of sequence is 0

Or

2) the value has changed

       for_each_possible_cpu(cpu) {
                seqcount_t *s = &per_cpu(xt_recseq, cpu);
                 u32 seq = raw_read_seqcount(s) ;

                 if (seq & 1) {
                        do {
                            cpu_relax();
                          } while (seq == raw_read_seqcount(s));

Thanks !