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

[Ananth N Mavinakayanahalli <amavin at redhat dot com>]

Prasanna S Panchamukhi wrote:

Hi Prasanna,

Some comments below:

....

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

...

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

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

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

...

> +/* New interface to remove a inserted kprobe if multiple handlers are defined
> + * for a given address. This routine accepts just a pointer to multi_handler
> + * structure. This routines checks and unregisters the probe, if the user trying
> + * to remove a probe is only the active user. If there are more active user
> + * registerd, it just deletes the multi_handler structure from the list.
> + */
> +int unregister_multiprobe(struct mult_handler *multh)
> +{
> +	struct mult_probe *multp;
> +	struct kprobe *temp;
> +	unsigned long flags = 0, flags1 = 0;
> +	int ret = -EEXIST;
> +
> +	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)
> +		goto out;
> +
> +	if (!(multp = container_of(temp, struct mult_probe, comm_probe)))
> +		goto out;

Because of the reason I mention above, the if() check above will never 
succeed, ie., goto out will never be executed.


Thanks,
Ananth