This is the mail archive of the
systemtap@sources.redhat.com
mailing list for the systemtap project.
Re: [RFC] Design + prototype: Multiple kprobes at an address - take2
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