| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20091221184631.GB24535@in.ibm.com>
Date: Tue, 22 Dec 2009 00:16:31 +0530
From: "K.Prasad" <prasad@...ux.vnet.ibm.com>
To: Frederic Weisbecker <fweisbec@...il.com>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"mingo@...e.hu" <mingo@...e.hu>,
Peter Zijlstra <peterz@...radead.org>
Subject: Re: [Patch 1/1] Introduce register_user_hbp_by_pid() and
unregister_user_hbp_by_pid()
On Fri, Dec 18, 2009 at 09:47:48PM +0100, Frederic Weisbecker wrote:
> On Thu, Dec 17, 2009 at 10:52:53PM +0530, K.Prasad wrote:
> > Provide an interface to (un)register user-space breakpoints using a
> > process' pid.
> >
> > Signed-off-by: K.Prasad <prasad@...ux.vnet.ibm.com>
> > ---
> > include/linux/hw_breakpoint.h | 8 +++
> > kernel/hw_breakpoint.c | 92 ++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 100 insertions(+)
> >
<snipped>
> >
> > +/*
> > + * Unregister breakpoints thread-by-thread, for all threads ranging from
> > + * @start to @end.
> > + */
> > +static inline void __unregister_user_hbp_for_threads(struct task_struct *start,
> > + struct task_struct *end)
>
> I'm not sure this wants to be inlined. The function is not not
> that tiny. May be let the compiler choose?
Okay...will make it just static.
> > +{
> > + struct perf_event *bp, *temp_bp;
> > +
> > + do {
> > + mutex_lock(&start->perf_event_mutex);
> > + list_for_each_entry_safe(bp, temp_bp, &start->perf_event_list,
> > + owner_entry) {
> > + if (bp->attr.type != PERF_TYPE_BREAKPOINT)
> > + continue;
> > + unregister_hw_breakpoint(bp);
> > + break;
>
> Do you really want to unregister all of them? What about those
> that may have been registered using perf syscall?
>
Seems that I got influenced heavily by the PPC64 design (was coding
simultaneously for it :-)...will accept a "struct perf_event_attr *" and
use that to identify appropriate breakpoint.
> > +/**
> > + * register_user_hbp_by_pid - register a hardware breakpoint for user space using pid
> > + * @attr: breakpoint attributes
> > + * @triggered: callback to trigger when we hit the breakpoint
> > + * @pid: pid of the thread group for which breakpoints must be registered
> > + */
> > +int register_user_hbp_by_pid(struct perf_event_attr *attr,
> > + perf_overflow_handler_t triggered,
> > + pid_t pid)
> > +{
> > + int ret;
> > + struct task_struct *t1, *t2;
>
> This needs rcu_read_lock()
>
Ok.
> > + t1 = t2 = find_task_by_vpid(pid);
> > + if (t1 == NULL)
> > + return -ESRCH;
> > +
> > + /*
> > + * Ensure that the breakpoint propogates to every new thread created in
> > + * this thread_group.
> > + */
> > + attr->inherit = 1;
> > + /*
> > + * Register a breakpoint individually for every thread of the
> > + * thread_group using register_user_hw_breakpoint() interface.
> > + * Warning: Involves redundant validation checks using
> > + * arch_validate_hwbkpt_settings().
> > + */
> > + do {
> > + ret = IS_ERR(register_user_hw_breakpoint(attr, triggered, t1));
> > + if (ret)
> > + goto fail;
> > + t1 = next_thread(t1);
> > + } while (t1 != t2);
>
> And this needs rcu_read_unlock()
>
Okay.
> > + return 0;
> > +fail:
> > + /*
> > + * Check if the very first register_user_hw_breakpoint() request
> > + * failed. If then, do nothing but return the error value.
> > + */
> > + if (t1 == t2)
> > + return ret;
> > + /*
> > + * Since there exists a thread where the breakpoint request was not
> > + * successful, we are unable to provide a process-wide breakpoint. Hence
> > + * cleanup the breakpoints from the previously registered threads.
> > + */
> > + __unregister_user_hbp_for_threads(t2, t1);
>
>
> There too.
>
> Once you play with tasks (if it's not current), and iterate
> through these, you need to protect either by read-lock
> tasklist_lock or using rcu.
>
> Rcu is the much prefered way here.
>
Okay. I wasn't sure if I had taken sufficient locks....thanks for
pointing it out.
> > +/**
> > + * unregister_hbp_by_pid - unregister a user-space hardware breakpoint previously registered using a pid
> > + * @pid: pid of the process for which breakpoint must be unregistered
> > + */
> > +void unregister_user_hbp_by_pid(pid_t pid)
> > +{
> > + struct task_struct *t1, *t2;
> > +
> > + t1 = t2 = find_task_by_vpid(pid);
> > + if (t1 == NULL)
> > + return;
> > +
> > + __unregister_user_hbp_for_threads(t1, t2);
>
>
>
> And this function needs rcu too.
>
> I don't see any in-kernel user for this new feature.
> That would be required to integrate it.
>
The proposed interfaces, as obvious, are mere wrappers over existing
(un)register_user_* interfaces, and don't do anything vastly different
in order to demonstrate them separately.
I can get a sample kernel module ready - that consumes pid and user-space
address to track write accesses, if you prefer it.
Thanks for reviewing the code!
-- K.Prasad
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists