| 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
| ||
|
Message-ID: <20250916163840.GL12516@e132581.arm.com>
Date: Tue, 16 Sep 2025 17:38:40 +0100
From: Leo Yan <leo.yan@....com>
To: Suzuki K Poulose <suzuki.poulose@....com>
Cc: Sean Anderson <sean.anderson@...ux.dev>, coresight@...ts.linaro.org,
linux-arm-kernel@...ts.infradead.org,
Yeoreum Yun <yeoreum.yun@....com>,
Mike Leach <mike.leach@...aro.org>,
Linu Cherian <lcherian@...vell.com>, linux-kernel@...r.kernel.org,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
James Clark <james.clark@...aro.org>
Subject: Re: [PATCH v3] coresight: Fix possible deadlock in coresight_panic_cb
On Tue, Sep 16, 2025 at 05:09:01PM +0100, Suzuki Kuruppassery Poulose wrote:
[...]
> > From ea78dd22cbdd97f709c5991d5bd3be97be6e137e Mon Sep 17 00:00:00 2001
> > From: Sean Anderson <sean.anderson@...ux.dev>
> > Date: Tue, 16 Sep 2025 16:03:58 +0100
> > Subject: [PATCH] coresight: Fix possible deadlock in coresight_panic_cb()
> >
> > coresight_panic_cb() is called during a panic. It invokes
> > bus_for_each_dev(), which then calls bus_to_subsys() and takes the
> > 'bus_kset->list_lock'. If a panic occurs after the lock has been
> > acquired, it can lead to a deadlock.
> >
> > Instead of using a common panic notifier to iterate the bus, this commit
> > directly registers the TMC device's panic notifier. This avoids bus
> > iteration and effectively eliminates the race condition that could cause
> > the deadlock.
>
> Well, if you are going that far, why not register the notifier from
> coresight-core ?
I have thought this but gave up.
When register a panic's notifier, it does not provide an argument for
passing a private data. So the code below uses container_of() to convert
notifier block pointer to the TMC driver data, as a result, the code is
specific to TMC driver.
I searched the kernel but did not find a case for passing private data
for panic notifier (note, this should be only an issue for panic
notifier but not for common notifiers).
> We could always check the panic_ops->sync, and
> then register/unregister the callback. Add the notifer to the csdev.
> Otherwise, there is no point in keeping the panic_ops in
> coresight_device, it is just for the TMC device.
ETF and ETR devices both support panic, this is why I think keeping
`panic_ops` callbacks can benefit code consolidation in
coresight-tmc-core.c (you could see the difference between my proposed
change and Sean's v3 patch).
But it is fine for me to drop `panic_ops` in coresight_device. At
least, this is much better than adding lock and list in patch v2.
Thanks,
Leo
> > Fixes: 46006ceb5d02 ("coresight: core: Add provision for panic callbacks")
> > Signed-off-by: Sean Anderson <sean.anderson@...ux.dev>
> > Signed-off-by: Leo Yan <leo.yan@....com>
> > ---
> > drivers/hwtracing/coresight/coresight-core.c | 42 -------------------
> > .../hwtracing/coresight/coresight-tmc-core.c | 26 ++++++++++++
> > drivers/hwtracing/coresight/coresight-tmc.h | 2 +
> > 3 files changed, 28 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> > index 3267192f0c1c..cb0cc8d77056 100644
> > --- a/drivers/hwtracing/coresight/coresight-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > @@ -21,7 +21,6 @@
> > #include <linux/property.h>
> > #include <linux/delay.h>
> > #include <linux/pm_runtime.h>
> > -#include <linux/panic_notifier.h>
> > #include "coresight-etm-perf.h"
> > #include "coresight-priv.h"
> > @@ -1566,36 +1565,6 @@ 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);
> > -
> > - return 0;
> > -}
> > -
> > -static struct notifier_block coresight_notifier = {
> > - .notifier_call = coresight_panic_cb,
> > -};
> > -
> > static int __init coresight_init(void)
> > {
> > int ret;
> > @@ -1608,20 +1577,11 @@ static int __init coresight_init(void)
> > if (ret)
> > goto exit_bus_unregister;
> > - /* Register function to be called for panic */
> > - ret = atomic_notifier_chain_register(&panic_notifier_list,
> > - &coresight_notifier);
> > - if (ret)
> > - goto exit_perf;
> > -
> > /* initialise the coresight syscfg API */
> > ret = cscfg_init();
> > if (!ret)
> > return 0;
> > - atomic_notifier_chain_unregister(&panic_notifier_list,
> > - &coresight_notifier);
> > -exit_perf:
> > etm_perf_exit();
> > exit_bus_unregister:
> > bus_unregister(&coresight_bustype);
> > @@ -1631,8 +1591,6 @@ static int __init coresight_init(void)
> > static void __exit coresight_exit(void)
> > {
> > cscfg_exit();
> > - atomic_notifier_chain_unregister(&panic_notifier_list,
> > - &coresight_notifier);
> > etm_perf_exit();
> > bus_unregister(&coresight_bustype);
> > }
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> > index 36599c431be6..108ed9daf56d 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> > @@ -21,6 +21,7 @@
> > #include <linux/slab.h>
> > #include <linux/dma-mapping.h>
> > #include <linux/spinlock.h>
> > +#include <linux/panic_notifier.h>
> > #include <linux/pm_runtime.h>
> > #include <linux/of.h>
> > #include <linux/of_address.h>
> > @@ -769,6 +770,21 @@ static void register_crash_dev_interface(struct tmc_drvdata *drvdata,
> > "Valid crash tracedata found\n");
> > }
> > +static int tmc_panic_cb(struct notifier_block *nb, unsigned long v, void *p)
> > +{
> > + struct tmc_drvdata *drvdata = container_of(nb, struct tmc_drvdata,
> > + panic_notifier);
> > + struct coresight_device *csdev = drvdata->csdev;
> > +
> > + if (coresight_get_mode(csdev) == CS_MODE_DISABLED)
> > + return 0;
> > +
> > + if (panic_ops(csdev))
> > + panic_ops(csdev)->sync(csdev);
> > +
> > + return 0;
> > +}
> > +
> > static int __tmc_probe(struct device *dev, struct resource *res)
> > {
> > int ret = 0;
> > @@ -885,6 +901,12 @@ static int __tmc_probe(struct device *dev, struct resource *res)
> > goto out;
> > }
> > + if (panic_ops(drvdata->csdev)) {
> > + drvdata->panic_notifier.notifier_call = tmc_panic_cb;
> > + atomic_notifier_chain_register(&panic_notifier_list,
> > + &drvdata->panic_notifier);
> > + }
> > +
> > out:
> > if (is_tmc_crashdata_valid(drvdata) &&
> > !tmc_prepare_crashdata(drvdata))
> > @@ -929,6 +951,10 @@ static void __tmc_remove(struct device *dev)
> > {
> > struct tmc_drvdata *drvdata = dev_get_drvdata(dev);
> > + if (panic_ops(drvdata->csdev))
> > + atomic_notifier_chain_unregister(&panic_notifier_list,
> > + &drvdata->panic_notifier);
> > +
> > /*
> > * Since misc_open() holds a refcount on the f_ops, which is
> > * etb fops in this case, device is there until last file
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> > index cbb4ba439158..873c5427673c 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc.h
> > +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> > @@ -243,6 +243,7 @@ struct tmc_resrv_buf {
> > * (after crash) by default.
> > * @crash_mdata: Reserved memory for storing tmc crash metadata.
> > * Used by ETR/ETF.
> > + * @panic_notifier: Notifier used to clean up during a panic
> > */
> > struct tmc_drvdata {
> > struct clk *atclk;
> > @@ -273,6 +274,7 @@ struct tmc_drvdata {
> > struct etr_buf *perf_buf;
> > struct tmc_resrv_buf resrv_buf;
> > struct tmc_resrv_buf crash_mdata;
> > + struct notifier_block panic_notifier;
> > };
> > struct etr_buf_operations {
>
Powered by blists - more mailing lists