linux-kernel - Re: [PATCH] blk-cgroup: add spin_lock for u64_stats

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: <ZowxtI69yd4IexQY@slm.duckdns.org>
Date: Mon, 8 Jul 2024 08:36:36 -1000
From: "tj@...nel.org" <tj@...nel.org>
To: Boy Wu (吳勃誼) <Boy.Wu@...iatek.com>
Cc: "boris@....io" <boris@....io>,
	"linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"cgroups@...r.kernel.org" <cgroups@...r.kernel.org>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"axboe@...nel.dk" <axboe@...nel.dk>,
	Iverlin Wang (王苳霖) <Iverlin.Wang@...iatek.com>,
	"josef@...icpanda.com" <josef@...icpanda.com>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
	"matthias.bgg@...il.com" <matthias.bgg@...il.com>,
	"angelogioacchino.delregno@...labora.com" <angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH] blk-cgroup: add spin_lock for u64_stats_update

Hello,

On Mon, Jul 08, 2024 at 02:00:42AM +0000, Boy Wu (吳勃誼) wrote:
> When accessing /sys/fs/cgroup/io.stat, blkcg_print_stat will be called,
> and there is a small chance that four processes on each CPU core are
> accessing /sys/fs/cgroup/io.stat, which means four CPUs are calling
> blkcg_print_stat. As a result, blkcg_fill_root_iostats will be called
> simultaneously. However, u64_stats_update_begin_irqsave and
> u64_stats_update_end_irqrestore are not protect by spin_locks, so there
> is a small chance that the sync.seq.sequence will be an odd number
> after u64_stats_update_end_irqrestore due to the concurrent CPUs acess,
> because sync.seq.sequence plus one is not an atomic operation.
> 
> do_raw_write_seqcount_begin():
> /usr/src/kernel/common/include/linux/seqlock.h:469
> c05e5cfc:       e5963030        ldr     r3, [r6, #48]   ; 0x30
> c05e5d00:       e2833001        add     r3, r3, #1
> c05e5d04:       e5863030        str     r3, [r6, #48]   ; 0x30
> /usr/src/kernel/common/include/linux/seqlock.h:470
> c05e5d08:       f57ff05a        dmb     ishst
> 
> do_raw_write_seqcount_end():
> /usr/src/kernel/common/include/linux/seqlock.h:489
> c05e5d30:       f57ff05a        dmb     ishst
> /usr/src/kernel/common/include/linux/seqlock.h:490
> c05e5d34:       e5963030        ldr     r3, [r6, #48]   ; 0x30
> c05e5d38:       e2833001        add     r3, r3, #1
> c05e5d3c:       e5863030        str     r3, [r6, #48]   ; 0x30
> 
> To prevent this problem, I added spin_locks in blkcg_fill_root_iostats,
> and this solution works fine to me when I use the stress-ng --sysfs
> test.

Ah, okay, so, there are two usages of u64_sync synchronization - one is for
blkg->iostat_cpu and the other for blkg->iostat. The former makes sense and
is safe as there can ever be only one updater with irq disabled. The latter
doesn't work as there can be multiple updaters and should be removed (and
replaced with spinlock). There's already blkg_stat_lock which probably was
added for a similar reason in __blkcg_rstat_flush(). Can you please update
the patch to remove the u64_sync usage for blkg->iostat and replace it with
blkg_stat_lock?

Thanks.

-- 
tejun