[PATCH] drm/gud: avoid divide error in gud_stats_debugfs

[PATCH] drm/gud: avoid divide error in gud_stats_debugfs

[YuanZhe Liu]

gud_stats_debugfs() computes the compression ratio by dividing the
accumulated uncompressed length by stats_actual_length. That counter is
zero until the first framebuffer flush has completed, so reading the
stats debugfs file on a display which negotiates LZ4 compression raises
a divide error before any frame has been sent. The wrap-around reset in
gud_flush_rect() also clears the counter for a moment, which a
concurrent read can observe.

Copy the counter into a local variable and skip the ratio computation
when it is zero.

Fixes: 40e1a70b4aed ("drm: Add GUD USB Display driver")
Cc: stable@vger.kernel.org
Co-developed-by: Bin Yu <byu@xidian.edu.cn>
Signed-off-by: Bin Yu <byu@xidian.edu.cn>
Co-developed-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn>
---
 drivers/gpu/drm/gud/gud_drv.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
index 3a1b9e2a2eaa..85ddb302d2ba 100644
--- a/drivers/gpu/drm/gud/gud_drv.c
+++ b/drivers/gpu/drm/gud/gud_drv.c
@@ -329,10 +329,17 @@ static int gud_stats_debugfs(struct seq_file *m, void *data)
 	seq_puts(m, "\n");
 
 	if (gdrm->compression) {
+		u64 actual_length = gdrm->stats_actual_length;
 		u64 remainder;
-		u64 ratio = div64_u64_rem(gdrm->stats_length, gdrm->stats_actual_length,
-					  &remainder);
-		u64 ratio_frac = div64_u64(remainder * 10, gdrm->stats_actual_length);
+		u64 ratio, ratio_frac;
+
+		if (!actual_length) {
+			seq_puts(m, "Compression ratio: n/a\n");
+			return 0;
+		}
+
+		ratio = div64_u64_rem(gdrm->stats_length, actual_length, &remainder);
+		ratio_frac = div64_u64(remainder * 10, actual_length);
 
 		seq_printf(m, "Compression ratio: %llu.%llu\n", ratio, ratio_frac);
 	}
-- 
2.34.1

Re: [PATCH] drm/gud: avoid divide error in gud_stats_debugfs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/gud: missing READ_ONCE() on stats_actual_length in gud_stats_debugfs()
--

commit c9d9a9652db7619eedb8feff2da3862c3012fe31
Author: YuanZhe Liu <25031212351@stu.xidian.edu.cn>

drm/gud: avoid divide error in gud_stats_debugfs

This commit copies gdrm->stats_actual_length into a local variable in
gud_stats_debugfs() and skips the compression ratio division when it is zero
to avoid a divide-by-zero error before the first framebuffer flush or during
a concurrent counter wrap-around reset in gud_flush_rect().

> diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
> index 680447f426f37..cec5d64e152bb 100644
> --- a/drivers/gpu/drm/gud/gud_drv.c
> +++ b/drivers/gpu/drm/gud/gud_drv.c
> @@ -329,10 +329,17 @@ static int gud_stats_debugfs(struct seq_file *m, void *data)
>  	seq_puts(m, "\n");
>  
>  	if (gdrm->compression) {
> +		u64 actual_length = gdrm->stats_actual_length;
>  		u64 remainder;
> -		u64 ratio = div64_u64_rem(gdrm->stats_length, gdrm->stats_actual_length,
> -					  &remainder);
> -		u64 ratio_frac = div64_u64(remainder * 10, gdrm->stats_actual_length);
> +		u64 ratio, ratio_frac;
> +
> +		if (!actual_length) {
> +			seq_puts(m, "Compression ratio: n/a\n");
> +			return 0;
> +		}
> +
> +		ratio = div64_u64_rem(gdrm->stats_length, actual_length, &remainder);
> +		ratio_frac = div64_u64(remainder * 10, actual_length);

[Severity: High]
Can the compiler reload gdrm->stats_actual_length from memory after the zero
check?

Neither gud_stats_debugfs() nor gud_flush_damage() -> gud_flush_rect() holds a
shared lock when accessing gdrm->stats_actual_length:

drivers/gpu/drm/gud/gud_pipe.c:gud_flush_rect() {
	...
	gdrm->stats_length += len;
	/* Did it wrap around? */
	if (gdrm->stats_length <= len && gdrm->stats_actual_length) {
		gdrm->stats_length = len;
		gdrm->stats_actual_length = 0;
	}
	gdrm->stats_actual_length += trlen;
	...
}

Because actual_length is loaded in gud_stats_debugfs() via a plain C
assignment without READ_ONCE(), the compiler is permitted to reload
gdrm->stats_actual_length from memory when passing actual_length to
div64_u64_rem() or div64_u64(). If gud_flush_rect() concurrently resets
gdrm->stats_actual_length to 0, could the zero check observe a non-zero value
while the subsequent division reloads 0 and still triggers a divide error?

Should gud_stats_debugfs() use READ_ONCE(gdrm->stats_actual_length) (or
synchronize the stats counters between gud_flush_rect() and
gud_stats_debugfs())?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029080816.16800-1-25031212351@stu.xidian.edu.cn?part=1