coresight: etf/etb: NULL Pointer dereference crash fixes

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

Quoted 8 lines · Sai Prakash Ranjan

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.

Quoted 8 lines

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

Quoted 8 lines · Sai Prakash Ranjan

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.

Quoted 8 lines

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

Quoted 27 lines · Suzuki K Poulose

Quoted 8 lines · Sai Prakash Ranjan

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.

Quoted 8 lines

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);

Quoted 3 lines

if (!tsk || is_kernel_event(event))
   /* skip ? */

Looking at it some more, is_kernel_event() is not exposed outside events core and probably for good reason. Why do we need to check for this and not just tsk?

Quoted 28 lines · Sai Prakash Ranjan

Quoted 27 lines · Suzuki K Poulose

Quoted 8 lines · Sai Prakash Ranjan

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.

Quoted 8 lines

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);

Quoted 7 lines

Quoted 3 lines

if (!tsk || is_kernel_event(event))
   /* skip ? */

Looking at it some more, is_kernel_event() is not exposed outside events core and probably for good reason. Why do we need to check for this and not just tsk?

Because the event->owner could be :

= NULL
= -1UL // kernel event
= valid.

Quoted 37 lines · Suzuki K Poulose

Quoted 28 lines · Sai Prakash Ranjan

Quoted 27 lines · Suzuki K Poulose

Quoted 8 lines · Sai Prakash Ranjan

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.

Quoted 8 lines

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);

Quoted 7 lines

Quoted 3 lines

if (!tsk || is_kernel_event(event))
   /* skip ? */

Looking at it some more, is_kernel_event() is not exposed outside events core and probably for good reason. Why do we need to check for this and not just tsk?

Quoted 6 lines

Because the event->owner could be :

= NULL
= -1UL // kernel event
= valid.

Yes I understood that part, but here we were trying to fix the NULL pointer dereference right and hence the question as to why we need to check for kernel events? I am no expert in perf but I don't see anywhere in the kernel checking for is_kernel_event(), so I am a bit skeptical if exporting that is actually right or not.

Quoted 45 lines · Sai Prakash Ranjan

Quoted 37 lines · Suzuki K Poulose

Quoted 28 lines · Sai Prakash Ranjan

Quoted 27 lines · Suzuki K Poulose

Quoted 8 lines · Sai Prakash Ranjan

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.

Quoted 8 lines

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);

Quoted 7 lines

Quoted 3 lines

if (!tsk || is_kernel_event(event))
   /* skip ? */

Looking at it some more, is_kernel_event() is not exposed outside events core and probably for good reason. Why do we need to check for this and not just tsk?

Quoted 6 lines

Because the event->owner could be :

= NULL
= -1UL // kernel event
= valid.

Quoted 7 lines

Yes I understood that part, but here we were trying to fix the NULL pointer dereference right and hence the question as to why we need to check for kernel events? I am no expert in perf but I don't see anywhere in the kernel checking for is_kernel_event(), so I am a bit skeptical if exporting that is actually right or not.

I have stress tested with the original patch many times now, i.e., without a check for event->owner and is_kernel_event() and didn't observe any crash. Plus on ETR where this was already done, no crashes were reported till date and with ETF, the issue was quickly reproducible, so I am fairly confident that this doesn't just delay the original issue but actually fixes it. I will run an overnight test again to confirm this.

Quoted 54 lines · Sai Prakash Ranjan

Quoted 45 lines · Sai Prakash Ranjan

Quoted 37 lines · Suzuki K Poulose

Quoted 28 lines · Sai Prakash Ranjan

Quoted 27 lines · Suzuki K Poulose

Quoted 8 lines · Sai Prakash Ranjan

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.

Quoted 8 lines

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);

Quoted 7 lines

Quoted 3 lines

if (!tsk || is_kernel_event(event))
   /* skip ? */

Looking at it some more, is_kernel_event() is not exposed outside events core and probably for good reason. Why do we need to check for this and not just tsk?

Quoted 6 lines

Because the event->owner could be :

 = NULL
 = -1UL  // kernel event
 = valid.

Quoted 7 lines

Yes I understood that part, but here we were trying to fix the NULL pointer dereference right and hence the question as to why we need to check for kernel events? I am no expert in perf but I don't see anywhere in the kernel checking for is_kernel_event(), so I am a bit skeptical if exporting that is actually right or not.

Quoted 8 lines

I have stress tested with the original patch many times now, i.e., without a check for event->owner and is_kernel_event() and didn't observe any crash. Plus on ETR where this was already done, no crashes were reported till date and with ETF, the issue was quickly reproducible, so I am fairly confident that this doesn't just delay the original issue but actually fixes it. I will run an overnight test again to confirm this.

I ran the overnight test which collected aroung 4G data(see below), with the following small change to see if the two cases (event->owner=NULL and is_kernel_event()) are triggered with suggested changes and it didn't trigger at all.
Do we still need those additional checks?

[ perf record: Captured and wrote 4677.989 MB perf.data ]

drivers/hwtracing/coresight/coresight-tmc-etf.c +16 -0

index 989d965f3d90..123c446ce585 100644
@@ -13,6 +13,13 @@
  #include "coresight-tmc.h"
  #include "coresight-etm-perf.h"
 
+#define TASK_TOMBSTONE ((void *)-1L)
+
+static bool is_kernel_event2(struct perf_event *event)
+{
+       return READ_ONCE(event->owner) == TASK_TOMBSTONE;
+}
+
  static int tmc_set_etf_buffer(struct coresight_device *csdev,
                               struct perf_output_handle *handle);
 

@@ -392,6 +399,15 @@ 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) {
+               pr_info("**sai in task=NULL**\n");
+               return NULL;
+       }
+
+       if (is_kernel_event2(event))
+               pr_info("**sai in is_kernel_event**\n");
 
         node = (event->cpu == -1) ? NUMA_NO_NODE : 

cpu_to_node(event->cpu);

Thanks,
Sai

Quoted 64 lines · Sai Prakash Ranjan

Quoted 54 lines · Sai Prakash Ranjan

Quoted 45 lines · Sai Prakash Ranjan

Quoted 37 lines · Suzuki K Poulose

Quoted 28 lines · Sai Prakash Ranjan

Quoted 27 lines · Suzuki K Poulose

Quoted 8 lines · Sai Prakash Ranjan

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.

Quoted 8 lines

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);

Quoted 7 lines

Quoted 3 lines

if (!tsk || is_kernel_event(event))
   /* skip ? */

Looking at it some more, is_kernel_event() is not exposed outside events core and probably for good reason. Why do we need to check for this and not just tsk?

Quoted 6 lines

Because the event->owner could be :

 = NULL
 = -1UL  // kernel event
 = valid.

Quoted 7 lines

Yes I understood that part, but here we were trying to fix the NULL pointer dereference right and hence the question as to why we need to check for kernel events? I am no expert in perf but I don't see anywhere in the kernel checking for is_kernel_event(), so I am a bit skeptical if exporting that is actually right or not.

Quoted 8 lines

I have stress tested with the original patch many times now, i.e., without a check for event->owner and is_kernel_event() and didn't observe any crash. Plus on ETR where this was already done, no crashes were reported till date and with ETF, the issue was quickly reproducible, so I am fairly confident that this doesn't just delay the original issue but actually fixes it. I will run an overnight test again to confirm this.

Quoted 6 lines

I ran the overnight test which collected aroung 4G data(see below), with the following small change to see if the two cases (event->owner=NULL and is_kernel_event()) are triggered with suggested changes and it didn't trigger at all.
Do we still need those additional checks?

Yes. Please see perf_event_create_kernel_event(), which is an exported function allowing any kernel code (including modules) to use the PMU (just like the userspace perf tool would do). Just because your use case doesn't trigger this (because you don't run something that can trigger this) doesn't mean this can't be triggered.

Quoted 72 lines · Suzuki Poulose

Quoted 64 lines · Sai Prakash Ranjan

Quoted 54 lines · Sai Prakash Ranjan

Quoted 45 lines · Sai Prakash Ranjan

Quoted 37 lines · Suzuki K Poulose

Quoted 28 lines · Sai Prakash Ranjan

Quoted 27 lines · Suzuki K Poulose

Quoted 8 lines · Sai Prakash Ranjan

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.

Quoted 8 lines

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);

Quoted 7 lines

Quoted 3 lines

if (!tsk || is_kernel_event(event))
   /* skip ? */

Looking at it some more, is_kernel_event() is not exposed outside events core and probably for good reason. Why do we need to check for this and not just tsk?

Quoted 6 lines

Because the event->owner could be :

 = NULL
 = -1UL  // kernel event
 = valid.

Quoted 7 lines

Yes I understood that part, but here we were trying to fix the NULL pointer dereference right and hence the question as to why we need to check for kernel events? I am no expert in perf but I don't see anywhere in the kernel checking for is_kernel_event(), so I am a bit skeptical if exporting that is actually right or not.

Quoted 8 lines

I have stress tested with the original patch many times now, i.e., without a check for event->owner and is_kernel_event() and didn't observe any crash. Plus on ETR where this was already done, no crashes were reported till date and with ETF, the issue was quickly reproducible, so I am fairly confident that this doesn't just delay the original issue but actually fixes it. I will run an overnight test again to confirm this.

Quoted 6 lines

I ran the overnight test which collected aroung 4G data(see below), with the following small change to see if the two cases (event->owner=NULL and is_kernel_event()) are triggered with suggested changes and it didn't trigger at all.
Do we still need those additional checks?

Quoted 7 lines

Yes. Please see perf_event_create_kernel_event(), which is an exported function allowing any kernel code (including modules) to use the PMU (just like the userspace perf tool would do). Just because your use case doesn't trigger this (because you don't run something that can trigger this) doesn't mean this can't be triggered.

Thanks for that pointer, I will add them in the next version.

Quoted 81 lines · Sai Prakash Ranjan

Quoted 79 lines · Suzuki Poulose

Quoted 64 lines · Sai Prakash Ranjan

Quoted 54 lines · Sai Prakash Ranjan

Quoted 45 lines · Sai Prakash Ranjan

Quoted 37 lines · Suzuki K Poulose

Quoted 28 lines · Sai Prakash Ranjan

Quoted 27 lines · Suzuki K Poulose

Quoted 8 lines · Sai Prakash Ranjan

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.

Quoted 8 lines

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);

Quoted 7 lines

Quoted 3 lines

if (!tsk || is_kernel_event(event))
   /* skip ? */

Looking at it some more, is_kernel_event() is not exposed outside events core and probably for good reason. Why do we need to check for this and not just tsk?

Quoted 6 lines

Because the event->owner could be :

 = NULL
 = -1UL  // kernel event
 = valid.

Quoted 7 lines

Yes I understood that part, but here we were trying to fix the NULL pointer dereference right and hence the question as to why we need to check for kernel events? I am no expert in perf but I don't see anywhere in the kernel checking for is_kernel_event(), so I am a bit skeptical if exporting that is actually right or not.

Quoted 8 lines

I have stress tested with the original patch many times now, i.e., without a check for event->owner and is_kernel_event() and didn't observe any crash. Plus on ETR where this was already done, no crashes were reported till date and with ETF, the issue was quickly reproducible, so I am fairly confident that this doesn't just delay the original issue but actually fixes it. I will run an overnight test again to confirm this.

Quoted 6 lines

I ran the overnight test which collected aroung 4G data(see below), with the following small change to see if the two cases (event->owner=NULL and is_kernel_event()) are triggered with suggested changes and it didn't trigger at all.
Do we still need those additional checks?

Yes. Please see perf_event_create_kernel_event(), which is an exported function allowing any kernel code (including modules) to use the PMU (just like the userspace perf tool would do). Just because your use case doesn't trigger this (because you don't run something that can trigger this) doesn't mean this can't be triggered.

Quoted 2 lines

Thanks for that pointer, I will add them in the next version.

And instead of redefining TASK_TOMBSTONE in the driver, you may simply use IS_ERR_OR_NULL(tsk) to cover both NULL case and kernel event.

Quoted 85 lines · Suzuki Poulose

Quoted 83 lines · Sai Prakash Ranjan

Quoted 72 lines · Suzuki Poulose

Quoted 64 lines · Sai Prakash Ranjan

Quoted 54 lines · Sai Prakash Ranjan

Quoted 45 lines · Sai Prakash Ranjan

Quoted 37 lines · Suzuki K Poulose

Quoted 28 lines · Sai Prakash Ranjan

Quoted 27 lines · Suzuki K Poulose

Quoted 8 lines · Sai Prakash Ranjan

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.

Quoted 8 lines

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);

Quoted 7 lines

Quoted 3 lines

if (!tsk || is_kernel_event(event))
   /* skip ? */

Looking at it some more, is_kernel_event() is not exposed outside events core and probably for good reason. Why do we need to check for this and not just tsk?

Quoted 6 lines

Because the event->owner could be :

 = NULL
 = -1UL  // kernel event
 = valid.

Quoted 7 lines

Yes I understood that part, but here we were trying to fix the NULL pointer dereference right and hence the question as to why we need to check for kernel events? I am no expert in perf but I don't see anywhere in the kernel checking for is_kernel_event(), so I am a bit skeptical if exporting that is actually right or not.

Quoted 8 lines

I have stress tested with the original patch many times now, i.e., without a check for event->owner and is_kernel_event() and didn't observe any crash. Plus on ETR where this was already done, no crashes were reported till date and with ETF, the issue was quickly reproducible, so I am fairly confident that this doesn't just delay the original issue but actually fixes it. I will run an overnight test again to confirm this.

Quoted 6 lines

I ran the overnight test which collected aroung 4G data(see below), with the following small change to see if the two cases (event->owner=NULL and is_kernel_event()) are triggered with suggested changes and it didn't trigger at all.
Do we still need those additional checks?

Quoted 7 lines

Yes. Please see perf_event_create_kernel_event(), which is an exported function allowing any kernel code (including modules) to use the PMU (just like the userspace perf tool would do). Just because your use case doesn't trigger this (because you don't run something that can trigger this) doesn't mean this can't be triggered.

Thanks for that pointer, I will add them in the next version.

Quoted 4 lines

And instead of redefining TASK_TOMBSTONE in the driver, you may simply use IS_ERR_OR_NULL(tsk) to cover both NULL case and kernel event.

Ugh sorry, sent out v2 exporting is_kernel_event() before seeing this comment, I will resend.

Quoted 91 lines · Sai Prakash Ranjan

Quoted 89 lines · Suzuki Poulose

Quoted 81 lines · Sai Prakash Ranjan

Quoted 72 lines · Suzuki Poulose

Quoted 64 lines · Sai Prakash Ranjan

Quoted 54 lines · Sai Prakash Ranjan

Quoted 45 lines · Sai Prakash Ranjan

Quoted 37 lines · Suzuki K Poulose

Quoted 28 lines · Sai Prakash Ranjan

Quoted 27 lines · Suzuki K Poulose

Quoted 8 lines · Sai Prakash Ranjan

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.

Quoted 8 lines

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);

Quoted 7 lines

Quoted 3 lines

if (!tsk || is_kernel_event(event))
   /* skip ? */

Looking at it some more, is_kernel_event() is not exposed outside events core and probably for good reason. Why do we need to check for this and not just tsk?

Quoted 6 lines

Because the event->owner could be :

 = NULL
 = -1UL  // kernel event
 = valid.

Quoted 7 lines

Yes I understood that part, but here we were trying to fix the NULL pointer dereference right and hence the question as to why we need to check for kernel events? I am no expert in perf but I don't see anywhere in the kernel checking for is_kernel_event(), so I am a bit skeptical if exporting that is actually right or not.

Quoted 8 lines

I have stress tested with the original patch many times now, i.e., without a check for event->owner and is_kernel_event() and didn't observe any crash. Plus on ETR where this was already done, no crashes were reported till date and with ETF, the issue was quickly reproducible, so I am fairly confident that this doesn't just delay the original issue but actually fixes it. I will run an overnight test again to confirm this.

Quoted 6 lines

I ran the overnight test which collected aroung 4G data(see below), with the following small change to see if the two cases (event->owner=NULL and is_kernel_event()) are triggered with suggested changes and it didn't trigger at all.
Do we still need those additional checks?

Quoted 7 lines

Yes. Please see perf_event_create_kernel_event(), which is an exported function allowing any kernel code (including modules) to use the PMU (just like the userspace perf tool would do). Just because your use case doesn't trigger this (because you don't run something that can trigger this) doesn't mean this can't be triggered.

Quoted 2 lines

Thanks for that pointer, I will add them in the next version.

And instead of redefining TASK_TOMBSTONE in the driver, you may simply use IS_ERR_OR_NULL(tsk) to cover both NULL case and kernel event.

Quoted 2 lines

Ugh sorry, sent out v2 exporting is_kernel_event() before seeing this comment, I will resend.

Saw that. I would say, wait until someone complains about that. If people are Ok with exporting it, it is fine. I guess it will be useful. You could fall back to this approach if there is resistance.