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

[Florian Rommel]

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