linux-kernel - Re: [RFC PATCH] hw-breakpoints, kgdb, x86: add a flag to pass DIE

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100723130430.GA5255@nowhere>
Date:	Fri, 23 Jul 2010 15:04:33 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Dongdong Deng <dongdong.deng@...driver.com>
Cc:	jason.wessel@...driver.com, will.deacon@....com,
	lethal@...ux-sh.org, mahesh@...ux.vnet.ibm.com,
	prasad@...ux.vnet.ibm.com, benh@...nel.crashing.org,
	paulus@...ba.org, mingo@...e.hu,
	kgdb-bugreport@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] hw-breakpoints, kgdb, x86: add a flag to pass
	DIE_DEBUG notification

On Fri, Jul 23, 2010 at 10:16:01AM +0800, Dongdong Deng wrote:
> The hw_breakpoint subsystem consumes all the hardware
> breakpoint exceptions since it hooks the notify_die
> handlers first, this means that kgdb doesn't get the
> opportunity to handle hw breakpoint exceptions generated
> by kgdb itself.
> 
> This patch adds an extend flag to perf_event_attr for
> hw_breakpoint_handler() to decide to pass or stop the
> DIE_DEBUG notification.
> 
> As KGDB set that flag, hw_breakpoint_handler() will pass
> the DIE_DEBUG notification, thus kgdb have the chance
> to take DIE_DEBUG notification.
> 
> Signed-off-by: Dongdong Deng <dongdong.deng@...driver.com>
> Reviewed-by: Bruce Ashfield <bruce.ashfield@...driver.com>
> ---
>  arch/x86/kernel/hw_breakpoint.c |   14 ++++++++++++++
>  arch/x86/kernel/kgdb.c          |    2 ++
>  include/linux/perf_event.h      |    9 +++++++++
>  3 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> index a8f1b80..b38f786 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
> @@ -406,6 +406,8 @@ EXPORT_SYMBOL_GPL(hw_breakpoint_restore);
>   * ii) When there are more bits than trap<n> set in DR6 register (such
>   * as BD, BS or BT) indicating that more than one debug condition is
>   * met and requires some more action in do_debug().
> + * iii) The source of hw breakpoint event want to handle the event
> + * by itself, currently just KGDB have this notion.
>   *
>   * NOTIFY_STOP returned for all other cases
>   *
> @@ -464,6 +466,18 @@ static int __kprobes hw_breakpoint_handler(struct die_args *args)
>  			break;
>  		}
>  
> +		if (bp->attr.flag == SKIP_HWBP_EVENT_PERF_FLAG) {
> +			/*
> +			 * when attr.flag is set to SKIP_HWBP_EVENT_PERF_FLAG
> +			 * it indicates currently hw breakpoint event
> +			 * source want to handle this event by itself.
> +			 * thus return NOTIFY_DONE here.
> +			 */
> +			rc = NOTIFY_DONE;
> +			rcu_read_unlock();
> +			break;
> +		}
> +



No. We really shouldn't make a user ABI change (adding attr.flag) just
to solve an in-kernel-only problem.

And moreover we probably don't need flags at all. Why not just turning kgdb handler
into a higher priority?

I don't even remember why kgdb has its own handler instead of using the
struct perf_event:overflow_handler. May be that's because of the early breakpoints.

--
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/