| 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: <d4b10a7f-6823-8b6d-c63d-a1df3c719a2e@arm.com>
Date: Wed, 23 Oct 2019 14:14:20 +0100
From: Suzuki K Poulose <suzuki.poulose@....com>
To: yabinc@...gle.com, mathieu.poirier@...aro.org,
alexander.shishkin@...ux.intel.com
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] coresight: Serialize enabling/disabling a link device.
On 10/18/2019 07:14 PM, Yabin Cui wrote:
> When tracing etm data of multiple threads on multiple cpus through perf
> interface, some link devices are shared between paths of different cpus.
> It creates race conditions when different cpus wants to enable/disable
> the same link device at the same time.
>
> Example 1:
> Two cpus want to enable different ports of a coresight funnel, thus
> calling the funnel enable operation at the same time. But the funnel
> enable operation isn't reentrantable.
>
> Example 2:
> For an enabled coresight dynamic replicator with refcnt=1, one cpu wants
> to disable it, while another cpu wants to enable it. Ideally we still have
> an enabled replicator with refcnt=1 at the end. But in reality the result
> is uncertain.
>
> Since coresight devices claim themselves when enabled for self-hosted
> usage, the race conditions above usually make the link devices not usable
> after many cycles.
>
> To fix the race conditions, this patch uses spinlocks to serialize
> enabling/disabling link devices.
>
> Fixes: a06ae8609b3d ("coresight: add CoreSight core layer framework")
> Signed-off-by: Yabin Cui <yabinc@...gle.com>
> ---
>
> v3 -> v4: moved lock from coresight_enable/disable_link() to
> enable/disable functions of each link device.
>
> I also removed lock protection of csdev->enable in v3. Because that
> needs to move csdev->enable inside the enable/disable functions of
> each link device. That's much effort with almost no benefits.
> csdev->enable seems only used for source devices in sysfs interface.
>
> ---
> .../hwtracing/coresight/coresight-funnel.c | 29 ++++++++----
> .../coresight/coresight-replicator.c | 31 +++++++++----
> .../hwtracing/coresight/coresight-tmc-etf.c | 39 ++++++++--------
> drivers/hwtracing/coresight/coresight.c | 45 ++++++-------------
> 4 files changed, 77 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c
> index 05f7896c3a01..8326d03a0d03 100644
> --- a/drivers/hwtracing/coresight/coresight-funnel.c
> +++ b/drivers/hwtracing/coresight/coresight-funnel.c
> @@ -44,6 +44,7 @@ struct funnel_drvdata {
> struct clk *atclk;
> struct coresight_device *csdev;
> unsigned long priority;
> + spinlock_t spinlock;
> };
>
> static int dynamic_funnel_enable_hw(struct funnel_drvdata *drvdata, int port)
> @@ -76,12 +77,20 @@ static int funnel_enable(struct coresight_device *csdev, int inport,
> {
> int rc = 0;
> struct funnel_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> + unsigned long flags;
>
> - if (drvdata->base)
> - rc = dynamic_funnel_enable_hw(drvdata, inport);
> + spin_lock_irqsave(&drvdata->spinlock, flags);
> + if (atomic_inc_return(&csdev->refcnt[inport]) == 1) {
> + if (drvdata->base)
> + rc = dynamic_funnel_enable_hw(drvdata, inport);
>
> - if (!rc)
> - dev_dbg(&csdev->dev, "FUNNEL inport %d enabled\n", inport);
> + if (rc)
> + atomic_dec(&csdev->refcnt[inport]);
> + else
> + dev_dbg(&csdev->dev, "FUNNEL inport %d enabled\n",
> + inport);
> + }
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> return rc;
> }
>
> @@ -107,11 +116,15 @@ static void funnel_disable(struct coresight_device *csdev, int inport,
> int outport)
> {
> struct funnel_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> + unsigned long flags;
>
> - if (drvdata->base)
> - dynamic_funnel_disable_hw(drvdata, inport);
> -
> - dev_dbg(&csdev->dev, "FUNNEL inport %d disabled\n", inport);
> + spin_lock_irqsave(&drvdata->spinlock, flags);
> + if (atomic_dec_return(&csdev->refcnt[inport]) == 0) {
> + if (drvdata->base)
> + dynamic_funnel_disable_hw(drvdata, inport);
> + dev_dbg(&csdev->dev, "FUNNEL inport %d disabled\n", inport);
> + }
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> }
>
> static const struct coresight_ops_link funnel_link_ops = {
> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
> index b29ba640eb25..427d8b8d0917 100644
> --- a/drivers/hwtracing/coresight/coresight-replicator.c
> +++ b/drivers/hwtracing/coresight/coresight-replicator.c
> @@ -36,6 +36,7 @@ struct replicator_drvdata {
> void __iomem *base;
> struct clk *atclk;
> struct coresight_device *csdev;
> + spinlock_t spinlock;
> };
>
> static void dynamic_replicator_reset(struct replicator_drvdata *drvdata)
> @@ -97,11 +98,20 @@ static int replicator_enable(struct coresight_device *csdev, int inport,
> {
> int rc = 0;
> struct replicator_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> -
> - if (drvdata->base)
> - rc = dynamic_replicator_enable(drvdata, inport, outport);
> - if (!rc)
> - dev_dbg(&csdev->dev, "REPLICATOR enabled\n");
> + unsigned long flags;
> +
> + spin_lock_irqsave(&drvdata->spinlock, flags);
> + if (atomic_inc_return(&csdev->refcnt[outport]) == 1) {
Since we now have the spinlock to protect us, we could simply do an
atomic_read() and then do the hw_enable() followed by an atomic_inc(),
if we are successful. That way we could make it more cleaner and avoid
the atomic_dec() if we encounter a failure. In fact we could simply get
away with the refcnt and replace them with a simple integer, but that
may be a different patch.
> + if (drvdata->base)
> + rc = dynamic_replicator_enable(drvdata, inport,
> + outport);
> +
> + if (rc)
> + atomic_dec(&csdev->refcnt[outport]);
> + else
> + dev_dbg(&csdev->dev, "REPLICATOR enabled\n");
> + }
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> return rc;
> }
>
> @@ -137,10 +147,15 @@ static void replicator_disable(struct coresight_device *csdev, int inport,
> int outport)
> {
> struct replicator_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> + unsigned long flags;
>
> - if (drvdata->base)
> - dynamic_replicator_disable(drvdata, inport, outport);
> - dev_dbg(&csdev->dev, "REPLICATOR disabled\n");
> + spin_lock_irqsave(&drvdata->spinlock, flags);
> + if (atomic_dec_return(&csdev->refcnt[outport]) == 0) {
> + if (drvdata->base)
> + dynamic_replicator_disable(drvdata, inport, outport);
> + dev_dbg(&csdev->dev, "REPLICATOR disabled\n");
> + }
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> }
>
> static const struct coresight_ops_link replicator_link_ops = {
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index 807416b75ecc..cb4a38541bf8 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -334,23 +334,25 @@ static int tmc_disable_etf_sink(struct coresight_device *csdev)
> static int tmc_enable_etf_link(struct coresight_device *csdev,
> int inport, int outport)
> {
> - int ret;
> + int ret = 0;
> unsigned long flags;
> struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> spin_lock_irqsave(&drvdata->spinlock, flags);
> - if (drvdata->reading) {
> - spin_unlock_irqrestore(&drvdata->spinlock, flags);
> - return -EBUSY;
> + if (atomic_inc_return(&csdev->refcnt[0]) == 1) {
> + if (drvdata->reading)
> + ret = -EBUSY;
Could we not check the drvdata->reading before the refcount and bail
out early ? We are protected by the spinlock anyway. Similar to the
above case we could check the refcount and only increment when we have
enabled.
Cheers
Suzuki
Powered by blists - more mailing lists