Re: [RFC] Design + prototype: Multiple kprobes at an address - take 2

[Prasanna S Panchamukhi <prasanna at in dot ibm dot com>]

> 
> >register_multiprobe(struct mult_handler *mh): 
> >	User has to allocate mult_handler (defined in kprobes.h) and pass
> >the pointer to register_multiprobe();
> >This routine does some housekeeping by storing reference to individual
> >handlers and registering kprobes with common handler if the user requests 
> >for
> >the first time at a given address. On subsequenct calls to insert probes on
> >the same address, this routines just adds the individual handlers to the 
> >list
> >without registering the kprobes.
> >unregister_multiprobe(struct mult_handler *mh);
> >	User has to pass the mult_handler pointer to unregister.
> >This routine just check if the he is the only active user and calls 
> >unregister
> >kprobes. If there are more active users, it just removes the individual 
> >hanlders
> >inserted by this user from the list.
> 
> This design currently does not handle a case of a mult_probe at an
> address where a kprobe is already present. I see a segfault during
> module unload.
> 
> Are we going to allow the registration of a probe (using existing api)
> and then another one using the multiprobe interface and vice versa?
> 

Well, let wrapper routine over these interfaces handle this issue.
Maneesh has some thoughts about the wrapper routine.(...see maneesh's email
for details)

The segfault you are seeing is not because of my patch, it is the known
bug in the existing kprobes. To recreate just call unregister_kprobe() without
even registering it and it segfaults. I will send out a patch to fix this 
kprobes problem.

> ...
> 
> > unsigned int kprobe_cpu = NR_CPUS;
> > static DEFINE_SPINLOCK(kprobe_lock);
> >+static DEFINE_SPINLOCK(multprobe_lock);
> 
> As Frank suggested, this can be a rwlock.
> 
> >+/* New interface to support multiple handlers feature without even 
> >changing a
> >+ * single line of exiting kprobes interface and data structures. This 
> >routines
> >+ * accepts pointer to mult_handler structure, user has to allocate
> >+ * multi_handler structure and pass the pointer. This routine basically 
> >checks
> >+ * and registers the kprobes common handlers if the user is inserting a 
> >probe
> >+ * for the first time and saves the references to individual kprobes 
> >handlers.
> >+ * On subsequent call to this routine to insert multiple handler at the 
> >same
> >+ * address, it just adds the mult_handler structure to the list.
> >+ */
> >+int register_multiprobe(struct mult_handler *multh)
> >+{
> >+	struct mult_probe *multp = NULL;
> >+	struct kprobe *temp = NULL;
> >+	unsigned long flags = 0, flags1 = 0;
> >+	int ret = 0;
> >+
> >+	spin_lock_irqsave(&multprobe_lock, flags);
> >+
> >+	spin_lock_irqsave(&kprobe_lock, flags1);
> >+	temp = get_kprobe(multh->kp.addr);
> >+	spin_unlock_irqrestore(&kprobe_lock, flags1);
> >+	if (temp == NULL) {
> >+		multp = kmalloc(sizeof(struct mult_probe), GFP_ATOMIC);
> >+		if (!multp) {
> >+			ret = -ENOMEM;
> >+			goto out;
> >+		}
> >+		multp->comm_probe.addr = multh->kp.addr;
> >+		multp->comm_probe.pre_handler = comm_pre_handler;
> >+		multp->comm_probe.post_handler = comm_post_handler;
> >+		multp->comm_probe.fault_handler = comm_fault_handler;
> >+		INIT_HLIST_HEAD(&multp->head);
> >+		register_kprobe(&multp->comm_probe);
> >+	} else {
> >+		multp = container_of(temp, struct mult_probe, comm_probe);
> >+		if (!multp) {
> >+			ret = -EEXIST;
> >+			goto out;
> >+		}
> 
> This is wrong - container_of is a macro and does not verify if you are
> passing a valid pointer imbedded in the higher structure. It always
> returns an address (correct/incorrect depends on the usage). Please
> see my patch of yesterday (take2) for a simpler usage. Same goes for
> other such usages in the patch.
> 

no harm in checking the ptr, but right thing would be to remove the checks.
Will be taken care in my next release.

> And it isn't a bad idea to double-check if temp->pre_handler == 
> comm_pre_handler before making continuing with the registration.
> 

Can you think of a situation where this check is really essential?

> >+	INIT_HLIST_NODE(&multh->hlist);
> >+	hlist_add_head(&multh->hlist, &multp->head);
> 
> Also, why do we need a hlist for the mult_probe and mult_handler
> structs? Can we not do with just lists?
> 

hlist can save some space for us.

Thanks
Prasanna

-- 

Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Ph: 91-80-25044636
<prasanna@in.ibm.com>