Re: [PATCH v2 2/5] drm/xe: Guard against NULL GT in xe_pmu.c

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Sep 26, 2025 at 08:09:20PM +0000, Jonathan Cavitt wrote:
> Static analysis reveals the following issue:
> xe_device_get_gt is theoretically able to return NULL in some cases, but
> several use cases don't check the return value before performing a
> dereference, resulting in a NULL pointer dereference.
> 
> Add guards against this in xe_pmu.c:
> - Use xe_root_mmio_gt instead of xe_device_get_gt for the gt id 0 case.
> - Modify event_supported and event_gt_forcewake to take a
>   struct xe_gt gt pointer.
> - Only perform xe_force_wake_put in xe_pmu_event_destroy if gt exists.
> 
> v2:
> - Pass xe_gt to event_gt_forcewake (Michal)
>   - Above necessitated passing xe_gt to event_supported (jcavitt)
> - Only perform xe_force_wake_put in xe_pmu_event_destroy if gt exists.
> 
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pmu.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
> index cab51d826345..d54981510a43 100644
> --- a/drivers/gpu/drm/xe/xe_pmu.c
> +++ b/drivers/gpu/drm/xe/xe_pmu.c
> @@ -130,18 +130,15 @@ static bool is_gt_frequency_event(struct perf_event *event)
>  	       id == XE_PMU_EVENT_GT_REQUESTED_FREQUENCY;
>  }
>  
> -static bool event_gt_forcewake(struct perf_event *event)
> +static bool event_gt_forcewake(struct perf_event *event, struct xe_gt *gt)
>  {
>  	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
>  	u64 config = event->attr.config;
> -	struct xe_gt *gt;
>  	unsigned int *fw_ref;
>  
>  	if (!is_engine_event(config) && !is_gt_frequency_event(event))
>  		return true;
>  
> -	gt = xe_device_get_gt(xe, config_to_gt_id(config));
> -
>  	fw_ref = kzalloc(sizeof(*fw_ref), GFP_KERNEL);
>  	if (!fw_ref)
>  		return false;
> @@ -157,11 +154,10 @@ static bool event_gt_forcewake(struct perf_event *event)
>  	return true;
>  }
>  
> -static bool event_supported(struct xe_pmu *pmu, unsigned int gt_id,
> +static bool event_supported(struct xe_pmu *pmu, struct xe_gt *gt,
>  			    unsigned int id)
>  {
>  	struct xe_device *xe = container_of(pmu, typeof(*xe), pmu);
> -	struct xe_gt *gt = xe_device_get_gt(xe, gt_id);
>  
>  	if (!gt)
>  		return false;
> @@ -219,7 +215,10 @@ static void xe_pmu_event_destroy(struct perf_event *event)
>  
>  	if (fw_ref) {
>  		gt = xe_device_get_gt(xe, config_to_gt_id(event->attr.config));
> -		xe_force_wake_put(gt_to_fw(gt), *fw_ref);
> +
> +		if (gt)
> +			xe_force_wake_put(gt_to_fw(gt), *fw_ref);
> +
>  		kfree(fw_ref);
>  		event->pmu_private = NULL;
>  	}
> @@ -233,7 +232,8 @@ static int xe_pmu_event_init(struct perf_event *event)
>  {
>  	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
>  	struct xe_pmu *pmu = &xe->pmu;
> -	unsigned int id, gt;
> +	unsigned int id, gt_id;
> +	struct xe_gt *gt;
>  
>  	if (!pmu->registered)
>  		return -ENODEV;
> @@ -248,8 +248,9 @@ static int xe_pmu_event_init(struct perf_event *event)
>  	if (event->cpu < 0)
>  		return -EINVAL;
>  
> -	gt = config_to_gt_id(event->attr.config);
> +	gt_id = config_to_gt_id(event->attr.config);
>  	id = config_to_event_id(event->attr.config);
> +	gt = xe_device_get_gt(xe, gt_id);
>  	if (!event_supported(pmu, gt, id))
>  		return -ENOENT;
>  
> @@ -262,7 +263,7 @@ static int xe_pmu_event_init(struct perf_event *event)
>  	if (!event->parent) {
>  		drm_dev_get(&xe->drm);
>  		xe_pm_runtime_get(xe);
> -		if (!event_gt_forcewake(event)) {
> +		if (!event_gt_forcewake(event, gt)) {
>  			xe_pm_runtime_put(xe);
>  			drm_dev_put(&xe->drm);
>  			return -EINVAL;
> @@ -443,13 +444,18 @@ static ssize_t event_attr_show(struct device *dev,
>  				       struct attribute *attr, int idx) \
>  	{								\
>  		struct perf_pmu_events_attr *pmu_attr;			\
> +		struct xe_device *xe;					\
>  		struct xe_pmu *pmu;					\
> +		struct xe_gt *gt;					\
>  									\
>  		pmu_attr = container_of(attr, typeof(*pmu_attr), attr.attr); \
>  		pmu = container_of(dev_get_drvdata(kobj_to_dev(kobj)),	\
>  				   typeof(*pmu), base);			\
>  									\
> -		return event_supported(pmu, 0, id_) ? attr->mode : 0;	\
> +		xe = container_of(pmu, typeof(*xe), pmu);		\
> +		gt = xe_root_mmio_gt(xe);				\
> +									\
> +		return event_supported(pmu, gt, id_) ? attr->mode : 0;	\

This case here highlights a big problem with this series, that is the
Michal's comment that was ignored.

event_supported doesn't care about the mmio. it is not getting the gt
because of the mmio area. It is a case where it will receive an id
and you need to lookup for the id and see if that GT exists or not.

That has nothing to do with the mmio itself.

I echo Michal comment on have 2 variants, when that get_gt that never
returns NULL and you mark cases like false positive.

And another variant for lookup-gt where it can return NULL and you need
to check. Intended for cases like this PMU that has nothing to do
with MMIO.

>  	}								\
>  	static const struct attribute_group pmu_group_ ##v_ = {		\
>  		.name = "events",					\
> @@ -497,7 +503,7 @@ static const struct attribute_group *pmu_events_attr_update[] = {
>  static void set_supported_events(struct xe_pmu *pmu)
>  {
>  	struct xe_device *xe = container_of(pmu, typeof(*xe), pmu);
> -	struct xe_gt *gt = xe_device_get_gt(xe, 0);
> +	struct xe_gt *gt = xe_root_mmio_gt(xe);
>  
>  	if (!xe->info.skip_guc_pc) {
>  		pmu->supported_events |= BIT_ULL(XE_PMU_EVENT_GT_C6_RESIDENCY);
> -- 
> 2.43.0
>