Re: [PATCH 7/7] uprobes: _register() should always doregister_for_each_vma(true)

From: Srikar Dronamraju
Date: Thu Dec 13 2012 - 05:57:00 EST


* Oleg Nesterov <oleg@xxxxxxxxxx> [2012-11-23 21:28:17]:

> To support the filtering uprobe_register() should do
> register_for_each_vma(true) every time the new consumer comes,
> we need to install the previously nacked breakpoints.
>
> Note:
> - uprobes_mutex[] should die, what is actually protects is
> alloc_uprobe().
>
> - UPROBE_RUN_HANDLER should die too, obviously it can't work
> unless uprobe has a single consumer. The consumer should
> serialize with _register/_unregister itself. Or this flag
> should live in uprobe_consumer->state.
>
> - Perhaps we can do some optimizations later. For example, if
> filter_chain() never returns false uprobe can record this
> fact and avoid the unnecessary register_for_each_vma().
>
> Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>


Acked-by: Srikar Dronamraju <srikar@xxxxxxxxxxxxxxxxxx>

> ---
> kernel/events/uprobes.c | 31 +++++++++++++------------------
> 1 files changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 7c98671..c80507d 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -482,16 +482,12 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> up_read(&uprobe->consumer_rwsem);
> }
>
> -/* Returns the previous consumer */
> -static struct uprobe_consumer *
> -consumer_add(struct uprobe *uprobe, struct uprobe_consumer *uc)
> +static void consumer_add(struct uprobe *uprobe, struct uprobe_consumer *uc)
> {
> down_write(&uprobe->consumer_rwsem);
> uc->next = uprobe->consumers;
> uprobe->consumers = uc;
> up_write(&uprobe->consumer_rwsem);
> -
> - return uc->next;
> }
>
> /*
> @@ -820,9 +816,15 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
> return err;
> }
>
> -static int __uprobe_register(struct uprobe *uprobe)
> +static int __uprobe_register(struct uprobe *uprobe, struct uprobe_consumer *uc)
> {
> - return register_for_each_vma(uprobe, true);
> + int err;
> +
> + consumer_add(uprobe, uc);
> + err = register_for_each_vma(uprobe, true);
> + if (!err) /* TODO: pointless unless the first consumer */
> + set_bit(UPROBE_RUN_HANDLER, &uprobe->flags);
> + return err;
> }
>
> static void __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
> @@ -867,21 +869,14 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
> if (offset > i_size_read(inode))
> return -EINVAL;
>
> - ret = 0;
> + ret = -ENOMEM;
> mutex_lock(uprobes_hash(inode));
> uprobe = alloc_uprobe(inode, offset);
> -
> - if (!uprobe) {
> - ret = -ENOMEM;
> - } else if (!consumer_add(uprobe, uc)) {
> - ret = __uprobe_register(uprobe);
> - if (ret) {
> + if (uprobe) {
> + ret = __uprobe_register(uprobe, uc);
> + if (ret)
> __uprobe_unregister(uprobe, uc);
> - } else {
> - set_bit(UPROBE_RUN_HANDLER, &uprobe->flags);
> - }
> }
> -
> mutex_unlock(uprobes_hash(inode));
> if (uprobe)
> put_uprobe(uprobe);
> --
> 1.5.5.1
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/