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

[Tang Yizhou]

On 14/7/26 2:31 pm, Tao Cui wrote:
> From: Tao Cui <cuitao@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>

-- 
Best Regards,
Yi