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

[Ananth N Mavinakayanahalli]

Prasanna S Panchamukhi wrote:
>>>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)

Good idea

> 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.
> 

Just verify if the probe exists in the list at all during unregister.

>>
>>>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.

It is a question of correct and incorrect and not harm :-). Also means
less code.

> 
>>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?

Consider this chunk form the patch:

+	spin_lock_irqsave(&kprobe_lock, flags1);
+	temp = get_kprobe(multh->kp.addr);

This will return a valid kprobe if one is present. But this does not
tell us whether the kprobe was registered using the multi interface or
the normal one.

+	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;
+		}
+	}

Now in the else case, you may end up adding the new kprobe to a
non-existing list since you don't have a check if the kprobe handlers
at the address are the aggregate handlers. AFAICS, this can have serious
consequences.

> 
>>>+	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.
> 

We don't need any hashing for the kprobes list at the same address - I
don't see why we should use hlists.

Thanks,
Ananth