No, I can't understand this patch...
On 07/16, Ravi Bangoria wrote:
>
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -63,6 +63,8 @@ static struct percpu_rw_semaphore dup_mmap_sem;
>
> /* Have a copy of original instruction */
> #define UPROBE_COPY_INSN 0
> +/* Reference counter offset is reloaded with non-zero value. */
> +#define REF_CTR_OFF_RELOADED 1
>
> struct uprobe {
> struct rb_node rb_node; /* node in the rb tree */
> @@ -476,9 +478,23 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
> return ret;
>
> ret = verify_opcode(old_page, vaddr, &opcode);
> - if (ret <= 0)
> + if (ret < 0)
> goto put_old;
I agree, "ret <= 0" wasn't nice even before this change, but "ret < 0" looks
worse because this is simply not possible.
> + /*
> + * If instruction is already patched but reference counter offset
> + * has been reloaded to non-zero value, increment the reference
> + * counter and return.
> + */
> + if (ret == 0) {
> + if (is_register &&
> + test_bit(REF_CTR_OFF_RELOADED, &uprobe->flags)) {
> + WARN_ON(!uprobe->ref_ctr_offset);
> + ret = update_ref_ctr(uprobe, mm, true);
> + }
> + goto put_old;
> + }
So we need to force update_ref_ctr(true) in case when uprobe_register_refctr()
detects the already registered uprobe with ref_ctr_offset == 0, and then it calls
register_for_each_vma().
Why this can't race with uprobe_mmap() ?
uprobe_mmap() can do install_breakpoint() right after REF_CTR_OFF_RELOADED was set,
then register_for_each_vma() will find this vma and do install_breakpoint() too.
If ref_ctr_vma was already mmaped, the counter will be incremented twice, no?
> @@ -971,6 +1011,7 @@ register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)
> bool is_register = !!new;
> struct map_info *info;
> int err = 0;
> + bool installed = false;
>
> percpu_down_write(&dup_mmap_sem);
> info = build_map_info(uprobe->inode->i_mapping,
> @@ -1000,8 +1041,10 @@ register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)
> if (is_register) {
> /* consult only the "caller", new consumer. */
> if (consumer_filter(new,
> - UPROBE_FILTER_REGISTER, mm))
> + UPROBE_FILTER_REGISTER, mm)) {
> err = install_breakpoint(uprobe, mm, vma, info->vaddr);
> + installed = true;
> + }
> } else if (test_bit(MMF_HAS_UPROBES, &mm->flags)) {
> if (!filter_chain(uprobe,
> UPROBE_FILTER_UNREGISTER, mm))
> @@ -1016,6 +1059,8 @@ register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)
> }
> out:
> percpu_up_write(&dup_mmap_sem);
> + if (installed)
> + clear_bit(REF_CTR_OFF_RELOADED, &uprobe->flags);
I simply can't understand this "bool installed"....
shouldn't we clear REF_CTR_OFF_RELOADED unconditionally after register_for_each_vma()?
Also. Suppose we have a registered uprobe with ref_ctr_offset == 0. Then you add and
remove uprobe with ref_ctr_offset != 0. But afaics uprobe->ref_ctr_offset is never
cleared, so another uprobe with a different ref_ctr_offset != 0 will hit pr_warn/-EINVAL
in alloc_uprobe() and find_old_trace_uprobe() added by the previous patch can't detect
this case?
Plus it seems that we can have the unbalanced update_ref_ctr(false), at least in case
when __uprobe_register() with REF_CTR_OFF_RELOADED set fails before it patches all mm's.
If/when the 1st uprobe with ref_ctr_offset == 0 goes away, remove_breakpoint() will dec
the counter even if wasn't incremented.
Quite possibly I am totally confused, but this patch wrong in many ways...
Oleg.
