Fix race in trace point in zrl_add_impl

We hit an illegal memory access in the zrlock trace point. The problem
is that zrl->zr_owner and zrl->zr_caller are assigned locklessly. And if
zrl->zr_owner got assigned a longer string between when __string()
calculate the strlen, and when __assign_str() does strcpy. The copy will
overflow the buffer.

==
For example:

Initial condition:
zrl->zr_owner = A
zrl->zr_caller = "abc"

Thread A                                 Thread B
-------------------------------------------------
if (zrl->zr_owner == A) {
  DTRACE_PROBE2() {
    __string() {
      strlen(zrl->zr_caller) -> 3
      allocate buf[4]
    }

                                        zrl->zr_owner = B
				        zrl->zr_caller = "abcd"

    __assign_str() {
      strcpy(buf, zrl->zr_caller) <- buffer overflow
==

Dereferencing zrl->zr_owner->pid may also be problematic, in that the
zrl->zr_owner got changed to other task, and that task exits, freeing
the task_struct. This should be very unlikely, as the other task need to
zrl_remove and exit between the dereferencing zr->zr_owner and
zr->zr_owner->pid. Nevertheless, we'll deal with it as well.

To fix the zrl->zr_caller issue, instead of copy the string content, we
just copy the pointer, this is safe because it always points to
__func__, which is static. As for the zrl->zr_owner issue, we pass in
curthread instead of using zrl->zr_owner.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Chunwei Chen <david.chen@nutanix.com>
Closes #7291

diff --git a/include/sys/trace_zrlock.h b/include/sys/trace_zrlock.h
index eacba759d32ea9ab20c4d825266b4a059818295a..fa330f2c19aa02b8f15c92072854065e7a0472a3 100644 (file)
--- a/include/sys/trace_zrlock.h
+++ b/include/sys/trace_zrlock.h
@@ -42,27 +42,27 @@
  */
 /* BEGIN CSTYLED */
 DECLARE_EVENT_CLASS(zfs_zrlock_class,
-       TP_PROTO(zrlock_t *zrl, uint32_t n),
-       TP_ARGS(zrl, n),
+       TP_PROTO(zrlock_t *zrl, kthread_t *owner, uint32_t n),
+       TP_ARGS(zrl, owner, n),
        TP_STRUCT__entry(
            __field(int32_t,            refcount)
 #ifdef ZFS_DEBUG
            __field(pid_t,              owner_pid)
-           __string(caller, zrl->zr_caller)
+           __field(const char *,       caller)
 #endif
            __field(uint32_t,           n)
        ),
        TP_fast_assign(
            __entry->refcount   = zrl->zr_refcount;
 #ifdef ZFS_DEBUG
-           __entry->owner_pid  = zrl->zr_owner ? zrl->zr_owner->pid : 0;
-           __assign_str(caller, zrl->zr_caller);
+           __entry->owner_pid  = owner ? owner->pid : 0;
+           __entry->caller = zrl->zr_caller ? zrl->zr_caller : "(null)";
 #endif
            __entry->n          = n;
        ),
 #ifdef ZFS_DEBUG
        TP_printk("zrl { refcount %d owner_pid %d caller %s } n %u",
-           __entry->refcount, __entry->owner_pid, __get_str(caller),
+           __entry->refcount, __entry->owner_pid, __entry->caller,
            __entry->n)
 #else
        TP_printk("zrl { refcount %d } n %u",
@@ -73,8 +73,8 @@ DECLARE_EVENT_CLASS(zfs_zrlock_class,
 
 #define        DEFINE_ZRLOCK_EVENT(name) \
 DEFINE_EVENT(zfs_zrlock_class, name, \
-       TP_PROTO(zrlock_t *zrl, uint32_t n), \
-       TP_ARGS(zrl, n))
+       TP_PROTO(zrlock_t *zrl, kthread_t *owner, uint32_t n), \
+       TP_ARGS(zrl, owner, n))
 DEFINE_ZRLOCK_EVENT(zfs_zrlock__reentry);
 
 #endif /* _TRACE_ZRLOCK_H */

diff --git a/module/zfs/zrlock.c b/module/zfs/zrlock.c
index 167d4c5061f43bcf5db71da96a74d5c6841d5c17..4f4854436e4ff2f2e8e158a1352f4cc1f7c6895b 100644 (file)
--- a/module/zfs/zrlock.c
+++ b/module/zfs/zrlock.c
@@ -82,8 +82,10 @@ zrl_add_impl(zrlock_t *zrl, const char *zc)
                                ASSERT3S((int32_t)n, >=, 0);
 #ifdef ZFS_DEBUG
                                if (zrl->zr_owner == curthread) {
-                                       DTRACE_PROBE2(zrlock__reentry,
-                                           zrlock_t *, zrl, uint32_t, n);
+                                       DTRACE_PROBE3(zrlock__reentry,
+                                           zrlock_t *, zrl,
+                                           kthread_t *, curthread,
+                                           uint32_t, n);
                                }
                                zrl->zr_owner = curthread;
                                zrl->zr_caller = zc;