Re: [PATCH v2 07/11] drm/xe/cper: Allow hardware error CPER reporting from xe_log

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 25-08-2026 23:24, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Blindly casting and passing the optional `data` parameter in `log_emit_cper` to hardware CPER emission paths causes kernel crashes if the pointer is NULL or an ERR_PTR.
> - [High] Synchronous hardware CPER emission inside the `xe_log` backend introduces blocking and sleepable operations into general-purpose logging paths, leading to deadlocks and panics when called from atomic contexts.
> - [Medium] Missing payload length validation before casting `data` to `struct xe_ras_error_class *` allows out-of-bounds memory access.
> --
>
> commit cde7fec12b9f07e68b4ed966ff02eeef7dd8e6db
> Author: Badal Nilawar <badal.nilawar@intel.com>
>
> drm/xe/cper: Allow hardware error CPER reporting from xe_log
>      
> Add a public wrapper xe_emit_hardware_error_cper() to enable xe_log.c to
> emit hardware error CPER records. This hooks into log_emit_cper() to report
> hardware signatures.
>
>> diff --git a/drivers/gpu/drm/xe/xe_log.c b/drivers/gpu/drm/xe/xe_log.c
>> index 5549ef6966fd8..c78fc195c55bd 100644
>> --- a/drivers/gpu/drm/xe/xe_log.c
>> +++ b/drivers/gpu/drm/xe/xe_log.c
> [ ... ]
>> @@ -10,15 +10,25 @@
>>   
>>   #include "xe_device.h"
>>   #include "xe_log.h"
>> +#include "xe_ras.h"
>>   #include "xe_printk.h"
>>   
>> +static bool is_hw_sigid(enum xe_sigid sigid)
>> +{
>> +	return (int)sigid >= INTEL_SIGID_GPU_XE_HARDWARE_START;
>> +}
>> +
>>   static void log_emit_cper(struct pci_dev *pdev, int cper_sev, enum xe_sigid sigid,
>>   			  u32 component, u32 location, const void *data, size_t len,
>>   			  struct va_format *vaf)
>>   {
>>   	KUNIT_STATIC_STUB_REDIRECT(log_emit_cper, pdev, cper_sev, sigid,
>>   				   component, location, data, len, vaf);
>> -	/* TODO */
>> +	/* TODO software CPER */
>> +
>> +	if (is_hw_sigid(sigid))
>> +		xe_emit_hardware_error_cper(pdev, cper_sev, sigid,
>> +					    (struct xe_ras_error_class *)data);
> [Severity: High]
> Will this block or sleep when called from atomic contexts?
>
> When a caller invokes xe_log macros (such as xe_log_comp_fatal()) from an
> interrupt handler or while holding a spinlock, this synchronously invokes
> xe_emit_hardware_error_cper() from a general-purpose logging path.
>
> The underlying hardware CPER emission path eventually calls get_counter(),
> which invokes xe_sysctrl_send_command(). That function explicitly checks
> for sleep and acquires a mutex:
>
> drivers/gpu/drm/xe/xe_sysctrl_mailbox.c:xe_sysctrl_send_command() {
>      might_sleep();
>      ...
>      guard(mutex)(&sc->cmd_lock);
>      ...
> }
>
> Should this operation be deferred to a workqueue to avoid deadlocks when
> logging from atomic contexts?

Current HW CPER users (xe_ras_counter_threshold_crossed() and 
xe_ras_process_errors()) are not IRQ-context paths and already issue 
mailbox commands through xe_sysctrl_send_command() before reaching the 
logging code.

As such, the CPER emission path does not introduce a new sleepable 
context requirement.

>
> [Severity: Medium]
> Could this cause an out-of-bounds read if the provided payload is smaller
> than the expected structure?
>
> The __xe_log_emit() API accepts an opaque data pointer alongside a length
> parameter. Since this cast ignores the len parameter, if a caller provides a
> valid payload that is smaller than sizeof(struct xe_ras_error_class),
> downstream functions like get_counter() will read past the end of the buffer
> when dereferencing it:
>
> drivers/gpu/drm/xe/xe_ras.c:get_counter() {
>      ...
>      request.counter = *counter;
>      ...
> }
This will be fixed in next rev.
>
>>   }
> [ ... ]
>> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
>> index ff9d917b8e29c..b4cdb5ec64915 100644
>> --- a/drivers/gpu/drm/xe/xe_ras.c
>> +++ b/drivers/gpu/drm/xe/xe_ras.c
>> @@ -837,6 +837,29 @@ static void emit_hw_error_cper(struct xe_device *xe,
>>   	}
>>   }
>>   
>> +/**
>> + * xe_emit_hardware_error_cper() - Emit a hardware error CPER record
>> + * @pdev: PCI device associated with the Xe device
>> + * @cper_sev: CPER severity
>> + * @sigid: Error signature identifier
>> + * @error_class: Hardware error classification details
>> + *
>> + * Emit a CPER record for a hardware error
>> + */
>> +void xe_emit_hardware_error_cper(struct pci_dev *pdev, int cper_sev, enum xe_sigid sigid,
>> +				 struct xe_ras_error_class *counter)
>> +{
>> +	struct xe_device *xe = pdev_to_xe_device(pdev);
>> +
>> +	if (!xe)
>> +		return;
>> +
>> +	if (counter && !ras_counter_is_valid(xe, counter))
>> +		return;
> [Severity: High]
> Does this check handle error pointers or NULL pointers safely?
>
> The xe_log_err() macro permits the data parameter to be an ERR_PTR
> (e.g., ERR_PTR(-EIO)). Because an error pointer is not NULL, this check
> will pass it directly to ras_counter_is_valid(), which will dereference it
> and cause a kernel panic:
>
> drivers/gpu/drm/xe/xe_ras.c:ras_counter_is_valid() {
>      u8 severity = counter->common.severity;
>      ...
> }
>
> Furthermore, if counter is NULL, this check is bypassed entirely, but the
> pointer is later unconditionally dereferenced inside get_counter() when
> emit_hw_error_cper() is called:
>
> drivers/gpu/drm/xe/xe_ras.c:get_counter() {
>      ...
>      request.counter = *counter;
>      ...
> }

This will be fixed in next rev.

Thanks,

Badal

>
>> +
>> +	emit_hw_error_cper(xe, counter, NULL, sigid, cper_sev);
>> +}
>> +
>>   /**
>>    * xe_ras_process_errors() - Process and contain hardware errors
>>    * @xe: xe device instance
> [ ... ]
>