Re: [PATCH] x86/kgdb: Allow removal of early BPs

[Lorena Kretzschmar]

Hi,

I wanted to ask about the status of the patch. Let us know if there are any
other steps we can undertake.

Kind regards
Lorena

On Fri 2020-12-18 16:44:21, Daniel Thompson wrote:
> Hi Stefan
> 
> On Mon, Dec 14, 2020 at 03:13:12PM +0100, Stefan Saecherl wrote:
> > The problem is that breakpoints that are set early (e.g. via kgdbwait)
> > cannot be deleted after boot completed (to be precise after mark_rodata_ro
> > ran).
> > 
> > When setting a breakpoint early there are executable pages that are
> > writable so the copy_to_kernel_nofault call in kgdb_arch_set_breakpoint
> > succeeds and the breakpoint is saved as type BP_BREAKPOINT.
> > 
> > Later in the boot write access to these pages is restricted. So when
> > removing the breakpoint the copy_to_kernel_nofault call in
> > kgdb_arch_remove_breakpoint is destined to fail and the breakpoint removal
> > fails. So after copy_to_kernel_nofault failed try to text_poke_kgdb which
> > can work around nonwriteability.
> > 
> > One thing to consider when doing this is that code can go away during boot
> > (e.g. .init.text). Previously kgdb_arch_remove_breakpoint handled this case
> > gracefully by just having copy_to_kernel_nofault fail but if one then calls
> > text_poke_kgdb the system dies due to the BUG_ON we moved out of
> > __text_poke.  To avoid this __text_poke now returns an error in case of a
> > nonpresent code page and the error is handled at call site.
> > 
> > Checkpatch complains about two uses of BUG_ON but the new code should not
> > trigger BUG_ON in cases where the old didn't.
> > 
> > Co-developed-by: Lorena Kretzschmar <qy15sije@xxxxxxxxxxxxx>
> > Signed-off-by: Lorena Kretzschmar <qy15sije@xxxxxxxxxxxxx>
> > Signed-off-by: Stefan Saecherl <stefan.saecherl@xxxxxx>
> 
> I took this to be a gap in the kgdbtest suite so I added a couple of
> tests that cover this area. Before this patch they failed now they
> pass (at least they do for ARCH=x86).
> 
> I don't see any new failures either, so:
> 
> Tested-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> 
> 
> Daniel.
> 
> 
> 
> > ---
> >  arch/x86/kernel/alternative.c | 16 +++++++----
> >  arch/x86/kernel/kgdb.c        | 54 ++++++++++++++++++++++++-----------
> >  2 files changed, 48 insertions(+), 22 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> > index 2400ad62f330..0f145d837885 100644
> > --- a/arch/x86/kernel/alternative.c
> > +++ b/arch/x86/kernel/alternative.c
> > @@ -878,11 +878,9 @@ static void *__text_poke(void *addr, const void *opcode, size_t len)
> >  		if (cross_page_boundary)
> >  			pages[1] = virt_to_page(addr + PAGE_SIZE);
> >  	}
> > -	/*
> > -	 * If something went wrong, crash and burn since recovery paths are not
> > -	 * implemented.
> > -	 */
> > -	BUG_ON(!pages[0] || (cross_page_boundary && !pages[1]));
> > +
> > +	if (!pages[0] || (cross_page_boundary && !pages[1]))
> > +		return ERR_PTR(-EFAULT);
> >  
> >  	/*
> >  	 * Map the page without the global bit, as TLB flushing is done with
> > @@ -976,7 +974,13 @@ void *text_poke(void *addr, const void *opcode, size_t len)
> >  {
> >  	lockdep_assert_held(&text_mutex);
> >  
> > -	return __text_poke(addr, opcode, len);
> > +	addr = __text_poke(addr, opcode, len);
> > +	/*
> > +	 * If something went wrong, crash and burn since recovery paths are not
> > +	 * implemented.
> > +	 */
> > +	BUG_ON(IS_ERR(addr));
> > +	return addr;
> >  }
> >  
> >  /**
> > diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
> > index ff7878df96b4..e98c9c43db7c 100644
> > --- a/arch/x86/kernel/kgdb.c
> > +++ b/arch/x86/kernel/kgdb.c
> > @@ -731,6 +731,7 @@ void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long ip)
> >  int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
> >  {
> >  	int err;
> > +	void *addr;
> >  
> >  	bpt->type = BP_BREAKPOINT;
> >  	err = copy_from_kernel_nofault(bpt->saved_instr, (char *)bpt->bpt_addr,
> > @@ -747,8 +748,14 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
> >  	 */
> >  	if (mutex_is_locked(&text_mutex))
> >  		return -EBUSY;
> > -	text_poke_kgdb((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr,
> > -		       BREAK_INSTR_SIZE);
> > +
> > +	addr = text_poke_kgdb((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr,
> > +				BREAK_INSTR_SIZE);
> > +	/* This should never trigger because the above call to copy_from_kernel_nofault
> > +	 * already succeeded.
> > +	 */
> > +	BUG_ON(IS_ERR(addr));
> > +
> >  	bpt->type = BP_POKE_BREAKPOINT;
> >  
> >  	return 0;
> > @@ -756,21 +763,36 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
> >  
> >  int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
> >  {
> > -	if (bpt->type != BP_POKE_BREAKPOINT)
> > -		goto knl_write;
> > -	/*
> > -	 * It is safe to call text_poke_kgdb() because normal kernel execution
> > -	 * is stopped on all cores, so long as the text_mutex is not locked.
> > -	 */
> > -	if (mutex_is_locked(&text_mutex))
> > -		goto knl_write;
> > -	text_poke_kgdb((void *)bpt->bpt_addr, bpt->saved_instr,
> > -		       BREAK_INSTR_SIZE);
> > -	return 0;
> > +	void *addr;
> > +	int err;
> >  
> > -knl_write:
> > -	return copy_to_kernel_nofault((char *)bpt->bpt_addr,
> > -				  (char *)bpt->saved_instr, BREAK_INSTR_SIZE);
> > +	if (bpt->type == BP_POKE_BREAKPOINT) {
> > +		if (mutex_is_locked(&text_mutex)) {
> > +			err = copy_to_kernel_nofault((char *)bpt->bpt_addr,
> > +							(char *)bpt->saved_instr,
> > +							BREAK_INSTR_SIZE);
> > +		} else {
> > +			/*
> > +			 * It is safe to call text_poke_kgdb() because normal kernel execution
> > +			 * is stopped on all cores, so long as the text_mutex is not locked.
> > +			 */
> > +			addr = text_poke_kgdb((void *)bpt->bpt_addr,
> > +							bpt->saved_instr,
> > +							BREAK_INSTR_SIZE);
> > +			err = PTR_ERR_OR_ZERO(addr);
> > +		}
> > +	} else {
> > +		err = copy_to_kernel_nofault((char *)bpt->bpt_addr,
> > +						(char *)bpt->saved_instr,
> > +						BREAK_INSTR_SIZE);
> > +		if (err == -EFAULT && !mutex_is_locked(&text_mutex)) {
> > +			addr = text_poke_kgdb((void *)bpt->bpt_addr,
> > +						bpt->saved_instr,
> > +						BREAK_INSTR_SIZE);
> > +			err = PTR_ERR_OR_ZERO(addr);
> > +		}
> > +	}
> > +	return err;
> >  }
> >  
> >  const struct kgdb_arch arch_kgdb_ops = {
> > -- 
> > 2.20.1