Hi Suzuki,
On 2020-10-13 22:05, Suzuki K Poulose wrote:
On 10/07/2020 02:00 PM, Sai Prakash Ranjan wrote:
There was a report of NULL pointer dereference in ETF enable
path for perf CS mode with PID monitoring. It is almost 100%
reproducible when the process to monitor is something very
active such as chrome and with ETF as the sink and not ETR.
Currently in a bid to find the pid, the owner is dereferenced
via task_pid_nr() call in tmc_enable_etf_sink_perf() and with
owner being NULL, we get a NULL pointer dereference.
Looking at the ETR and other places in the kernel, ETF and the
ETB are the only places trying to dereference the task(owner)
in tmc_enable_etf_sink_perf() which is also called from the
sched_in path as in the call trace. Owner(task) is NULL even
in the case of ETR in tmc_enable_etr_sink_perf(), but since we
cache the PID in alloc_buffer() callback and it is done as part
of etm_setup_aux() when allocating buffer for ETR sink, we never
dereference this NULL pointer and we are safe. So lets do the
The patch is necessary to fix some of the issues. But I feel it is
not complete. Why is it safe earlier and not later ? I believe we are
simply reducing the chances of hitting the issue, by doing this earlier
than
later.
I did stress it for a long time with this patch and did not face
any issues but I guess it doesn't hurt to have the check as you
suggested.
I would say we better fix all instances to make sure that the
event->owner is valid. (e.g, I can see that the for kernel events
event->owner == -1 ?)
struct task_struct *tsk = READ_ONCE(event->owner);
if (!tsk || is_kernel_event(event))
/* skip ? */
So to confirm my understanding, I will add the above checks on top
of this patch for ETR, ETB and ETF something like below?
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c
b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 989d965f3d90..86ff0dda0444 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -392,6 +392,10 @@ static void *tmc_alloc_etf_buffer(struct
coresight_device *csdev,
{
int node;
struct cs_buffers *buf;
+ struct task_struct *task = READ_ONCE(event->owner);
+
+ if (!task || is_kernel_event(event))
+ return NULL;
node = (event->cpu == -1) ? NUMA_NO_NODE :
cpu_to_node(event->cpu);
@@ -400,7 +404,7 @@ static void *tmc_alloc_etf_buffer(struct
coresight_device *csdev,
if (!buf)
return NULL;
- buf->pid = task_pid_nr(event->owner);
+ buf->pid = task_pid_nr(task);
buf->snapshot = overwrite;
buf->nr_pages = nr_pages;
buf->data_pages = pages;
Thanks,
Sai
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member
of Code Aurora Forum, hosted by The Linux Foundation
