commit 11c39a7375bd2759b53b89236e755c91a4f5aad8

Author: Frank Ch. Eigler <fche@redhat.com>

Date:   Tue Jun 16 20:35:53 2020 -0400

    RHBZ1847676: uprobes-inode tweaks redux

    Added (back) a spinlock to manage the stapiu_consumer -> process_list

    structure, since it is occasionally travered from uprobe pre-handlers,

    which are sometimes entered in atomic context (e.g. on rhel7).  There,

    the normal mutex_t is unsafe.  So restoring a spinlock_t just for

    those shortlived traversals, rhel7 and rawhide are both happy.

diff --git a/runtime/linux/uprobes-inode.c b/runtime/linux/uprobes-inode.c

index 156360e..922c9f1 100644

--- a/runtime/linux/uprobes-inode.c

+++ b/runtime/linux/uprobes-inode.c

@@ -143,7 +143,8 @@ struct stapiu_consumer {

   struct list_head instance_list_head; // the resulting uprobe instances for this consumer

   struct list_head process_list_head; // the processes for this consumer

-

+  spinlock_t process_list_lock; // protect list; used briefly from even atomic contexts

+        

   // List of perf counters used by each probe

   // This list is an index into struct stap_perf_probe,

   long perf_counters_dim;

@@ -174,16 +175,19 @@ stapiu_probe_prehandler (struct uprobe_consumer *inst, struct pt_regs *regs)

       // First find the related process, set by stapiu_change_plus.

       // NB: This is a linear search performed for every probe hit!

-      // This could be an algorithmic problem if the list gets large, but

-      // we'll wait until this is demonstratedly a hotspot before optimizing.

-      mutex_lock(&c->consumer_lock);

+      // This could be an algorithmic problem if the list gets large,

+      // but we'll wait until this is demonstratedly a hotspot before

+      // optimizing.  NB: on rhel7 sometimes we're invoked from atomic

+      // context, so must be careful to use the spinlock, not the

+      // mutex.

+      spin_lock(&c->process_list_lock);

       list_for_each_entry(p, &c->process_list_head, process_list) {

 	if (p->tgid == current->tgid) {

 	  process = p;

 	  break;

 	}

       }

-      mutex_unlock(&c->consumer_lock);

+      spin_unlock(&c->process_list_lock);

       if (!process) {

 #ifdef UPROBE_HANDLER_REMOVE

 	/* Once we're past the starting phase, we can be sure that any

@@ -344,7 +348,7 @@ static void

 stapiu_decrement_semaphores(struct stapiu_consumer *consumers, size_t nconsumers)

 {

   size_t i;

-  /* NB: no stapiu_process_slots_lock needed, as the task_finder engine is

+  /* NB: no process_list_lock use needed as the task_finder engine is

    * already stopped by now, so no one else will mess with us.  We need

    * to be sleepable for access_process_vm.  */

   for (i = 0; i < nconsumers; ++i) {

@@ -433,7 +437,8 @@ stapiu_init(struct stapiu_consumer *consumers, size_t nconsumers)

     INIT_LIST_HEAD(&c->instance_list_head);

     INIT_LIST_HEAD(&c->process_list_head);

     mutex_init(&c->consumer_lock);

-

+    spin_lock_init(&c->process_list_lock);

+    

     dbug_uprobes("registering task-finder for procname:%s buildid:%s\n",

 		 ((char*)c->finder.procname ?: (char*)""),

 		 ((char*)c->finder.build_id ?: (char*)""));

@@ -560,7 +565,9 @@ stapiu_change_plus(struct stapiu_consumer* c, struct task_struct *task,

    * calls us in this case with relocation=offset=0, so

    * we don't have to worry about it.  */

   p->base = relocation - offset;

+  spin_lock (&c->process_list_lock);

   list_add(&p->process_list, &c->process_list_head);

+  spin_unlock (&c->process_list_lock);

   rc = 0;

   mutex_unlock(&c->consumer_lock);

@@ -587,28 +594,40 @@ stapiu_change_semaphore_plus(struct stapiu_consumer* c, struct task_struct *task

 {

   int rc = 0;

   struct stapiu_process *p;

+  int any_found;

   if (! c->sdt_sem_offset) // nothing to do

     return 0;

-  /* NB: no lock after this point, as we need to be sleepable for

-   * get/put_user semaphore action.  The given process should be frozen

-   * while we're busy, so it's not an issue.

-   */

-

-  mutex_lock(&c->consumer_lock);

-

+  // NB: we mustn't hold a lock while changing the task memory,

+  // but we need a lock to protect the process_list from concurrent

+  // add/delete.  So hold a spinlock during iteration until the first

+  // hit, then unlock & process.  NB: We could in principle have multiple

+  // instances of the same process in the list (e.g., if the process

+  // somehow maps in the same solib multiple times).  We can't easily

+  // both iterate this list (in a spinlock-protected safe way), and

+  // relax the spinlock enough to do a safe stapiu_write_task_semaphore()

+  // call within the loop.  So we will hit only the copy in our list.

+  any_found = 0;

+  spin_lock(&c->process_list_lock);

   /* Look through all the consumer's processes and increment semaphores.  */

   list_for_each_entry(p, &c->process_list_head, process_list) {

     unsigned long addr = p->base + c->sdt_sem_offset;

     if (addr >= relocation && addr < relocation + length) {

-      int rc2 = stapiu_write_task_semaphore(task, addr, +1);

+      int rc2;

+      // unlock list and process write for this entry

+      spin_unlock(&c->process_list_lock);

+      any_found=1;

+      rc2 = stapiu_write_task_semaphore(task, addr, +1);

       if (!rc)

-	rc = rc2;

+        rc = rc2;

+      break; // exit list_for_each loop

     }

   }

-

-  mutex_unlock(&c->consumer_lock);

+  if (! any_found)

+    spin_unlock(&c->process_list_lock);

+  else

+    ; // already unlocked

   return rc;

 }

@@ -635,8 +654,9 @@ stapiu_change_minus(struct stapiu_consumer* c, struct task_struct *task,

   //   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:

+  //   and cleaning it up NOW is tricky - we could use the process_list_lock

+  //   to protect the list (as done in stapiu_change_semaphore_plus),

+  //   but not our sleepy mutex:

   //

   // [ 1955.410237]  ? stapiu_change_minus+0x38/0xf0 [stap_54a723c01c50d972590a5c901516849_15522]

   // [ 1955.411583]  __mutex_lock+0x35/0x820

commit 4ccdfe4536d702612912e96d7b6278b169917eaa

Author: Frank Ch. Eigler <fche@redhat.com>

Date:   Mon Jul 6 13:27:46 2020 -0400

    RHBZ1847676 cont'd: more uprobes-inode/onthefly concurrency controls

    The systemtap.onthefly/*.exp tests had recently become hang-prone on

    some kernels, for reasons still not completely understood.  This set

    of patches adds:

    - irq*-block spinlocks into uprobes-invoked paths, in case there is

      peculiar reentrancy (from irq-related tracepoints)

    - a mutex lock/unlock into the stapiu_exit() path, in case there is

      a concurrent stapiu_refresh() invoked by onthefly machinery around

      exit time

    - restrictions into the onthefly module_refresh() translator code to

      preclude STAP_SESSION_STOPPING as a time to do any sort of refresh

      operation.  Now probes that were disarmed will stay disarmed during

      probe-end/error/etc. processing, which is always valid with the

      spec, and avoids a class of late module-refresh ops

    Testing on rhel7 and rawhide indicates the reproducible hang is gone.

    Our testsuite already tortures this code; invoke by hand via:

    % sudo make installcheck RUNTESTFLAGS="-v affection.exp hrtimer_onthefly.exp kprobes_onthefly.exp tracepoint_onthefly.exp uprobes_onthefly.exp"

diff --git a/runtime/linux/uprobes-inode.c b/runtime/linux/uprobes-inode.c

index 922c9f1..3de7281 100644

--- a/runtime/linux/uprobes-inode.c

+++ b/runtime/linux/uprobes-inode.c

@@ -172,6 +172,7 @@ stapiu_probe_prehandler (struct uprobe_consumer *inst, struct pt_regs *regs)

   if (_stp_target) // need we filter by pid at all?

     {

       struct stapiu_process *p, *process = NULL;

+      unsigned long flags;

       // First find the related process, set by stapiu_change_plus.

       // NB: This is a linear search performed for every probe hit!

@@ -180,14 +181,14 @@ stapiu_probe_prehandler (struct uprobe_consumer *inst, struct pt_regs *regs)

       // optimizing.  NB: on rhel7 sometimes we're invoked from atomic

       // context, so must be careful to use the spinlock, not the

       // mutex.

-      spin_lock(&c->process_list_lock);

+      spin_lock_irqsave(&c->process_list_lock, flags);

       list_for_each_entry(p, &c->process_list_head, process_list) {

 	if (p->tgid == current->tgid) {

 	  process = p;

 	  break;

 	}

       }

-      spin_unlock(&c->process_list_lock);

+      spin_unlock_irqrestore(&c->process_list_lock, flags);

       if (!process) {

 #ifdef UPROBE_HANDLER_REMOVE

 	/* Once we're past the starting phase, we can be sure that any

@@ -398,7 +399,7 @@ static void

 stapiu_consumer_refresh(struct stapiu_consumer *c)

 {

   struct stapiu_instance *inst;

-

+  

   mutex_lock(& c->consumer_lock);

   list_for_each_entry(inst, &c->instance_list_head, instance_list) {

@@ -420,7 +421,10 @@ stapiu_exit(struct stapiu_consumer *consumers, size_t nconsumers)

   stapiu_decrement_semaphores(consumers, nconsumers);

   for (i = 0; i < nconsumers; ++i) {

     struct stapiu_consumer *c = &consumers[i];

+    // protect against conceivable stapiu_refresh() at same time

+    mutex_lock(& c->consumer_lock);

     stapiu_consumer_unreg(c);

+    mutex_unlock(& c->consumer_lock);

     /* NB: task_finder needs no unregister. */

   }

 }

@@ -480,6 +484,7 @@ stapiu_change_plus(struct stapiu_consumer* c, struct task_struct *task,

   struct stapiu_instance *inst = NULL;

   struct stapiu_process *p;

   int j;

+  unsigned long flags;

   if (! inode) {

       rc = -EINVAL;

@@ -565,9 +570,9 @@ stapiu_change_plus(struct stapiu_consumer* c, struct task_struct *task,

    * calls us in this case with relocation=offset=0, so

    * we don't have to worry about it.  */

   p->base = relocation - offset;

-  spin_lock (&c->process_list_lock);

+  spin_lock_irqsave (&c->process_list_lock, flags);

   list_add(&p->process_list, &c->process_list_head);

-  spin_unlock (&c->process_list_lock);

+  spin_unlock_irqrestore (&c->process_list_lock, flags);

   rc = 0;

   mutex_unlock(&c->consumer_lock);

@@ -595,6 +600,7 @@ stapiu_change_semaphore_plus(struct stapiu_consumer* c, struct task_struct *task

   int rc = 0;

   struct stapiu_process *p;

   int any_found;

+  unsigned long flags;

   if (! c->sdt_sem_offset) // nothing to do

     return 0;

@@ -609,14 +615,14 @@ stapiu_change_semaphore_plus(struct stapiu_consumer* c, struct task_struct *task

   // relax the spinlock enough to do a safe stapiu_write_task_semaphore()

   // call within the loop.  So we will hit only the copy in our list.

   any_found = 0;

-  spin_lock(&c->process_list_lock);

+  spin_lock_irqsave(&c->process_list_lock, flags);

   /* Look through all the consumer's processes and increment semaphores.  */

   list_for_each_entry(p, &c->process_list_head, process_list) {

     unsigned long addr = p->base + c->sdt_sem_offset;

     if (addr >= relocation && addr < relocation + length) {

       int rc2;

       // unlock list and process write for this entry

-      spin_unlock(&c->process_list_lock);

+      spin_unlock_irqrestore(&c->process_list_lock, flags);

       any_found=1;

       rc2 = stapiu_write_task_semaphore(task, addr, +1);

       if (!rc)

@@ -625,7 +631,7 @@ stapiu_change_semaphore_plus(struct stapiu_consumer* c, struct task_struct *task

     }

   }

   if (! any_found)

-    spin_unlock(&c->process_list_lock);

+    spin_unlock_irqrestore(&c->process_list_lock, flags);

   else

     ; // already unlocked

diff --git a/translate.cxx b/translate.cxx

index 10b3d32..b10af5a 100644

--- a/translate.cxx

+++ b/translate.cxx

@@ -2144,19 +2144,13 @@ c_unparser::emit_module_refresh ()

     o->newline() << "mutex_lock(&module_refresh_mutex);";

   /* If we're not in STARTING/RUNNING state, don't try doing any work.

-     PR16766 */

+     PR16766.  We don't want to run refresh ops during e.g. STOPPING,

+     so as to possibly activate uprobes near shutdown. */

   o->newline() << "state = atomic_read (session_state());";

-  o->newline() << "if (state != STAP_SESSION_RUNNING && state != STAP_SESSION_STARTING && state != STAP_SESSION_ERROR) {";

-  // cannot _stp_warn etc. since we're not in probe context

-  o->newline(1) << "#if defined(__KERNEL__)";

-  o->newline() << "if (state != STAP_SESSION_STOPPING)";

-  o->newline(1) << "printk (KERN_ERR \"stap module notifier triggered in unexpected state %d\\n\", state);";

-  o->indent(-1);

-  o->newline() << "#endif";

-

+  o->newline() << "if (state != STAP_SESSION_RUNNING && state != STAP_SESSION_STARTING) {";

+  o->newline(1);

   if (!session->runtime_usermode_p())

     o->newline() << "mutex_unlock(&module_refresh_mutex);";

-

   o->newline() << "return;";

   o->newline(-1) << "}";

commit 046fa017d2ab7fea1a4ba2295c31f768c072855e

Author: Frank Ch. Eigler <fche@redhat.com>

Date:   Sun Jul 12 09:57:15 2020 -0400

    RHBZ1847676 cont'd: one more uprobes-inode/onthefly concurrency control

    In uprobes-inode.c (stapiu_change_plus), the runtime can react to

    arrivals of new mappings of a solib or executable by registering new

    uprobes.  Due to an assumption that this could not happen at

    inconvenient times (such as a stapiu_refresh or near shutdown times),

    the actual uprobes registration operation was done outside the

    consumer_lock mutex being held.  But it appears this can happen at bad

    times, so the mutex needs to be held, just like within

    stapiu_consumer_refresh().

    The onthefly tests now survive iterating testing on rawhide+lockdep

    and rhel7+lockdep.

diff --git a/runtime/linux/uprobes-inode.c b/runtime/linux/uprobes-inode.c

index 3de7281..01c8a07 100644

--- a/runtime/linux/uprobes-inode.c

+++ b/runtime/linux/uprobes-inode.c

@@ -575,12 +575,10 @@ stapiu_change_plus(struct stapiu_consumer* c, struct task_struct *task,

   spin_unlock_irqrestore (&c->process_list_lock, flags);

   rc = 0;

-  mutex_unlock(&c->consumer_lock);

-  

   // Register actual uprobe if cond_enabled right now

   if (c->probe->cond_enabled)

     (void) stapiu_register(inst, c);

-  goto out;

+  goto out1;

  out2:

   _stp_kfree(inst);

commit a9a0131eb59e8abc197d3d2a553a86bcdec3dd70

Author: Frank Ch. Eigler <fche@redhat.com>

Date:   Fri Jul 17 22:33:04 2020 -0400

    rhbz1857749: uprobes-inode regression in sdt semaphore setting

    Previous code neglected to set sdt.h semaphores for more than the

    first process systemtap happened to encounter.  This was from a

    mistaken understanding of what it meant for stapiu_change_plus() to be

    called with the same inode/consumer combination.  Even though uprobes

    are automatically shared, each new process still needs its perfctr and

    sdt-semaphores individually set, so we do that now (as before the

    rework of this code).  Mechanized testing incoming shortly.

diff --git a/runtime/linux/uprobes-inode.c b/runtime/linux/uprobes-inode.c

index 01c8a07..de81839 100644

--- a/runtime/linux/uprobes-inode.c

+++ b/runtime/linux/uprobes-inode.c

@@ -190,6 +190,10 @@ stapiu_probe_prehandler (struct uprobe_consumer *inst, struct pt_regs *regs)

       }

       spin_unlock_irqrestore(&c->process_list_lock, flags);

       if (!process) {

+        /* We know that we're in -c/-x mode, but this process is not

+           in the process hierarchy, so the uprobe should be ignored

+           and future hits prevented.  PR15278

+        */

 #ifdef UPROBE_HANDLER_REMOVE

 	/* Once we're past the starting phase, we can be sure that any

 	 * processes which are executing code in a mapping have already

@@ -242,8 +246,8 @@ stapiu_register (struct stapiu_instance* inst, struct stapiu_consumer* c)

 	       (unsigned long) inst->inode->i_ino,

 	       (void*) (uintptr_t) c->offset,

 	       c->probe->index,

-	       ((char*)c->finder.procname ?: (char*)""),

-	       ((char*)c->finder.build_id ?: (char*)""));

+	       ((char*)c->finder.procname ?: ((char*)c->solib_pathname ?: "")),

+               ((char*)c->finder.build_id ?: ((char*)c->solib_build_id ?: "")));

   if (!c->return_p) {

     inst->kconsumer.handler = stapiu_probe_prehandler;

@@ -444,8 +448,8 @@ stapiu_init(struct stapiu_consumer *consumers, size_t nconsumers)

     spin_lock_init(&c->process_list_lock);

     dbug_uprobes("registering task-finder for procname:%s buildid:%s\n",

-		 ((char*)c->finder.procname ?: (char*)""),

-		 ((char*)c->finder.build_id ?: (char*)""));

+                 ((char*)c->finder.procname ?: ""),

+                 ((char*)c->finder.build_id ?: ""));

     ret = stap_register_task_finder_target(&c->finder);

     if (ret != 0) {

@@ -499,22 +503,22 @@ stapiu_change_plus(struct stapiu_consumer* c, struct task_struct *task,

   if (rc)

     goto out;

-  dbug_uprobes("notified for inode-offset u%sprobe "

+  dbug_uprobes("notified for inode-offset arrival u%sprobe "

 	       "%lu:%p pidx %zu target procname:%s buildid:%s\n",

 	       c->return_p ? "ret" : "",

 	       (unsigned long) inode->i_ino,

 	       (void*) (uintptr_t) c->offset,

 	       c->probe->index,

-	       ((char*)c->finder.procname ?: (char*)""),

-	       ((char*)c->finder.build_id ?: (char*)""));

+	       ((char*)c->finder.procname ?: ((char*)c->solib_pathname ?: "")),

+               ((char*)c->finder.build_id ?: ((char*)c->solib_build_id ?: "")));

   /* Check the buildid of the target (if we haven't already). We

    * lock the target so we don't have concurrency issues. */

   mutex_lock(&c->consumer_lock);

-  // Check if we already have an instance for this inode, as though we

-  // were called twice by task-finder mishap, or (hypothetically) the

-  // shlib was mmapped twice.

+  // Check if we already have an instance for this inode.  This is normal:

+  // if a different process maps the same solib, or forks into the same

+  // executable.  In this case, we must not re-register the same uprobe.

   list_for_each_entry(i, &c->instance_list_head, instance_list) {

     if (i->inode == inode) {

       inst = i;

@@ -522,28 +526,33 @@ stapiu_change_plus(struct stapiu_consumer* c, struct task_struct *task,

     }

   }

-  if (inst) { // wouldn't expect a re-notification

-    if (inst->registered_p != c->probe->cond_enabled)

-      // ... this should not happen

-      ;

-    goto out1;

-  }

-

-  // Normal case: need a new one.

-  inst = _stp_kzalloc(sizeof(struct stapiu_instance));

-  if (! inst) {

-    rc = -ENOMEM;

-    goto out1;

-  }

+  if (!inst) { // new instance; need new uprobe etc.

+    // Normal case: need a new one.

+    inst = _stp_kzalloc(sizeof(struct stapiu_instance));

+    if (! inst) {

+      rc = -ENOMEM;

+      goto out1;

+    }

-  inst->sconsumer = c; // back link essential; that's how we go from uprobe *handler callback

+    inst->sconsumer = c; // back link essential; that's how we go from uprobe *handler callback

+          

+    /* Grab the inode first (to prevent TOCTTOU problems). */

+    inst->inode = igrab(inode);

+    if (!inst->inode) {

+      rc = -EINVAL;

+      goto out2;

+    }

+  

+    // Add the inode/instance to the list

+    list_add(&inst->instance_list, &c->instance_list_head);

-  /* Grab the inode first (to prevent TOCTTOU problems). */

-  inst->inode = igrab(inode);

-  if (!inst->inode) {

-    rc = -EINVAL;

-    goto out2;

+    // Register the actual uprobe if cond_enabled already

+    if (c->probe->cond_enabled)

+      (void) stapiu_register(inst, c);

   }

+

+  // ... but we may have to do per-process work anyway: perfctr

+  // initialization and sdt.h semaphore manipulation!

   // Perform perfctr registration if required

   for (j=0; j < c->perf_counters_dim; j++) {

@@ -551,12 +560,10 @@ stapiu_change_plus(struct stapiu_consumer* c, struct task_struct *task,

       (void) _stp_perf_read_init ((c->perf_counters)[j], task);

   }

-  // Add the inode/instance to the list

-  list_add(&inst->instance_list, &c->instance_list_head);

-

   // Associate this consumer with this process.  If we encounter

   // resource problems here, we don't really have to undo the uprobe

-  // registrations etc. already in effect.

+  // registrations etc. already in effect.  It may break correct

+  // tracking of process hierarchy in -c/-x operation, but too bad.

   p = _stp_kzalloc(sizeof(struct stapiu_process));

   if (! p) {

     rc = -ENOMEM;

@@ -573,11 +580,10 @@ stapiu_change_plus(struct stapiu_consumer* c, struct task_struct *task,

   spin_lock_irqsave (&c->process_list_lock, flags);

   list_add(&p->process_list, &c->process_list_head);

   spin_unlock_irqrestore (&c->process_list_lock, flags);

-

+  // NB: actual semaphore value bumping is done later

+  

   rc = 0;

   // Register actual uprobe if cond_enabled right now

-  if (c->probe->cond_enabled)

-    (void) stapiu_register(inst, c);

   goto out1;

  out2:

@@ -617,11 +623,21 @@ stapiu_change_semaphore_plus(struct stapiu_consumer* c, struct task_struct *task

   /* Look through all the consumer's processes and increment semaphores.  */

   list_for_each_entry(p, &c->process_list_head, process_list) {

     unsigned long addr = p->base + c->sdt_sem_offset;

+    if (p->tgid != task->tgid) // skip other processes in the list

+      continue;

     if (addr >= relocation && addr < relocation + length) {

       int rc2;

       // unlock list and process write for this entry

       spin_unlock_irqrestore(&c->process_list_lock, flags);

       any_found=1;

+

+      dbug_uprobes("incrementing semaphore (u%sprobe) pid %ld "

+                   "pidx %zu address %lx\n",

+                   c->return_p ? "ret" : "",

+                   (long) task->tgid,

+                   c->probe->index,

+                   (unsigned long) addr);

+

       rc2 = stapiu_write_task_semaphore(task, addr, +1);

       if (!rc)

         rc = rc2;

@@ -641,15 +657,8 @@ stapiu_change_semaphore_plus(struct stapiu_consumer* c, struct task_struct *task

  * about the semaphores, so we can just release the process slot.  */

 static int

 stapiu_change_minus(struct stapiu_consumer* c, struct task_struct *task,

-		    unsigned long relocation, unsigned long length)

+		    unsigned long addr, unsigned long length)

 {

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

-  

   // We don't need do anything really.

   // A process going away means:

   // - its uprobes will no longer fire: no problem, the uprobe inode

@@ -674,6 +683,36 @@ stapiu_change_minus(struct stapiu_consumer* c, struct task_struct *task,

   // [ 1955.436334]  ? __x64_sys_execve+0x27/0x30

   // [ 1955.437700]  ? do_syscall_64+0x5c/0xa0

+  // But as an optimization - to avoid having them build up indefinitely,

+  // and make semaphore operations go slowly, we will nuke matching entries anyway.

+  unsigned long flags;

+  struct stapiu_process *p, *tmp;

+  unsigned nmatch=0;

+  

+  spin_lock_irqsave(&c->process_list_lock, flags);

+  list_for_each_entry_safe(p, tmp, &c->process_list_head, process_list) {

+    // we nuke by matching semaphore address (where ..._semaphore_plus wrote)

+    // against the address range being unmapped

+    unsigned long semaddr = p->base + c->sdt_sem_offset;

+    if (p->tgid != task->tgid) // skip other processes in the list

+      continue;

+    if (semaddr >= addr && semaddr < addr + length) {

+      list_del(&p->process_list);

+      _stp_kfree (p);

+      nmatch ++;

+    }

+  }

+  spin_unlock_irqrestore(&c->process_list_lock, flags);

+

+  if (nmatch > 0)

+    dbug_uprobes("notified for inode-offset departure u%sprobe "

+                 "pidx %zu matches:%u procname:%s buildid:%s\n",

+                 c->return_p ? "ret" : "",

+                 c->probe->index,

+                 nmatch,

+                 ((char*)c->finder.procname ?: ((char*)c->solib_pathname ?: "")),

+                 ((char*)c->finder.build_id ?: ((char*)c->solib_build_id ?: "")));

+

   return 0;

 }

commit e90530877ee21cffa2a9d53567ba5b5de1dd9b32

Author: Frank Ch. Eigler <fche@redhat.com>

Date:   Mon Jul 27 07:58:30 2020 -0400

    PR25568 / RHBZ1857749: buildid/uprobes/inode rework, task_finder etc. side

    During work on a new stress tests for build-id based probes (coming in

    next commit), it was found that the task_finder2 logic for buildid

    verification didn't, well, work, because it was never run (due to an

    erroneous pathlen conditional), and couldn't be safely run where it

    was (because it was under spinlock but would have done

    access_process_vm).  Reworked the relevant bits of task_finder2 to

    perform build-id verification for processes later - during the quiesce

    callback periods.  (Buildid verification for solibs is already done

    in the task_finder2 consumer uprobes-inode.c.)

    Testing with sdt_misc indicated a case where a preexisting process

    (with solib sdt.h semaphores) was being attached to by a new stap

    binary.  task_finder2's enumeration of the preexising processes'

    memory map segments violated assumptions by recent code related to

    tracking in stapiu_process[] lists.  (It did not mirror the temporal

    ld.so mmap sequence.)  Changed this tracking to use the inode* as the

    key, and stop trying to track mapping lengths, to make positive

    matches and eliminate duplicate stapiu_process[] entries for the same

    (process,solib) permutation.  Reworked stapiu_process[] accumulation

    generally to move to the two immediate task_finder callbacks, out of

    stapiu_change_plus().

    Added lots of commentary and diagnostics throughout.  stap

    -DDEBUG_UPROBES give meaningful info about uprobes & sdt semaphores;

    with -DDEBUG_TASK_FINDER, more but not overwhelming relevant info

    appears.

diff --git a/runtime/linux/task_finder2.c b/runtime/linux/task_finder2.c

index 9777efb..8b8057a 100644

--- a/runtime/linux/task_finder2.c

+++ b/runtime/linux/task_finder2.c

@@ -652,8 +652,8 @@ __verify_build_id(struct task_struct *tsk, unsigned long addr,

 	tsk_build_id[build_id_len] = '\0';

 	if (strcmp(build_id, tsk_build_id)) {

-		dbug_task(2, "target build-id not matched: [%s] != [%s]\n",

-			  build_id, tsk_build_id);

+		dbug_task(2, "target build-id not matched: [%s] @ 0x%lx != [%s]\n",

+			  build_id, addr, tsk_build_id);

 		return false;

 	}

@@ -884,16 +884,9 @@ __stp_utrace_attach_match_filename(struct task_struct *tsk,

 		// procname/build-id and match an "all thread" probe.

 		if (tgt == NULL)

 			continue;

-		/* buildid-based target */

-		else if (tgt->build_id_len > 0 && tgt->procname > 0

-			 && !__verify_build_id(tsk,

-					       tgt->build_id_vaddr,

-					       tgt->build_id,

-					       tgt->build_id_len))

-		{

-			continue;

-		}

-		else if (tgt->build_id_len == 0 && tgt->pathlen > 0

+                /* buildid-based target ... gets checked in __stp_tf_quiesce_worker */

+                /* procname-based target */

+		else if (tgt->pathlen > 0

 			 && (tgt->pathlen != filelen

 			     || strcmp(tgt->procname, filename) != 0))

 		{

@@ -1341,6 +1334,34 @@ __stp_tf_quiesce_worker(struct task_work *work)

 		return;

 	}

+        /* If we had a build-id based executable probe (so we have a

+         * tgt->build_id) set, we could not check it back in

+         * __stp_utrace_attach_* because we can't do sleepy

+         * access_process_vm() calls from there.  BUt now that we're

+         * in process context, quiesced, finally we can check.  If we

+         * were build-id based, and the build-id does not match, then

+         * we UTRACE_DETACH from this process and skip the callbacks.

+         *

+         * XXX: For processes that do match, we redo this check every

+         * time this callbacks is encountered somehow.  That's

+         * probably unnecessary.

+         */

+        if (tgt->build_id_len > 0) {

+                int ok = __verify_build_id(current,

+                                           tgt->build_id_vaddr,

+                                           tgt->build_id,

+                                           tgt->build_id_len);

+

+                dbug_task(2, "verified buildid-target process pid=%ld ok=%d\n",

+                          (long) current->tgid, ok);

+                if (!ok) {

+                        // stap_utrace_detach (current, & tgt->ops);

+                        /* Remember that this task_work_func is finished. */

+                        stp_task_work_func_done();

+                        return;

+                }

+        } 

+        

 	__stp_tf_handler_start();

 	/* NB make sure we run mmap callbacks before other callbacks

@@ -1434,6 +1455,21 @@ __stp_utrace_task_finder_target_quiesce(u32 action,

 		}

 	}

 	else {

+                /* Like in __stp_tf_quiesce_worker(), verify build-id now if belated. */

+                if (tgt->build_id_len > 0) {

+                        int ok = __verify_build_id(current,

+                                                   tgt->build_id_vaddr,

+                                                   tgt->build_id,

+                                                   tgt->build_id_len);

+                        

+                        dbug_task(2, "verified2 buildid-target process pid=%ld ok=%d\n",

+                                  (long) current->tgid, ok);

+                        if (!ok) {

+                                __stp_tf_handler_end();

+                                return UTRACE_RESUME; // NB: not _DETACH; that interferes with other engines

+                        }

+                } 

+                

 		/* NB make sure we run mmap callbacks before other callbacks

 		 * like 'probe process.begin' handlers so that the vma tracker

 		 * is already initialized in the latter contexts */

@@ -1797,15 +1833,7 @@ stap_start_task_finder(void)

 					 struct stap_task_finder_target, list);

 			if (tgt == NULL)

 				continue;

-			/* buildid-based target */

-			else if (tgt->build_id_len > 0 && tgt->procname > 0

-				 && !__verify_build_id(tsk,

-						       tgt->build_id_vaddr,

-						       tgt->build_id,

-						       tgt->build_id_len))

-			{

-				continue;

-			}

+			/* buildid-based target ... gets checked in __stp_tf_quiesce_worker */

 			/* procname-based target */