etm_setup_aux() fetches the per-CPU source pointer while preparing perf AUX trace paths. This can race with CoreSight device unregistration, the ETM device may has been released while the AUX setup still use it, leading to use-after-free.
Move per-CPU path construction into etm_event_build_path() and use the coresight_{get|put}_percpu_source_ref() pairs to take and drop the device references, this ensures the device is safe to access during path construction.
Update comments accordingly. Document a PREEMPT_RT corner case: the put_device() may release resources while coresight_dev_lock (a raw spinlock) is held, and the release may attempt to acquire a spinlock that becomes sleepable under PREEMPT_RT. This will be fixed in the future.
Reviewed-by: Yeoreum Yun yeoreum.yun@arm.com Reviewed-by: James Clark james.clark@linaro.org Tested-by: James Clark james.clark@linaro.org Tested-by: Jie Gan jie.gan@oss.qualcomm.com Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-core.c | 38 +++++- drivers/hwtracing/coresight/coresight-etm-perf.c | 160 +++++++++++++---------- drivers/hwtracing/coresight/coresight-priv.h | 3 +- 3 files changed, 130 insertions(+), 71 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index 995a4a1bab3d983141237f453b6f630e73e69e5e..fc4855c3dfaafe04b3bdc2dd607128353f38e984 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -109,13 +109,47 @@ static void coresight_clear_percpu_source(struct coresight_device *csdev) per_cpu(csdev_source, csdev->cpu) = NULL; }
-struct coresight_device *coresight_get_percpu_source(int cpu) +struct coresight_device *coresight_get_percpu_source_ref(int cpu) { + struct coresight_device *csdev; + if (WARN_ON(cpu < 0)) return NULL;
guard(raw_spinlock_irqsave)(&coresight_dev_lock); - return per_cpu(csdev_source, cpu); + + csdev = per_cpu(csdev_source, cpu); + if (!csdev) + return NULL; + + /* + * Holding a reference to the csdev->dev ensures that the + * coresight_device is live for the caller. The path building + * logic can safely either build a path to the sink or fail + * if the device is being unregistered (if there was a race). + * The caller can skip the "source" device, if no path could + * be built. + */ + get_device(&csdev->dev); + + return csdev; +} + +void coresight_put_percpu_source_ref(struct coresight_device *csdev) +{ + if (!csdev || !coresight_is_percpu_source(csdev)) + return; + + guard(raw_spinlock_irqsave)(&coresight_dev_lock); + + /* + * TODO: coresight_device_release() is invoked to release resources when + * the device's refcount reaches zero. It then calls free_percpu(), + * which acquires pcpu_lock — a sleepable lock when PREEMPT_RT is + * enabled. Since the raw spinlock coresight_dev_lock is held, this can + * lead to a potential "scheduling while atomic" issue. + */ + put_device(&csdev->dev); }
struct coresight_device *coresight_get_source(struct coresight_path *path) diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index b9e556818c3c6873ed53e5a1b8052443dd7740d0..89b99c3caedbc34da57f406797701425d36a1333 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -343,6 +343,86 @@ static struct coresight_path *etm_event_get_ctxt_path(struct etm_ctxt *ctxt) return path; }
+static struct coresight_path * +etm_event_build_path(struct perf_event *event, int cpu, + struct coresight_device *user_sink, + struct coresight_device *match_sink) +{ + struct coresight_path *path = NULL; + struct coresight_device *source, *sink; + int ret; + + source = coresight_get_percpu_source_ref(cpu); + + /* + * If there is no ETM associated with this CPU or ever we try to trace + * on this CPU, we handle it accordingly. + */ + if (!source) + return NULL; + + /* + * If AUX pause feature is enabled but the ETM driver does not + * support the operations, skip for this source. + */ + if (event->attr.aux_start_paused && + (!source_ops(source)->pause_perf || + !source_ops(source)->resume_perf)) { + dev_err_once(&source->dev, "AUX pause is not supported.\n"); + goto out; + } + + /* If sink has been specified by user, directly use it */ + if (user_sink) { + sink = user_sink; + } else { + /* + * No sink provided - look for a default sink for all the ETMs, + * where this event can be scheduled. + * + * We allocate the sink specific buffers only once for this + * event. If the ETMs have different default sink devices, we + * can only use a single "type" of sink as the event can carry + * only one sink specific buffer. Thus we have to make sure + * that the sinks are of the same type and driven by the same + * driver, as the one we allocate the buffer for. We don't + * trace on a CPU if the sink is not compatible. + */ + + /* Find the default sink for this ETM */ + sink = coresight_find_default_sink(source); + if (!sink) + goto out; + + /* Check if this sink compatible with the last sink */ + if (match_sink && !sinks_compatible(match_sink, sink)) + goto out; + } + + /* + * Building a path doesn't enable it, it simply builds a + * list of devices from source to sink that can be + * referenced later when the path is actually needed. + */ + path = coresight_build_path(source, sink); + if (IS_ERR(path)) + goto out; + + /* ensure we can allocate a trace ID for this CPU */ + ret = coresight_path_assign_trace_id(path, CS_MODE_PERF); + if (ret) { + coresight_release_path(path); + path = NULL; + goto out; + } + + coresight_trace_id_perf_start(&sink->perf_sink_id_map); + +out: + coresight_put_percpu_source_ref(source); + return IS_ERR_OR_NULL(path) ? NULL : path; +} + static void *etm_setup_aux(struct perf_event *event, void **pages, int nr_pages, bool overwrite) { @@ -350,9 +430,8 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, int cpu = event->cpu; cpumask_t *mask; struct coresight_device *sink = NULL; - struct coresight_device *user_sink = NULL, *last_sink = NULL; + struct coresight_device *user_sink = NULL; struct etm_event_data *event_data = NULL; - int ret;
event_data = alloc_event_data(cpu); if (!event_data) @@ -383,80 +462,25 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, */ for_each_cpu(cpu, mask) { struct coresight_path *path; - struct coresight_device *csdev;
- csdev = coresight_get_percpu_source(cpu); - /* - * If there is no ETM associated with this CPU clear it from - * the mask and continue with the rest. If ever we try to trace - * on this CPU, we handle it accordingly. - */ - if (!csdev) { + path = etm_event_build_path(event, cpu, user_sink, sink); + if (!path) { + /* + * Failed to create a path for the CPU, clear it from + * the mask and continue to next one. + */ cpumask_clear_cpu(cpu, mask); continue; }
/* - * If AUX pause feature is enabled but the ETM driver does not - * support the operations, clear this CPU from the mask and - * continue to next one. + * The first found sink is saved here and passed to + * etm_event_build_path() to check whether the remaining ETMs + * have a compatible default sink. */ - if (event->attr.aux_start_paused && - (!source_ops(csdev)->pause_perf || !source_ops(csdev)->resume_perf)) { - dev_err_once(&csdev->dev, "AUX pause is not supported.\n"); - cpumask_clear_cpu(cpu, mask); - continue; - } - - /* - * No sink provided - look for a default sink for all the ETMs, - * where this event can be scheduled. - * We allocate the sink specific buffers only once for this - * event. If the ETMs have different default sink devices, we - * can only use a single "type" of sink as the event can carry - * only one sink specific buffer. Thus we have to make sure - * that the sinks are of the same type and driven by the same - * driver, as the one we allocate the buffer for. As such - * we choose the first sink and check if the remaining ETMs - * have a compatible default sink. We don't trace on a CPU - * if the sink is not compatible. - */ - if (!user_sink) { - /* Find the default sink for this ETM */ - sink = coresight_find_default_sink(csdev); - if (!sink) { - cpumask_clear_cpu(cpu, mask); - continue; - } - - /* Check if this sink compatible with the last sink */ - if (last_sink && !sinks_compatible(last_sink, sink)) { - cpumask_clear_cpu(cpu, mask); - continue; - } - last_sink = sink; - } - - /* - * Building a path doesn't enable it, it simply builds a - * list of devices from source to sink that can be - * referenced later when the path is actually needed. - */ - path = coresight_build_path(csdev, sink); - if (IS_ERR(path)) { - cpumask_clear_cpu(cpu, mask); - continue; - } - - /* ensure we can allocate a trace ID for this CPU */ - ret = coresight_path_assign_trace_id(path, CS_MODE_PERF); - if (ret) { - cpumask_clear_cpu(cpu, mask); - coresight_release_path(path); - continue; - } + if (!user_sink && !sink) + sink = coresight_get_sink(path);
- coresight_trace_id_perf_start(&sink->perf_sink_id_map); *etm_event_cpu_path_ptr(event_data, cpu) = path; }
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index 4d6f5bc1df9cc5dfd974a8b27820328e7916f169..808d1546f568278d62fe72d871b6af82eb830074 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -249,7 +249,8 @@ void coresight_add_helper(struct coresight_device *csdev, void coresight_set_percpu_sink(int cpu, struct coresight_device *csdev); struct coresight_device *coresight_get_percpu_sink(int cpu); struct coresight_device *coresight_get_source(struct coresight_path *path); -struct coresight_device *coresight_get_percpu_source(int cpu); +struct coresight_device *coresight_get_percpu_source_ref(int cpu); +void coresight_put_percpu_source_ref(struct coresight_device *csdev); void coresight_disable_source(struct coresight_device *csdev, void *data); void coresight_pause_source(struct coresight_device *csdev); int coresight_resume_source(struct coresight_device *csdev);