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 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 ? */ Suzuki
