From: Oleg Nesterov <oleg@redhat.com>
To: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
Alan Stern <stern@rowland.harvard.edu>,
Ingo Molnar <mingo@elte.hu>,
Jan Kratochvil <jan.kratochvil@redhat.com>,
Maneesh Soni <maneesh@linux.vnet.ibm.com>,
Prasad <prasad@linux.vnet.ibm.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] ptrace/x86: dont delay perf_event_disable() till second pass in ptrace_write_dr7()
Date: Tue, 16 Apr 2013 15:30:17 +0200 [thread overview]
Message-ID: <20130416133017.GB9189@redhat.com> (raw)
In-Reply-To: <20130416004452.GG17561@somewhere.redhat.com>
On 04/16, Frederic Weisbecker wrote:
>
> On Sun, Apr 14, 2013 at 09:12:32PM +0200, Oleg Nesterov wrote:
> > ptrace_write_dr7() skips ptrace_modify_breakpoint(disabled => true)
> > unless second_pass, this buys nothing but complicates the code and
> > means that we always do the main loop twice even if "disabled" was
> > never true.
> >
> > The comment says:
> >
> > Don't unregister the breakpoints right-away,
> > unless all register_user_hw_breakpoint()
> > requests have succeeded.
> >
> > I think this logic was always wrong, hw_breakpoint_del() does not
> > free the slot so perf_event_disable() can't hurt.
>
> For the record, I think it was necessary before
> 44234adcdce38f83c56e05f808ce656175b4beeb
> ("hw-breakpoints: Modify breakpoints without unregistering them") because
> modifying a breakpoint implied that the old bp was released and a new one
> was created, opening a little race window in between against concurrent
> breakpoint users.
Aah, thank, I'll update the changelog.
> Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
Thanks!
> > old_dr7 = ptrace_get_dr7(thread->ptrace_bps);
> > @@ -651,35 +643,31 @@ restore:
> > bool disabled = !decode_dr7(data, i, &len, &type);
> > struct perf_event *bp = thread->ptrace_bps[i];
> >
> > - if (disabled) {
> > + if (!bp) {
> > + if (disabled)
> > + continue;
> > /*
> > - * Don't unregister the breakpoints right-away, unless
> > - * all register_user_hw_breakpoint() requests have
> > - * succeeded. This prevents any window of opportunity
> > - * for debug register grabbing by other users.
> > + * We should have at least an inactive breakpoint at
> > + * this slot. It means the user is writing dr7 without
> > + * having written the address register first.
> > */
> > - if (!bp || !second_pass)
> > - continue;
> > + rc = -EINVAL;
> > + break;
> > }
> >
> > rc = ptrace_modify_breakpoint(bp, len, type, tsk, disabled);
> > if (rc)
> > break;
>
> It would be nice to warn here:
>
> WARN_ON_ONCE(rc && second_pass);
Well, I disagree.
To clarify, I agree with WARN_ON_ONCE(), but afaics it has nothing to
do with "second_pass",
> And these are indeed supposed
> to.
Indeed, but this is because ptrace_modify_breakpoint() should not fail.
So, what do you think if I change the main loop above
rc = ptrace_modify_breakpoint(...)
- if (rc)
+ if (WARN_ON_ONCE(rc))
break;
instead?
Oleg.
next prev parent reply other threads:[~2013-04-16 13:33 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-04-14 19:12 [PATCH 0/2] ptrace/x86: simplify ptrace_write_dr7() Oleg Nesterov
2013-04-14 19:12 ` [PATCH 1/2] ptrace/x86: simplify the "disable" logic in ptrace_write_dr7() Oleg Nesterov
2013-04-16 0:03 ` Frederic Weisbecker
2013-04-14 19:12 ` [PATCH 2/2] ptrace/x86: dont delay perf_event_disable() till second pass " Oleg Nesterov
2013-04-16 0:44 ` Frederic Weisbecker
2013-04-16 13:30 ` Oleg Nesterov [this message]
2013-04-16 22:00 ` Frederic Weisbecker
2013-04-17 12:40 ` Oleg Nesterov
2013-04-14 19:30 ` [PATCH 0/2] ptrace/x86: simplify ptrace_write_dr7() Jan Kratochvil
2013-04-14 19:40 ` Oleg Nesterov
2013-04-15 23:36 ` Frederic Weisbecker
2013-04-16 13:25 ` Oleg Nesterov
2013-04-17 4:57 ` Jan Kratochvil
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=20130416133017.GB9189@redhat.com \
--to=oleg@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=fweisbec@gmail.com \
--cc=jan.kratochvil@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=maneesh@linux.vnet.ibm.com \
--cc=mingo@elte.hu \
--cc=prasad@linux.vnet.ibm.com \
--cc=stern@rowland.harvard.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).