| 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: <ec11e7f4-ed82-44eb-8e72-5a5094b67cda@linaro.org>
Date: Wed, 27 Nov 2024 16:30:22 +0000
From: James Clark <james.clark@...aro.org>
To: Yeoreum Yun <yeoreum.yun@....com>, suzuki.poulose@....com,
mike.leach@...aro.org
Cc: coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-rt-devel@...ts.linux.dev, nd@....com,
alexander.shishkin@...ux.intel.com, bigeasy@...utronix.de,
clrkwllms@...nel.org, rostedt@...dmis.org
Subject: Re: [PATCH 2/9] coresight-etm4x: change etmv4_drvdata spinlock type
to raw_spinlock_t
On 25/11/2024 9:48 am, Yeoreum Yun wrote:
> From: Levi Yun <yeoreum.yun@....com>
>
> In coresight-etm4x drivers, etmv4_drvdata->spinlock can be held during
> __schedule() by perf_event_task_sched_out()/in().
>
> Since etmv4_drvdata->spinlock type is spinlock_t and
> perf_event_task_sched_out()/in() is called after acquiring rq_lock,
> which is raw_spinlock_t (an unsleepable lock),
> this poses an issue in PREEMPT_RT kernel where spinlock_t is sleepable.
>
> To address this, change type etmv4_drvdata->spinlock
> in coresight-etm4x drivers, which can be called
> by perf_event_task_sched_out()/in(), from spinlock_t to raw_spinlock_t.
>
> Signed-off-by: Yeoreum Yun <yeoreum.yun@....com>
> ---
[...]
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> index a9f19629f3f8..2e6b79c37f87 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> @@ -174,99 +174,99 @@ static ssize_t reset_store(struct device *dev,
> if (kstrtoul(buf, 16, &val))
> return -EINVAL;
>
> - spin_lock(&drvdata->spinlock);
> - if (val)
> - config->mode = 0x0;
> + scoped_guard(raw_spinlock, &drvdata->spinlock) {
> + if (val)
> + config->mode = 0x0;
>
> - /* Disable data tracing: do not trace load and store data transfers */
> - config->mode &= ~(ETM_MODE_LOAD | ETM_MODE_STORE);
> - config->cfg &= ~(TRCCONFIGR_INSTP0_LOAD | TRCCONFIGR_INSTP0_STORE);
> + /* Disable data tracing: do not trace load and store data transfers */
> + config->mode &= ~(ETM_MODE_LOAD | ETM_MODE_STORE);
> + config->cfg &= ~(TRCCONFIGR_INSTP0_LOAD | TRCCONFIGR_INSTP0_STORE);
>
> - /* Disable data value and data address tracing */
> - config->mode &= ~(ETM_MODE_DATA_TRACE_ADDR |
> - ETM_MODE_DATA_TRACE_VAL);
> - config->cfg &= ~(TRCCONFIGR_DA | TRCCONFIGR_DV);
> + /* Disable data value and data address tracing */
> + config->mode &= ~(ETM_MODE_DATA_TRACE_ADDR |
> + ETM_MODE_DATA_TRACE_VAL);
> + config->cfg &= ~(TRCCONFIGR_DA | TRCCONFIGR_DV);
>
> - /* Disable all events tracing */
> - config->eventctrl0 = 0x0;
> - config->eventctrl1 = 0x0;
> + /* Disable all events tracing */
> + config->eventctrl0 = 0x0;
> + config->eventctrl1 = 0x0;
>
> - /* Disable timestamp event */
> - config->ts_ctrl = 0x0;
> + /* Disable timestamp event */
> + config->ts_ctrl = 0x0;
>
> - /* Disable stalling */
> - config->stall_ctrl = 0x0;
> + /* Disable stalling */
> + config->stall_ctrl = 0x0;
>
> - /* Reset trace synchronization period to 2^8 = 256 bytes*/
> - if (drvdata->syncpr == false)
> - config->syncfreq = 0x8;
> + /* Reset trace synchronization period to 2^8 = 256 bytes*/
> + if (drvdata->syncpr == false)
> + config->syncfreq = 0x8;
>
> - /*
> - * Enable ViewInst to trace everything with start-stop logic in
> - * started state. ARM recommends start-stop logic is set before
> - * each trace run.
> - */
> - config->vinst_ctrl = FIELD_PREP(TRCVICTLR_EVENT_MASK, 0x01);
> - if (drvdata->nr_addr_cmp > 0) {
> - config->mode |= ETM_MODE_VIEWINST_STARTSTOP;
> - /* SSSTATUS, bit[9] */
> - config->vinst_ctrl |= TRCVICTLR_SSSTATUS;
> - }
> + /*
> + * Enable ViewInst to trace everything with start-stop logic in
> + * started state. ARM recommends start-stop logic is set before
> + * each trace run.
> + */
> + config->vinst_ctrl = FIELD_PREP(TRCVICTLR_EVENT_MASK, 0x01);
> + if (drvdata->nr_addr_cmp > 0) {
> + config->mode |= ETM_MODE_VIEWINST_STARTSTOP;
> + /* SSSTATUS, bit[9] */
> + config->vinst_ctrl |= TRCVICTLR_SSSTATUS;
> + }
>
> - /* No address range filtering for ViewInst */
> - config->viiectlr = 0x0;
> + /* No address range filtering for ViewInst */
> + config->viiectlr = 0x0;
>
> - /* No start-stop filtering for ViewInst */
> - config->vissctlr = 0x0;
> - config->vipcssctlr = 0x0;
> + /* No start-stop filtering for ViewInst */
> + config->vissctlr = 0x0;
> + config->vipcssctlr = 0x0;
>
> - /* Disable seq events */
> - for (i = 0; i < drvdata->nrseqstate-1; i++)
> - config->seq_ctrl[i] = 0x0;
> - config->seq_rst = 0x0;
> - config->seq_state = 0x0;
> + /* Disable seq events */
> + for (i = 0; i < drvdata->nrseqstate-1; i++)
> + config->seq_ctrl[i] = 0x0;
> + config->seq_rst = 0x0;
> + config->seq_state = 0x0;
>
> - /* Disable external input events */
> - config->ext_inp = 0x0;
> + /* Disable external input events */
> + config->ext_inp = 0x0;
>
> - config->cntr_idx = 0x0;
> - for (i = 0; i < drvdata->nr_cntr; i++) {
> - config->cntrldvr[i] = 0x0;
> - config->cntr_ctrl[i] = 0x0;
> - config->cntr_val[i] = 0x0;
> - }
> + config->cntr_idx = 0x0;
> + for (i = 0; i < drvdata->nr_cntr; i++) {
> + config->cntrldvr[i] = 0x0;
> + config->cntr_ctrl[i] = 0x0;
> + config->cntr_val[i] = 0x0;
> + }
>
> - config->res_idx = 0x0;
> - for (i = 2; i < 2 * drvdata->nr_resource; i++)
> - config->res_ctrl[i] = 0x0;
> + config->res_idx = 0x0;
> + for (i = 2; i < 2 * drvdata->nr_resource; i++)
> + config->res_ctrl[i] = 0x0;
>
> - config->ss_idx = 0x0;
> - for (i = 0; i < drvdata->nr_ss_cmp; i++) {
> - config->ss_ctrl[i] = 0x0;
> - config->ss_pe_cmp[i] = 0x0;
> - }
> + config->ss_idx = 0x0;
> + for (i = 0; i < drvdata->nr_ss_cmp; i++) {
> + config->ss_ctrl[i] = 0x0;
> + config->ss_pe_cmp[i] = 0x0;
> + }
>
> - config->addr_idx = 0x0;
> - for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
> - config->addr_val[i] = 0x0;
> - config->addr_acc[i] = 0x0;
> - config->addr_type[i] = ETM_ADDR_TYPE_NONE;
> - }
> + config->addr_idx = 0x0;
> + for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
> + config->addr_val[i] = 0x0;
> + config->addr_acc[i] = 0x0;
> + config->addr_type[i] = ETM_ADDR_TYPE_NONE;
> + }
>
> - config->ctxid_idx = 0x0;
> - for (i = 0; i < drvdata->numcidc; i++)
> - config->ctxid_pid[i] = 0x0;
> + config->ctxid_idx = 0x0;
> + for (i = 0; i < drvdata->numcidc; i++)
> + config->ctxid_pid[i] = 0x0;
>
> - config->ctxid_mask0 = 0x0;
> - config->ctxid_mask1 = 0x0;
> + config->ctxid_mask0 = 0x0;
> + config->ctxid_mask1 = 0x0;
>
> - config->vmid_idx = 0x0;
> - for (i = 0; i < drvdata->numvmidc; i++)
> - config->vmid_val[i] = 0x0;
> - config->vmid_mask0 = 0x0;
> - config->vmid_mask1 = 0x0;
> + config->vmid_idx = 0x0;
> + for (i = 0; i < drvdata->numvmidc; i++)
> + config->vmid_val[i] = 0x0;
>
> - spin_unlock(&drvdata->spinlock);
> + config->vmid_mask0 = 0x0;
> + config->vmid_mask1 = 0x0;
> + }
There's a lot of churn in this function just to use the new scoped
guard, but there was only one lock and unlock anyway. There are a few
other cases of this in the whole set.
The scoped guards make it easier to write correct code, but I'm not sure
we gain anything here other than a bigger diff and more to review by
changing already working code. Having said that I did review all the
changes and I'm pretty sure they're all ok, so I'm on the fence about
whether it's worth going back and undoing all of them.
Surely updating to raw spinlocks is a search and replace to fix a
specific problem, and the scoped guard stuff can be done on a case by
case basis when anything is re-written in the future?
I don't know if we're considering a fixes tag, if PREEMPT_RT is 6.12?
It's probably not necessary but if so we definitely want to minimise the
change.
Powered by blists - more mailing lists