From: oleg@redhat•com (Oleg Nesterov)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v6 5/6] Uprobes/sdt: Prevent multiple reference counter for same uprobe
Date: Wed, 25 Jul 2018 13:08:02 +0200 [thread overview]
Message-ID: <20180725110802.GA27325@redhat.com> (raw)
In-Reply-To: <20180716084706.28244-6-ravi.bangoria@linux.ibm.com>
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.
next prev parent reply other threads:[~2018-07-25 11:08 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-16 8:47 [PATCH v6 0/6] Uprobes: Support SDT markers having reference count (semaphore) Ravi Bangoria
2018-07-16 8:47 ` [PATCH v6 1/6] Uprobes: Simplify uprobe_register() body Ravi Bangoria
2018-07-16 8:47 ` [PATCH v6 2/6] Uprobe: Additional argument arch_uprobe to uprobe_write_opcode() Ravi Bangoria
2018-07-16 8:47 ` [PATCH v6 3/6] Uprobes: Support SDT markers having reference count (semaphore) Ravi Bangoria
2018-07-23 16:26 ` Oleg Nesterov
2018-07-24 3:34 ` Ravi Bangoria
2018-07-27 13:59 ` Oleg Nesterov
2018-07-24 14:21 ` Masami Hiramatsu
2018-07-16 8:47 ` [PATCH v6 4/6] trace_uprobe/sdt: Prevent multiple reference counter for same uprobe Ravi Bangoria
2018-07-16 8:47 ` [PATCH v6 5/6] Uprobes/sdt: " Ravi Bangoria
2018-07-25 11:08 ` Oleg Nesterov [this message]
2018-07-27 4:17 ` Ravi Bangoria
2018-07-27 13:55 ` Oleg Nesterov
2018-07-16 8:47 ` [PATCH v6 6/6] perf probe: Support SDT markers having reference counter (semaphore) Ravi Bangoria
2018-07-20 13:47 ` [PATCH v6 0/6] Uprobes: Support SDT markers having reference count (semaphore) Ravi Bangoria
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180725110802.GA27325@redhat.com \
--to=oleg@redhat$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox