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

[sashiko-bot@kernel.org]

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?

[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;
    ...
}

>  }
[ ... ]
> 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;
    ...
}

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825175916.1103841-13-badal.nilawar@intel.com?part=7