Re: [PATCH 2/4] drm/amdgpu/gfx9: replace BUG_ON/BUG with WARN_ON_ONCE in ring emission

["Christian König" <christian.koenig@amd.com>]

On 4/26/26 23:52, jbmoore wrote:
> From: "John B. Moore" <jbmoore61@gmail.com>
> 
> Replace all BUG_ON() and BUG() assertions in the gfx_v9_0 ring
> emission paths with WARN_ON_ONCE() and graceful recovery.  Ten sites
> are converted across wait_reg_mem, gpu_early_init, parse_ind_reg_list,
> init_rlc_save_restore_list, kiq_read_clock, emit_ib_gfx,
> emit_ib_compute, emit_fence, get_wptr_compute, and set_wptr_compute.
> 
> These assertions guard conditions that are either:
> - Address alignment checks on a deprecated byte-swap encoding from
>   legacy pre-amdgpu hardware (bits [1:0] must be zero), or
> - Switch-case defaults that should be unreachable but are better
>   handled with dev_err + return -EINVAL than a kernel panic.
> 
> Several of the address alignment BUG_ON sites in the IB emission
> paths (emit_ib_gfx, emit_ib_compute) are reachable from unprivileged
> userspace via crafted DRM_IOCTL_AMDGPU_CS submissions, causing a
> fatal kernel panic in a scheduler worker thread.
> 
> For address checks, clear the reserved bits and proceed.  For
> unreachable switch defaults, log the error and return.  For the
> doorbell-only wptr paths, log with WARN_ONCE and return zero /
> no-op.  Ring emission callbacks return void, so force-aligning
> and proceeding is the accepted pattern.
> 
> Found by a custom amdgpu DRM ioctl fuzzer.
> 
> Fixes: b1023571479020e9 ("drm/amdgpu: implement GFX 9.0 support (v2)")
> Signed-off-by: John B. Moore <jbmoore61@gmail.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 50 +++++++++++++++++----------
>  1 file changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index 2eb32f92a..47e81c33d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -1182,8 +1182,8 @@ static void gfx_v9_0_wait_reg_mem(struct amdgpu_ring *ring, int eng_sel,
>  				 WAIT_REG_MEM_FUNCTION(3) |  /* equal */
>  				 WAIT_REG_MEM_ENGINE(eng_sel)));
>  
> -	if (mem_space)
> -		BUG_ON(addr0 & 0x3); /* Dword align */
> +	if (mem_space && WARN_ON_ONCE(addr0 & 0x3))
> +		addr0 &= ~0x3; /* Force dword align */

Same comment as with the SDMA, please use only WARN_ON() and not WARN_ON_ONCE() and don't mask the bits.

>  	amdgpu_ring_write(ring, addr0);
>  	amdgpu_ring_write(ring, addr1);
>  	amdgpu_ring_write(ring, ref);
> @@ -2107,8 +2107,10 @@ static int gfx_v9_0_gpu_early_init(struct amdgpu_device *adev)
>  			return err;
>  		break;
>  	default:
> -		BUG();
> -		break;
> +		dev_err(adev->dev,
> +			"unsupported GFX IP version 0x%x for gfx_v9_0\n",
> +			amdgpu_ip_version(adev, GC_HWIP, 0));
> +		return -EINVAL;

Mhm, that is most likely a bad idea.

The BUG() here is perfectly justified because the system will crash later on anyway and this way we at least stop at the earliest possible time.

Regards,
Christian.

>  	}
>  
>  	adev->gfx.config.gb_addr_config = gb_addr_config;
> @@ -2808,7 +2810,8 @@ static void gfx_v9_1_parse_ind_reg_list(int *register_list_format,
>  					break;
>  			}
>  
> -			BUG_ON(idx >= unique_indirect_reg_count);
> +			if (WARN_ON_ONCE(idx >= unique_indirect_reg_count))
> +				break;
>  
>  			if (!unique_indirect_regs[idx])
>  				unique_indirect_regs[idx] = register_list_format[indirect_offset];
> @@ -2885,7 +2888,8 @@ static int gfx_v9_1_init_rlc_save_restore_list(struct amdgpu_device *adev)
>  			}
>  		}
>  
> -		BUG_ON(j >= unique_indirect_reg_count);
> +		if (WARN_ON_ONCE(j >= unique_indirect_reg_count))
> +			break;
>  
>  		i++;
>  	}
> @@ -4209,7 +4213,8 @@ static uint64_t gfx_v9_0_kiq_read_clock(struct amdgpu_device *adev)
>  	struct amdgpu_kiq *kiq = &adev->gfx.kiq[0];
>  	struct amdgpu_ring *ring = &kiq->ring;
>  
> -	BUG_ON(!ring->funcs->emit_rreg);
> +	if (WARN_ON_ONCE(!ring->funcs->emit_rreg))
> +		return 0;
>  
>  	spin_lock_irqsave(&kiq->ring_lock, flags);
>  	if (amdgpu_device_wb_get(adev, &reg_val_offs)) {
> @@ -5431,7 +5436,8 @@ static void gfx_v9_0_ring_emit_ib_gfx(struct amdgpu_ring *ring,
>  	}
>  
>  	amdgpu_ring_write(ring, header);
> -	BUG_ON(ib->gpu_addr & 0x3); /* Dword align */
> +	if (WARN_ON_ONCE(ib->gpu_addr & 0x3)) /* Dword align */
> +		ib->gpu_addr &= ~0x3ULL;
>  	amdgpu_ring_write(ring,
>  #ifdef __BIG_ENDIAN
>  		(2 << 0) |
> @@ -5527,7 +5533,8 @@ static void gfx_v9_0_ring_emit_ib_compute(struct amdgpu_ring *ring,
>  	}
>  
>  	amdgpu_ring_write(ring, PACKET3(PACKET3_INDIRECT_BUFFER, 2));
> -	BUG_ON(ib->gpu_addr & 0x3); /* Dword align */
> +	if (WARN_ON_ONCE(ib->gpu_addr & 0x3)) /* Dword align */
> +		ib->gpu_addr &= ~0x3ULL;
>  	amdgpu_ring_write(ring,
>  #ifdef __BIG_ENDIAN
>  				(2 << 0) |
> @@ -5567,10 +5574,13 @@ static void gfx_v9_0_ring_emit_fence(struct amdgpu_ring *ring, u64 addr,
>  	 * the address should be Qword aligned if 64bit write, Dword
>  	 * aligned if only send 32bit data low (discard data high)
>  	 */
> -	if (write64bit)
> -		BUG_ON(addr & 0x7);
> -	else
> -		BUG_ON(addr & 0x3);
> +	if (write64bit) {
> +		if (WARN_ON_ONCE(addr & 0x7))
> +			addr &= ~0x7ULL;
> +	} else {
> +		if (WARN_ON_ONCE(addr & 0x3))
> +			addr &= ~0x3ULL;
> +	}
>  	amdgpu_ring_write(ring, lower_32_bits(addr));
>  	amdgpu_ring_write(ring, upper_32_bits(addr));
>  	amdgpu_ring_write(ring, lower_32_bits(seq));
> @@ -5639,10 +5649,13 @@ static u64 gfx_v9_0_ring_get_wptr_compute(struct amdgpu_ring *ring)
>  	u64 wptr;
>  
>  	/* XXX check if swapping is necessary on BE */
> -	if (ring->use_doorbell)
> +	if (ring->use_doorbell) {
>  		wptr = atomic64_read((atomic64_t *)ring->wptr_cpu_addr);
> -	else
> -		BUG();
> +	} else {
> +		WARN_ONCE(1, "gfx_v9_0: non-doorbell wptr read on ring %s, only doorbell method supported on gfx9\n",
> +			  ring->name);
> +		wptr = 0;
> +	}
>  	return wptr;
>  }
>  
> @@ -5654,8 +5667,9 @@ static void gfx_v9_0_ring_set_wptr_compute(struct amdgpu_ring *ring)
>  	if (ring->use_doorbell) {
>  		atomic64_set((atomic64_t *)ring->wptr_cpu_addr, ring->wptr);
>  		WDOORBELL64(ring->doorbell_index, ring->wptr);
> -	} else{
> -		BUG(); /* only DOORBELL method supported on gfx9 now */
> +	} else {
> +		WARN_ONCE(1, "gfx_v9_0: non-doorbell wptr write on ring %s, only doorbell method supported on gfx9\n",
> +			  ring->name);
>  	}
>  }
>