linux-kernel - Re: [PATCH] blk-cgroup: don't clear stat in blkcg_reset

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: Thu, 27 Jun 2024 10:10:06 -1000
From: Tejun Heo <tj@...nel.org>
To: Li Lingfeng <lilingfeng@...weicloud.com>
Cc: josef@...icpanda.com, hch@....de, axboe@...nel.dk, longman@...hat.com,
	ming.lei@...hat.com, cgroups@...r.kernel.org,
	linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
	yangerkun@...wei.com, yukuai1@...weicloud.com, houtao1@...wei.com,
	yi.zhang@...wei.com, lilingfeng3@...wei.com
Subject: Re: [PATCH] blk-cgroup: don't clear stat in blkcg_reset_stats()

Hello,

On Thu, Jun 27, 2024 at 05:08:56PM +0800, Li Lingfeng wrote:
> The list corruption described in commit 6da668063279 ("blk-cgroup: fix
> list corruption from resetting io stat") has no effect. It's unnecessary
> to fix it.

I find this paragraph a bit confusing. At the time, it was broken, right?
And if we were to memset() now, it'd break again.

> As for cgroup v1, it does not use iostat any more after commit
> ad7c3b41e86b("blk-throttle: Fix io statistics for cgroup v1"), so using
> memset to clear iostat has no real effect.

Ah, okay, this is because we made the stats blk-throtl specific but didn't
implement ->pd_reset_stat_fn(), right?

> As for cgroup v2, it will not call blkcg_reset_stats() to corrupt the
> list.
> 
> The list of root cgroup can be used by both cgroup v1 and v2 while
> non-root cgroup can't since it must be removed before switch between
> cgroup v1 and v2.
> So it may has effect if the list of root used by cgroup v2 was corrupted
> after switching to cgroup v1, and switch back to cgroup v2 to use the
> corrupted list again.
> However, the root cgroup will not use the list any more after commit
> ef45fe470e1e("blk-cgroup: show global disk stats in root cgroup io.stat").

Hmm... I'm still having a bit of trouble following this line of argument
given that all the patch does is dropping stat clearing.

> @@ -668,7 +645,6 @@ static int blkcg_reset_stats(struct cgroup_subsys_state *css,
>  	 * anyway.  If you get hit by a race, retry.
>  	 */
>  	hlist_for_each_entry(blkg, &blkcg->blkg_list, blkcg_node) {
> -		blkg_clear_stat(blkg);
>  		for (i = 0; i < BLKCG_MAX_POLS; i++) {
>  			struct blkcg_policy *pol = blkcg_policy[i];

The patch looks fine to me although it'd be nice to follow up with a patch
to implement ->pd_reset_stat_fn() for blk-throtl. I'm not quite following
the list corruption part of argument.

Thanks.

-- 
tejun