strace: Block the target when the strace q is full
authorBarret Rhoden <brho@cs.berkeley.edu>
Sun, 29 Jan 2017 22:42:54 +0000 (17:42 -0500)
committerBarret Rhoden <brho@cs.berkeley.edu>
Thu, 9 Feb 2017 17:31:09 +0000 (12:31 -0500)
Unless drop_overflow is set, the *target* will block instead of dropping
a systrace record, for most syscalls.

Syscalls that cannot block are blacklisted.  The main reason for not
blocking is due to accessing per-core state of the calling core.  Once
we block, we could migrate or that state could otherwise be invalid.

This strace option triggered a bunch of bugs, so be careful.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/drivers/dev/proc.c
kern/include/syscall.h
kern/src/syscall.c

index 64ac2ab..77cd6eb 100644 (file)
@@ -94,6 +94,7 @@ enum {
        CMvmkill,
        CMstraceme,
        CMstraceall,
+       CMstrace_drop,
 };
 
 enum {
@@ -161,6 +162,7 @@ struct cmdtab proccmd[] = {
        {CMvmkill, "vmkill", 0},
        {CMstraceme, "straceme", 0},
        {CMstraceall, "straceall", 0},
+       {CMstrace_drop, "strace_drop", 2},
 };
 
 /*
@@ -628,6 +630,10 @@ static struct chan *procopen(struct chan *c, int omode)
                                spin_unlock(&p->strace->lock);
                                error(EBUSY, "Process is already being traced");
                        }
+                       /* It's not critical that we reopen before setting tracing, but it's
+                        * a little cleaner (concurrent syscalls could be trying to use the
+                        * queue before it was reopened, and they'd throw). */
+                       qreopen(p->strace->q);
                        p->strace->tracing = TRUE;
                        spin_unlock(&p->strace->lock);
                        /* the ref we are upping is the one we put in __proc_free, which is
@@ -853,6 +859,7 @@ static void procclose(struct chan *c)
 
                assert(c->flag & COPEN);        /* only way aux should have been set */
                s->tracing = FALSE;
+               qhangup(s->q, NULL);
                kref_put(&s->users);
                c->aux = NULL;
        }
@@ -1376,12 +1383,16 @@ static void procctlreq(struct proc *p, char *va, int n)
        switch (ct->index) {
        case CMstraceall:
        case CMstraceme:
+       case CMstrace_drop:
                /* common allocation.  if we inherited, we might have one already */
                if (!p->strace) {
                        strace = kzmalloc(sizeof(*p->strace), MEM_WAIT);
                        spinlock_init(&strace->lock);
                        bitmap_set(strace->trace_set, 0, MAX_SYSCALL_NR);
-                       strace->q = qopen(65536, Qdropoverflow|Qcoalesce, NULL, NULL);
+                       strace->q = qopen(65536, 0, NULL, NULL);
+                       /* The queue is reopened and hungup whenever we open the Qstrace
+                        * file.  This hangup might not be necessary, but is safer. */
+                       qhangup(strace->q, NULL);
                        /* both of these refs are put when the proc is freed.  procs is for
                         * every process that has this p->strace.  users is procs + every
                         * user (e.g. from open()).
@@ -1442,6 +1453,14 @@ static void procctlreq(struct proc *p, char *va, int n)
        case CMstraceall:
                p->strace->inherit = TRUE;
                break;
+       case CMstrace_drop:
+               if (!strcmp(cb->f[1], "on"))
+                       p->strace->drop_overflow = TRUE;
+               else if (!strcmp(cb->f[1], "off"))
+                       p->strace->drop_overflow = FALSE;
+               else
+                       error(EINVAL, "strace_drop takes on|off %s", cb->f[1]);
+               break;
        }
        poperror();
        kfree(cb);
index 16e2950..cdff324 100644 (file)
@@ -46,6 +46,7 @@ struct systrace_record {
 struct strace {
        bool tracing;
        bool inherit;
+       bool drop_overflow;
        atomic_t nr_drops;
        unsigned long appx_nr_sysc;
        struct kref procs; /* when procs goes to zero, q is hung up. */
index 248908f..9f068d7 100644 (file)
@@ -104,17 +104,58 @@ static size_t systrace_fill_pretty_buf(struct systrace_record *trace,
        return len;
 }
 
+/* If some syscalls block, then they can really hurt the user and the
+ * kernel.  For instance, if you blocked another call because the trace queue is
+ * full, the 2LS will want to yield the vcore, but then *that* call would block
+ * too.  Since that caller was in vcore context, the core will just spin
+ * forever.
+ *
+ * Even worse, some syscalls operate on the calling core or current context,
+ * thus accessing pcpui.  If we block, then that old context is gone.  Worse, we
+ * could migrate and then be operating on a different core.  Imagine
+ * SYS_halt_core.  Doh! */
+static bool sysc_can_block(unsigned int sysc_num)
+{
+       switch (sysc_num) {
+       case SYS_proc_yield:
+       case SYS_fork:
+       case SYS_exec:
+       case SYS_pop_ctx:
+       case SYS_getvcoreid:
+       case SYS_halt_core:
+       case SYS_vc_entry:
+       case SYS_change_vcore:
+       case SYS_change_to_m:
+               return FALSE;
+       }
+       return TRUE;
+}
+
 /* Helper: spits out our trace to the various sinks. */
 static void systrace_output(struct systrace_record *trace,
                             struct strace *strace, bool entry)
 {
+       ERRSTACK(1);
        size_t pretty_len;
 
+       /* qio ops can throw, especially the blocking qwrite.  I had it block on the
+        * outbound path of sys_proc_destroy().  The rendez immediately throws. */
+       if (waserror()) {
+               poperror();
+               return;
+       }
        pretty_len = systrace_fill_pretty_buf(trace, entry);
-       if (strace)
-               qiwrite(strace->q, trace->pretty_buf, pretty_len);
+       if (strace) {
+               /* At this point, we're going to emit the exit trace.  It's just a
+                * question of whether or not we block while doing it. */
+               if (strace->drop_overflow || !sysc_can_block(trace->syscallno))
+                       qiwrite(strace->q, trace->pretty_buf, pretty_len);
+               else
+                       qwrite(strace->q, trace->pretty_buf, pretty_len);
+       }
        if (systrace_loud)
                printk("%s", trace->pretty_buf);
+       poperror();
 }
 
 static bool should_strace(struct proc *p, struct syscall *sysc)
@@ -127,9 +168,14 @@ static bool should_strace(struct proc *p, struct syscall *sysc)
                return FALSE;
        /* TOCTTOU concerns - sysc is __user. */
        sysc_num = ACCESS_ONCE(sysc->num);
+       if (qfull(p->strace->q)) {
+               if (p->strace->drop_overflow || !sysc_can_block(sysc_num)) {
+                       atomic_inc(&p->strace->nr_drops);
+                       return FALSE;
+               }
+       }
        if (sysc_num > MAX_SYSCALL_NR)
                return FALSE;
-       /* TODO: for drops, consider checking for qio overflow first. */
        return test_bit(sysc_num, p->strace->trace_set);
 }
 
@@ -145,25 +191,20 @@ static void systrace_start_trace(struct kthread *kthread, struct syscall *sysc)
        kthread->strace = 0;
        if (!should_strace(p, sysc))
                return;
+       /* TODO: consider a block_alloc and qpass, though note that we actually
+        * write the same trace in twice (entry and exit). */
        trace = kmalloc(SYSTR_BUF_SZ, MEM_ATOMIC);
        if (p->strace) {
-               /* We're using qiwrite below, which has no flow control.  We'll do it
-                * manually.  TODO: consider a block_alloc and qpass, though note that
-                * we actually write the same trace in twice (entry and exit).
-                * Alternatively, we can add another qio method that has flow control
-                * and non blocking. */
-               if (qfull(p->strace->q)) {
+               if (!trace) {
                        atomic_inc(&p->strace->nr_drops);
-                       kfree(trace);
                        return;
                }
-               if (!trace)
-                       atomic_inc(&p->strace->nr_drops);
                /* Avoiding the atomic op.  We sacrifice accuracy for less overhead. */
                p->strace->appx_nr_sysc++;
+       } else {
+               if (!trace)
+                       return;
        }
-       if (!trace)
-               return;
        /* if you ever need to debug just one strace function, this is
         * handy way to do it: just bail out if it's not the one you
         * want.
@@ -2593,6 +2634,7 @@ void run_local_syscall(struct syscall *sysc)
        }
        pcpui->cur_kthread->sysc = sysc;        /* let the core know which sysc it is */
        systrace_start_trace(pcpui->cur_kthread, sysc);
+       pcpui = &per_cpu_info[core_id()];       /* reload again */
        alloc_sysc_str(pcpui->cur_kthread);
        /* syscall() does not return for exec and yield, so put any cleanup in there
         * too. */
@@ -2602,6 +2644,7 @@ void run_local_syscall(struct syscall *sysc)
        pcpui = &per_cpu_info[core_id()];
        free_sysc_str(pcpui->cur_kthread);
        systrace_finish_trace(pcpui->cur_kthread, sysc->retval);
+       pcpui = &per_cpu_info[core_id()];       /* reload again */
        /* Some 9ns paths set errstr, but not errno.  glibc will ignore errstr.
         * this is somewhat hacky, since errno might get set unnecessarily */
        if ((current_errstr()[0] != 0) && (!sysc->err))