netdev - Re: [PATCH] netfilter: Don't track counter updates of do_add

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]

Message-ID: <CANn89iJGUCp-4fRJWzwNzakyNZM=_mSNjX=_OUT8WJW-+isAfA@mail.gmail.com>
Date: Thu, 22 Aug 2024 08:03:41 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: takakura@...inux.co.jp
Cc: pablo@...filter.org, kadlec@...filter.org, davem@...emloft.net, 
	dsahern@...nel.org, kuba@...nel.org, pabeni@...hat.com, fw@...len.de, 
	netfilter-devel@...r.kernel.org, coreteam@...filter.org, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] netfilter: Don't track counter updates of do_add_counters()

On Thu, Aug 22, 2024 at 6:36 AM <takakura@...inux.co.jp> wrote:
>
> From: Ryo Takakura <takakura@...inux.co.jp>
>
> While adding counters in do_add_counters(), we call
> xt_write_recseq_begin/end to indicate that counters are being updated.
> Updates are being tracked so that the counters retrieved by get_counters()
> will reflect concurrent updates.
>
> However, there is no need to track the updates done by do_add_counters() as
> both do_add_counters() and get_counters() acquire per ipv4,ipv6,arp mutex
> beforehand which prevents concurrent update and retrieval between the two.
>
> Moreover, as the xt_write_recseq_begin/end is shared among ipv4,ipv6,arp,
> do_add_counters() called by one of ipv4,ipv6,arp can falsely delay the
> synchronization of concurrent get_counters() or xt_replace_table() called
> by any other than the one calling do_add_counters().
>
> So remove xt_write_recseq_begin/end from do_add_counters() for ipv4,ipv6,arp.

Completely wrong patch.

There is no way we can update pairs of 64bit counters without any
synchronization.

This is per cpu sequence, the 'shared among ipv4,ipv6,arp' part is moot.

We could use cmpxchg128 on 64bit arches, but I suspect there will be
no improvement.