linux-kernel - Re: [PATCH] coresight: Fix possible deadlock in coresight_panic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ab3023a8-ebe0-48ac-b725-9f5d34ba94fa@arm.com>
Date: Wed, 10 Sep 2025 17:48:53 +0100
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Sean Anderson <sean.anderson@...ux.dev>, coresight@...ts.linaro.org,
 linux-arm-kernel@...ts.infradead.org
Cc: Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 linux-kernel@...r.kernel.org, Mike Leach <mike.leach@...aro.org>,
 Linu Cherian <lcherian@...vell.com>, James Clark <james.clark@...aro.org>
Subject: Re: [PATCH] coresight: Fix possible deadlock in coresight_panic_cb

Hi Sean

On 28/08/2025 19:17, Sean Anderson wrote:
> coresight_panic_cb is called with interrupts disabled during panics.
> However, bus_for_each_dev calls bus_to_subsys which takes
> bus_kset->list_lock without disabling IRQs. This will cause a deadlock
> if a panic occurs while one of the other coresight functions that uses
> bus_for_each_dev is running.
> 
> Maintain a separate list of coresight devices to access during a panic.

Thanks for the patch. I have a minor comment.

> 
> Fixes: 46006ceb5d02 ("coresight: core: Add provision for panic callbacks")
> Signed-off-by: Sean Anderson <sean.anderson@...ux.dev>
> ---
> 
>   drivers/hwtracing/coresight/coresight-core.c | 40 ++++++++++----------
>   include/linux/coresight.h                    |  1 +
>   2 files changed, 22 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index fa758cc21827..1f1bf0e2bf92 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1315,6 +1315,9 @@ void coresight_release_platform_data(struct coresight_device *csdev,
>   		coresight_remove_conns_sysfs_group(csdev);
>   }
>   
> +static DEFINE_SPINLOCK(csdev_lock);
> +static LIST_HEAD(csdev_list);

May be add a comment here to mention why we maintain this list ?

> +
>   struct coresight_device *coresight_register(struct coresight_desc *desc)
>   {
>   	int ret;
> @@ -1374,11 +1377,16 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>   		goto out_unlock;
>   	}
>   
> +	scoped_guard(spinlock_irq, &csdev_lock)
> +		list_add(&csdev->csdev_list, &csdev_list);
> +
>   	if (csdev->type == CORESIGHT_DEV_TYPE_SINK ||
>   	    csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) {
>   		ret = etm_perf_add_symlink_sink(csdev);
>   
>   		if (ret) {
> +			scoped_guard(spinlock_irq, &csdev_lock)
> +				list_del(&csdev->csdev_list);
Could this be moved to coresight_device_release(), which will be called
when the device gets unregistered ? That way, you don't need it here
and in coresight_unregister()  too.

Rest looks good to me

Suzuki

>   			device_unregister(&csdev->dev);
>   			/*
>   			 * As with the above, all resources are free'd
> @@ -1427,6 +1435,8 @@ void coresight_unregister(struct coresight_device *csdev)
>   	coresight_remove_conns(csdev);
>   	coresight_clear_default_sink(csdev);
>   	coresight_release_platform_data(csdev, csdev->dev.parent, csdev->pdata);
> +	scoped_guard(spinlock_irq, &csdev_lock)
> +		list_del(&csdev->csdev_list);
>   	device_unregister(&csdev->dev);
>   }
>   EXPORT_SYMBOL_GPL(coresight_unregister);
> @@ -1563,28 +1573,20 @@ const struct bus_type coresight_bustype = {
>   	.name	= "coresight",
>   };
>   
> -static int coresight_panic_sync(struct device *dev, void *data)
> -{
> -	int mode;
> -	struct coresight_device *csdev;
> -
> -	/* Run through panic sync handlers for all enabled devices */
> -	csdev = container_of(dev, struct coresight_device, dev);
> -	mode = coresight_get_mode(csdev);
> -
> -	if ((mode == CS_MODE_SYSFS) || (mode == CS_MODE_PERF)) {
> -		if (panic_ops(csdev))
> -			panic_ops(csdev)->sync(csdev);
> -	}
> -
> -	return 0;
> -}
> -
>   static int coresight_panic_cb(struct notifier_block *self,
>   			       unsigned long v, void *p)
>   {
> -	bus_for_each_dev(&coresight_bustype, NULL, NULL,
> -				 coresight_panic_sync);
> +	struct coresight_device *csdev;
> +
> +	guard(spinlock)(&csdev_lock);
> +	list_for_each_entry(csdev, &csdev_list, csdev_list) {
> +		/* Run through panic sync handlers for all enabled devices */
> +		int mode = coresight_get_mode(csdev);
> +
> +		if ((mode == CS_MODE_SYSFS || mode == CS_MODE_PERF) &&
> +		    panic_ops(csdev))
> +			panic_ops(csdev)->sync(csdev);
> +	}
>   
>   	return 0;
>   }
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 4ac65c68bbf4..a5e62ebd03b5 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -302,6 +302,7 @@ struct coresight_device {
>   	/* system configuration and feature lists */
>   	struct list_head feature_csdev_list;
>   	struct list_head config_csdev_list;
> +	struct list_head csdev_list;
>   	raw_spinlock_t cscfg_csdev_lock;
>   	void *active_cscfg_ctxt;
>   };