blk-cgroup: avoid 32-bit overflow in root io.stat byte accounting

[PATCH] blk-cgroup: avoid 32-bit overflow in root io.stat byte accounting

Posted by Tao Cui 1 week, 4 days ago

From: Tao Cui <cuitao@kylinos.cn>

blkcg_fill_root_iostats() converts per-CPU sector counts to bytes with

    tmp.bytes[BLKG_IOSTAT_READ] += cpu_dkstats->sectors[STAT_READ] << 9;

but disk_stats.sectors is `unsigned long`, and the shift is carried out in
that type before the result is promoted to the u64 accumulator.  On 32-bit
kernels (unsigned long is 32 bits) this wraps, so once a per-CPU counter
reaches 2**23 sectors (~4 GiB) the computed byte count is wrong, corrupting
the root cgroup's io.stat.

Every other sector->byte conversion in the tree casts to a wide type first
((loff_t)sectors << SECTOR_SHIFT in bdev.c, (u64)max_sectors << SECTOR_SHIFT
in blk-settings.c); do the same here.

Fixes: ef45fe470e1e5 ("blk-cgroup: show global disk stats in root cgroup io.stat")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 block/blk-cgroup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index d2a1f5903f24..a778aa9d2bb9 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1112,11 +1112,11 @@ static void blkcg_fill_root_iostats(void)
 				cpu_dkstats->ios[STAT_DISCARD];
 			// convert sectors to bytes
 			tmp.bytes[BLKG_IOSTAT_READ] +=
-				cpu_dkstats->sectors[STAT_READ] << 9;
+				(u64)cpu_dkstats->sectors[STAT_READ] << SECTOR_SHIFT;
 			tmp.bytes[BLKG_IOSTAT_WRITE] +=
-				cpu_dkstats->sectors[STAT_WRITE] << 9;
+				(u64)cpu_dkstats->sectors[STAT_WRITE] << SECTOR_SHIFT;
 			tmp.bytes[BLKG_IOSTAT_DISCARD] +=
-				cpu_dkstats->sectors[STAT_DISCARD] << 9;
+				(u64)cpu_dkstats->sectors[STAT_DISCARD] << SECTOR_SHIFT;
 		}
 
 		flags = u64_stats_update_begin_irqsave(&blkg->iostat.sync);
-- 
2.43.0

Re: [PATCH] blk-cgroup: avoid 32-bit overflow in root io.stat byte accounting

Posted by Bart Van Assche 1 week, 2 days ago

On 7/13/26 11:31 PM, Tao Cui wrote:
> Every other sector->byte conversion in the tree casts to a wide type first
> ((loff_t)sectors << SECTOR_SHIFT in bdev.c, (u64)max_sectors << SECTOR_SHIFT
> in blk-settings.c); do the same here.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH] blk-cgroup: avoid 32-bit overflow in root io.stat byte accounting

Posted by Tang Yizhou 1 week, 2 days ago

On 14/7/26 2:31 pm, Tao Cui wrote:
> From: Tao Cui <cuitao@kylinos.cn>
> 
> blkcg_fill_root_iostats() converts per-CPU sector counts to bytes with
> 
>     tmp.bytes[BLKG_IOSTAT_READ] += cpu_dkstats->sectors[STAT_READ] << 9;
> 
> but disk_stats.sectors is `unsigned long`, and the shift is carried out in
> that type before the result is promoted to the u64 accumulator.  On 32-bit
> kernels (unsigned long is 32 bits) this wraps, so once a per-CPU counter
> reaches 2**23 sectors (~4 GiB) the computed byte count is wrong, corrupting
> the root cgroup's io.stat.
> 
> Every other sector->byte conversion in the tree casts to a wide type first
> ((loff_t)sectors << SECTOR_SHIFT in bdev.c, (u64)max_sectors << SECTOR_SHIFT
> in blk-settings.c); do the same here.
> 
> Fixes: ef45fe470e1e5 ("blk-cgroup: show global disk stats in root cgroup io.stat")
> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> ---
>  block/blk-cgroup.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index d2a1f5903f24..a778aa9d2bb9 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -1112,11 +1112,11 @@ static void blkcg_fill_root_iostats(void)
>  				cpu_dkstats->ios[STAT_DISCARD];
>  			// convert sectors to bytes
>  			tmp.bytes[BLKG_IOSTAT_READ] +=
> -				cpu_dkstats->sectors[STAT_READ] << 9;
> +				(u64)cpu_dkstats->sectors[STAT_READ] << SECTOR_SHIFT;
>  			tmp.bytes[BLKG_IOSTAT_WRITE] +=
> -				cpu_dkstats->sectors[STAT_WRITE] << 9;
> +				(u64)cpu_dkstats->sectors[STAT_WRITE] << SECTOR_SHIFT;
>  			tmp.bytes[BLKG_IOSTAT_DISCARD] +=
> -				cpu_dkstats->sectors[STAT_DISCARD] << 9;
> +				(u64)cpu_dkstats->sectors[STAT_DISCARD] << SECTOR_SHIFT;
>  		}
>  
>  		flags = u64_stats_update_begin_irqsave(&blkg->iostat.sync);

Reviewed-by: Tang Yizhou <yizhou.tang@shopee.com>

-- 
Best Regards,
Yi