linux-kernel - Re: [PATCH v2 2/2] x86/kgdb: fix hang on failed breakpoint removal

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: <01b5b729ba11141dc6a8b67ac50ca4c63d332d18.camel@florommel.de>
Date: Tue, 13 Aug 2024 17:05:40 +0200
From: Florian Rommel <mail@...rommel.de>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, Dave
 Hansen <dave.hansen@...ux.intel.com>, "H . Peter Anvin" <hpa@...or.com>,
 Jason Wessel <jason.wessel@...driver.com>, Daniel Thompson
 <daniel.thompson@...aro.org>,  Douglas Anderson <dianders@...omium.org>,
 Lorena Kretzschmar <qy15sije@....cs.fau.de>, Stefan Saecherl
 <stefan.saecherl@....de>, Peter Zijlstra <peterz@...radead.org>, Christophe
 JAILLET <christophe.jaillet@...adoo.fr>, Randy Dunlap
 <rdunlap@...radead.org>, Masami Hiramatsu <mhiramat@...nel.org>, Andrew
 Morton <akpm@...ux-foundation.org>, Christophe Leroy
 <christophe.leroy@...roup.eu>, Geert Uytterhoeven
 <geert+renesas@...der.be>,  kgdb-bugreport@...ts.sourceforge.net,
 x86@...nel.org,  linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] x86/kgdb: fix hang on failed breakpoint removal

Hi Thomas,

Thanks for the feedback.

On Mon, 2024-08-12 at 23:04 +0200, Thomas Gleixner wrote:
> Either you know it or not. Speculation is reserved for CPUs.

I have now checked it, it is always the static_key patching mechanism
when I reproduce the issue (in QEMU with a varying number of CPUs), but
we can skip this statement.

> 
> >  {
> >  	if (exception == 3 && kgdb_isremovedbreak(regs->ip - 1)) {
> > +		struct kgdb_bkpt *bpt;
> > +		int i, error;
> > +
> >  		regs->ip -= 1;
> > +
> > +		/*
> > +		 * Try to remove the spurious int3 instruction.
> > +		 * These int3s can result from failed breakpoint removals
> > +		 * in kgdb_arch_remove_breakpoint.
> > +		 */
> > +		for (bpt = NULL, i = 0; i < KGDB_MAX_BREAKPOINTS; i++) {
> > +			if (kgdb_break[i].bpt_addr == regs->ip &&
> > +			    kgdb_break[i].state == BP_REMOVED &&
> > +			    (kgdb_break[i].type == BP_BREAKPOINT ||
> > +			     kgdb_break[i].type == BP_POKE_BREAKPOINT)) {
> > +				bpt = &kgdb_break[i];
> > +				break;
> > +			}
> > +		}
> 
> Seriously? The KGBD core already walked that array in
> kgdb_isremovedbreak() just so you can walk it again here.
> 
> struct kkgdb_bkpt *kgdb_get_removed_breakpoint(unsigned long addr)
> {
>         struct kgdb_bkpt *bp = kgdb_break;
>        
> 	for (int i = 0; i < KGDB_MAX_BREAKPOINTS; i++, bp++) {
> 		if (bp>.state == BP_REMOVED && bp->kgdb_bpt_addr == addr)
> 			return bp;
> 	}
> 	return NULL;
> }
> 
> bool kgdb_isremovedbreak(unsigned long addr)
> {
>         return !!kgdb_get_removed_breakpoint(addr);
> }
> 
> bool kgdb_rewind_exception(int exception, struct pt_regs *regs)
> {
>         struct kgdb_bkpt *bp;
> 
> 	if (exception != 3)
>         	return false;
> 
>         bp = kgdb_get_removed_breakpoint(--regs->ip);
>         if (!bp || !bp->type == BP_BREAKPOINT)
>         	return false;
> 
> Hmm?

Ok, ok, looks much better. My intention was to keep the changes in the
generic debug core minimal, especially since efficiency is not
important here.. but I see, it should be done right.

Best regards,
Flo