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: Fri, 28 Jun 2024 11:14:08 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: Tejun Heo <tj@...nel.org>, 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,
 "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH] blk-cgroup: don't clear stat in blkcg_reset_stats()

Hi,

在 2024/06/28 4:10, Tejun Heo 写道:
> 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?

I'm afraid not... Implement pd_reset_stat_fn() or not is another
problem, this patch should be just code cleanup, not fixing any real
problems.

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

The code deleted by this patch was claimed to fix a lsit corruption,
however, the list corruption does not exist now hence related code can
be removed:

1) Take a look at blk_cgroup_bio_start, now there are two conditions
before this blkg can be added to the per_cpu list:

blk_cgroup_bio_start
  if (!cgroup_subsys_on_dfl(io_cgrp_subsys))
  -> the list will always be empty for cgroup v1
   return;
  if (!cgroup_parent(blkcg->css.cgroup))
  -> the list will always be empty for root blkg
   return;
  ...
  llist_add()

2) blkcg_reset_stats can only be called from cgroup v1 api, hence
there is nothing to be cleared for blkg_clear_stat();

3) Noted that user can switch from cgroup v2 to v1, however, we found
that user must delete all the child cg to do that, hence only root blkg
can be kept after switching to v1. And root blkg is bypassed from
blk_cgroup_bio_start(), hence no problem.

Thanks,
Kuai
> 
> Thanks.
>