From bcf55e8f4805aa9d4740311df60d477e2c6b1fd4 Mon Sep 17 00:00:00 2001 From: "Frank Ch. Eigler" Date: Thu, 11 Jun 2020 00:28:01 +0000 Subject: [PATCH] uprobes-inode: rework for buildid vs. onthefly work Tweak locking in the cleanup paths. stapiu_consumer_unreg (at stap exit time) now does all cleanup, including its process_list, and stapiu_change_minus (at probed process exit) does nothing. This seems to strike a good balance between cheering up lockdep and using a little more memory during very long-lived stap jobs. --- runtime/linux/uprobes-inode.c | 55 ++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/runtime/linux/uprobes-inode.c b/runtime/linux/uprobes-inode.c index bbb54baef..156360ec6 100644 --- a/runtime/linux/uprobes-inode.c +++ b/runtime/linux/uprobes-inode.c @@ -366,9 +366,10 @@ static void stapiu_consumer_unreg(struct stapiu_consumer *c) { struct stapiu_instance *inst, *in2; + struct stapiu_process *p, *tmp; - mutex_lock(& c->consumer_lock); - + // no need for locking protection; by the time this cleanup + // is triggered, no further list modifying ops can also go list_for_each_entry_safe(inst, in2, &c->instance_list_head, instance_list) { if (inst->registered_p) stapiu_unregister(inst, c); @@ -378,7 +379,12 @@ stapiu_consumer_unreg(struct stapiu_consumer *c) _stp_kfree(inst); } - mutex_unlock(& c->consumer_lock); + // NB: it's hypothetically possible for the same process to show up + // multiple times in the list. Don't break after the first. + list_for_each_entry_safe(p, tmp, &c->process_list_head, process_list) { + list_del(&p->process_list); + _stp_kfree (p); + } } @@ -614,33 +620,36 @@ static int stapiu_change_minus(struct stapiu_consumer* c, struct task_struct *task, unsigned long relocation, unsigned long length) { - struct stapiu_process *p, *tmp; - dbug_uprobes("notified for inode-offset departure u%sprobe " "pidx %zu target procname:%s buildid:%s\n", c->return_p ? "ret" : "", c->probe->index, ((char*)c->finder.procname ?: (char*)""), ((char*)c->finder.build_id ?: (char*)"")); - - /* NB: we aren't unregistering uprobes and releasing the - * inode here. The registration is system-wide, based on - * inode, not process based. Similarly, any perfctr setup - * is automatically torn down. */ - mutex_lock(&c->consumer_lock); - - // NB: it's hypothetically possible for the same process to show up - // multiple times in the list. Don't break after the first. - list_for_each_entry_safe(p, tmp, &c->process_list_head, process_list) { - if (p->tgid == task->tgid && (relocation <= p->relocation && - p->relocation < relocation+length)) { - list_del(&p->process_list); - _stp_kfree (p); - } - } - - mutex_unlock(&c->consumer_lock); + // We don't need do anything really. + // A process going away means: + // - its uprobes will no longer fire: no problem, the uprobe inode + // is shared and persistent + // - its sdt semaphores (if any) will be nonzero: no problem, the + // process is dying anyway + // - the stapiu_consumer's process_list linked list will have a record + // of the dead process: well, not great, it'll be cleaned up eventually, + // and cleaning it up NOW is tricky - need some spin lock to protect the list, + // but not out sleepy mutex: + // + // [ 1955.410237] ? stapiu_change_minus+0x38/0xf0 [stap_54a723c01c50d972590a5c901516849_15522] + // [ 1955.411583] __mutex_lock+0x35/0x820 + // [ 1955.416858] ? _raw_spin_unlock+0x1f/0x30 + // [ 1955.419649] ? utrace_control+0xbe/0x2d0 [stap_54a723c01c50d972590a5c901516849_15522] + // [ 1955.421702] stapiu_change_minus+0x38/0xf0 [stap_54a723c01c50d972590a5c901516849_15522] + // [ 1955.425147] ? __stp_utrace_task_finder_target_exec+0x74/0xc0 [stap_54a723c01c50d972590a5c901516849_15522] + // [ 1955.429773] ? utrace_report_exec+0xdb/0x140 [stap_54a723c01c50d972590a5c901516849_15522] + // [ 1955.431398] ? __do_execve_file+0xa05/0xb30 + // [ 1955.432923] ? do_execve+0x27/0x30 + // [ 1955.436334] ? __x64_sys_execve+0x27/0x30 + // [ 1955.437700] ? do_syscall_64+0x5c/0xa0 + return 0; } -- 2.43.5