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] [day] [month] [year] [list]

Message-Id: <20240822103749.228468-1-takakura@valinux.co.jp>
Date: Thu, 22 Aug 2024 19:37:49 +0900
From: takakura@...inux.co.jp
To: edumazet@...gle.com
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()

Hi Eric,

Thanks for taking a look at the patch! And sorry that I see that I was 
missing the point of the synchronization.

On 2024-08-22 6:03 Eric Dumazet wrote:
>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.

Yes, I was completely wrong about why the synchronization is required...

>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.

I see. And if we were to use cmpxchg128, we would also need to come up with 
the way for xt_replace_table()'s synchronization which I guess the current 
per cpu sequence is more suited.

Sincerely,
Ryo Takakura