Re: [PATCH v2] mm/page-writeback: avoid 32-bit overflow in dirty threshold shares

[Jan Kara]

On Wed 21-10-26 06:06:54, Hungyu Lin wrote:
> The ratios used by __wb_calc_thresh() are scaled to parts per million,
> but the threshold and ratio operands are unsigned long. On 32-bit
> kernels, their multiplication can wrap before the result is assigned
> to u64.
> 
> For example, with a threshold of 25600 pages and the default maximum
> ratio of 1000000, wb_max_thresh becomes 4125 pages instead of 25600.
> The shared threshold and minimum share calculations can overflow in
> the same way, producing incorrect per-writeback dirty limits.
> 
> Use a u64 threshold for the three ratio multiplications and div_u64()
> to retain the existing division order and rounding.
> 
> Fixes: ae82291e9ca4 ("mm: use part per 1000000 for bdi ratios")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Hungyu Lin <dennylin0707@xxxxxxxxx>

Looks ok. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
> Changes in v2:
> - Use a u64 threshold variable instead of repeated casts, as suggested by
>   Andrew Morton.
> 
>  mm/page-writeback.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index eeab25d6ce36..1d6aab8781a5 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -883,6 +883,7 @@ static unsigned long __wb_calc_thresh(struct dirty_throttle_control *dtc,
>  {
>  	struct wb_domain *dom = dtc_dom(dtc);
>  	struct bdi_writeback *wb = dtc->wb;
> +	u64 threshold = thresh;
>  	u64 wb_thresh;
>  	u64 wb_max_thresh;
>  	unsigned long numerator, denominator;
> @@ -894,13 +895,14 @@ static unsigned long __wb_calc_thresh(struct dirty_throttle_control *dtc,
>  	fprop_fraction_percpu(&dom->completions, dtc->wb_completions,
>  			      &numerator, &denominator);
>  
> -	wb_thresh = (thresh * (100 * BDI_RATIO_SCALE - bdi_min_ratio)) / (100 * BDI_RATIO_SCALE);
> +	wb_thresh = div_u64(threshold * (100 * BDI_RATIO_SCALE - bdi_min_ratio),
> +			    100 * BDI_RATIO_SCALE);
>  	wb_thresh *= numerator;
>  	wb_thresh = div64_ul(wb_thresh, denominator);
>  
>  	wb_min_max_ratio(wb, &wb_min_ratio, &wb_max_ratio);
>  
> -	wb_thresh += (thresh * wb_min_ratio) / (100 * BDI_RATIO_SCALE);
> +	wb_thresh += div_u64(threshold * wb_min_ratio, 100 * BDI_RATIO_SCALE);
>  
>  	/*
>  	 * It's very possible that wb_thresh is close to 0 not because the
> @@ -916,7 +918,7 @@ static unsigned long __wb_calc_thresh(struct dirty_throttle_control *dtc,
>  			wb_thresh = max(wb_thresh, (thresh - dtc->dirty) / 8);
>  	}
>  
> -	wb_max_thresh = thresh * wb_max_ratio / (100 * BDI_RATIO_SCALE);
> +	wb_max_thresh = div_u64(threshold * wb_max_ratio, 100 * BDI_RATIO_SCALE);
>  	if (wb_thresh > wb_max_thresh)
>  		wb_thresh = wb_max_thresh;
>  
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR