[RFC,1/6] x86/alternative: assert text_mutex is taken
diff mbox series

Message ID 20180829081147.184610-2-namit@vmware.com
State New
Headers show
Series
  • x86: text_poke() fixes
Related show

Commit Message

Nadav Amit Aug. 29, 2018, 8:11 a.m. UTC
Use lockdep to ensure that text_mutex is taken when text_poke() is
called.

Actually it is not always taken, specifically when it is called by kgdb,
so take the lock in these cases.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arch/x86/kernel/alternative.c | 1 +
 arch/x86/kernel/kgdb.c        | 9 +++++++++
 2 files changed, 10 insertions(+)

Comments

Masami Hiramatsu Aug. 29, 2018, 8:59 a.m. UTC | #1
On Wed, 29 Aug 2018 01:11:42 -0700
Nadav Amit <namit@vmware.com> wrote:

> Use lockdep to ensure that text_mutex is taken when text_poke() is
> called.
> 
> Actually it is not always taken, specifically when it is called by kgdb,
> so take the lock in these cases.

Can we really take a mutex in kgdb context?

kgdb_arch_remove_breakpoint
  <- dbg_deactivate_sw_breakpoints
    <- kgdb_reenter_check
       <- kgdb_handle_exception
          <- __kgdb_notify
            <- kgdb_ll_trap
              <- do_int3
            <- kgdb_notify
              <- die notifier

kgdb_arch_set_breakpoint
  <- dbg_activate_sw_breakpoints
    <- kgdb_reenter_check
       <- kgdb_handle_exception
           ...

Both seems called in exception context, so we can not take a mutex lock.
I think kgdb needs a special path.

Thanks,

> 
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  arch/x86/kernel/alternative.c | 1 +
>  arch/x86/kernel/kgdb.c        | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 014f214da581..916c11b410c4 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -699,6 +699,7 @@ void *text_poke(void *addr, const void *opcode, size_t len)
>  	 * pages as they are not yet initialized.
>  	 */
>  	BUG_ON(!after_bootmem);
> +	lockdep_assert_held(&text_mutex);
>  
>  	if (!core_kernel_text((unsigned long)addr)) {
>  		pages[0] = vmalloc_to_page(addr);
> diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
> index 8e36f249646e..60b99c76086c 100644
> --- a/arch/x86/kernel/kgdb.c
> +++ b/arch/x86/kernel/kgdb.c
> @@ -768,8 +768,12 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
>  	 */
>  	if (mutex_is_locked(&text_mutex))
>  		return -EBUSY;
> +
> +	/* Take the mutex to avoid lockdep assertion failures. */
> +	mutex_lock(&text_mutex);
>  	text_poke((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr,
>  		  BREAK_INSTR_SIZE);
> +	mutex_unlock(&text_mutex);
>  	err = probe_kernel_read(opc, (char *)bpt->bpt_addr, BREAK_INSTR_SIZE);
>  	if (err)
>  		return err;
> @@ -793,7 +797,12 @@ int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
>  	 */
>  	if (mutex_is_locked(&text_mutex))
>  		goto knl_write;
> +
> +	/* Take the mutex to avoid lockdep assertion failures. */
> +	mutex_lock(&text_mutex);
>  	text_poke((void *)bpt->bpt_addr, bpt->saved_instr, BREAK_INSTR_SIZE);
> +	mutex_unlock(&text_mutex);
> +
>  	err = probe_kernel_read(opc, (char *)bpt->bpt_addr, BREAK_INSTR_SIZE);
>  	if (err || memcmp(opc, bpt->saved_instr, BREAK_INSTR_SIZE))
>  		goto knl_write;
> -- 
> 2.17.1
>
Nadav Amit Aug. 29, 2018, 5:11 p.m. UTC | #2
at 1:59 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Wed, 29 Aug 2018 01:11:42 -0700
> Nadav Amit <namit@vmware.com> wrote:
> 
>> Use lockdep to ensure that text_mutex is taken when text_poke() is
>> called.
>> 
>> Actually it is not always taken, specifically when it is called by kgdb,
>> so take the lock in these cases.
> 
> Can we really take a mutex in kgdb context?
> 
> kgdb_arch_remove_breakpoint
>  <- dbg_deactivate_sw_breakpoints
>    <- kgdb_reenter_check
>       <- kgdb_handle_exception
>          <- __kgdb_notify
>            <- kgdb_ll_trap
>              <- do_int3
>            <- kgdb_notify
>              <- die notifier
> 
> kgdb_arch_set_breakpoint
>  <- dbg_activate_sw_breakpoints
>    <- kgdb_reenter_check
>       <- kgdb_handle_exception
>           ...
> 
> Both seems called in exception context, so we can not take a mutex lock.
> I think kgdb needs a special path.

You are correct, but I don’t want a special path. Presumably text_mutex is
guaranteed not to be taken according to the code.

So I guess the only concern is lockdep. Do you see any problem if I change
mutex_lock() into mutex_trylock()? It should always succeed, and I can add a
warning and a failure path if it fails for some reason.
Nadav Amit Aug. 29, 2018, 7:36 p.m. UTC | #3
at 10:11 AM, Nadav Amit <namit@vmware.com> wrote:

> at 1:59 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
>> On Wed, 29 Aug 2018 01:11:42 -0700
>> Nadav Amit <namit@vmware.com> wrote:
>> 
>>> Use lockdep to ensure that text_mutex is taken when text_poke() is
>>> called.
>>> 
>>> Actually it is not always taken, specifically when it is called by kgdb,
>>> so take the lock in these cases.
>> 
>> Can we really take a mutex in kgdb context?
>> 
>> kgdb_arch_remove_breakpoint
>> <- dbg_deactivate_sw_breakpoints
>>   <- kgdb_reenter_check
>>      <- kgdb_handle_exception
>>         <- __kgdb_notify
>>           <- kgdb_ll_trap
>>             <- do_int3
>>           <- kgdb_notify
>>             <- die notifier
>> 
>> kgdb_arch_set_breakpoint
>> <- dbg_activate_sw_breakpoints
>>   <- kgdb_reenter_check
>>      <- kgdb_handle_exception
>>          ...
>> 
>> Both seems called in exception context, so we can not take a mutex lock.
>> I think kgdb needs a special path.
> 
> You are correct, but I don’t want a special path. Presumably text_mutex is
> guaranteed not to be taken according to the code.
> 
> So I guess the only concern is lockdep. Do you see any problem if I change
> mutex_lock() into mutex_trylock()? It should always succeed, and I can add a
> warning and a failure path if it fails for some reason.

Err.. This will not work. I think I will drop this patch, since I cannot
find a proper yet simple assertion. Creating special path just for the
assertion seems wrong.
Sean Christopherson Aug. 29, 2018, 8:13 p.m. UTC | #4
On Wed, Aug 29, 2018 at 07:36:22PM +0000, Nadav Amit wrote:
> at 10:11 AM, Nadav Amit <namit@vmware.com> wrote:
> 
> > at 1:59 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> >> On Wed, 29 Aug 2018 01:11:42 -0700
> >> Nadav Amit <namit@vmware.com> wrote:
> >> 
> >>> Use lockdep to ensure that text_mutex is taken when text_poke() is
> >>> called.
> >>> 
> >>> Actually it is not always taken, specifically when it is called by kgdb,
> >>> so take the lock in these cases.
> >> 
> >> Can we really take a mutex in kgdb context?
> >> 
> >> kgdb_arch_remove_breakpoint
> >> <- dbg_deactivate_sw_breakpoints
> >>   <- kgdb_reenter_check
> >>      <- kgdb_handle_exception
> >>         <- __kgdb_notify
> >>           <- kgdb_ll_trap
> >>             <- do_int3
> >>           <- kgdb_notify
> >>             <- die notifier
> >> 
> >> kgdb_arch_set_breakpoint
> >> <- dbg_activate_sw_breakpoints
> >>   <- kgdb_reenter_check
> >>      <- kgdb_handle_exception
> >>          ...
> >> 
> >> Both seems called in exception context, so we can not take a mutex lock.
> >> I think kgdb needs a special path.
> > 
> > You are correct, but I don’t want a special path. Presumably text_mutex is
> > guaranteed not to be taken according to the code.
> > 
> > So I guess the only concern is lockdep. Do you see any problem if I change
> > mutex_lock() into mutex_trylock()? It should always succeed, and I can add a
> > warning and a failure path if it fails for some reason.
> 
> Err.. This will not work. I think I will drop this patch, since I cannot
> find a proper yet simple assertion. Creating special path just for the
> assertion seems wrong.

It's probably worth expanding the comment for text_poke() to call out
the kgdb case and reference kgdb_arch_{set,remove}_breakpoint(), whose
code and comments make it explicitly clear why its safe for them to
call text_poke() without acquiring the lock.  Might prevent someone
from going down this path again in the future.
Nadav Amit Aug. 29, 2018, 8:44 p.m. UTC | #5
at 1:13 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:

> On Wed, Aug 29, 2018 at 07:36:22PM +0000, Nadav Amit wrote:
>> at 10:11 AM, Nadav Amit <namit@vmware.com> wrote:
>> 
>>> at 1:59 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote:
>>> 
>>>> On Wed, 29 Aug 2018 01:11:42 -0700
>>>> Nadav Amit <namit@vmware.com> wrote:
>>>> 
>>>>> Use lockdep to ensure that text_mutex is taken when text_poke() is
>>>>> called.
>>>>> 
>>>>> Actually it is not always taken, specifically when it is called by kgdb,
>>>>> so take the lock in these cases.
>>>> 
>>>> Can we really take a mutex in kgdb context?
>>>> 
>>>> kgdb_arch_remove_breakpoint
>>>> <- dbg_deactivate_sw_breakpoints
>>>>  <- kgdb_reenter_check
>>>>     <- kgdb_handle_exception
>>>>        <- __kgdb_notify
>>>>          <- kgdb_ll_trap
>>>>            <- do_int3
>>>>          <- kgdb_notify
>>>>            <- die notifier
>>>> 
>>>> kgdb_arch_set_breakpoint
>>>> <- dbg_activate_sw_breakpoints
>>>>  <- kgdb_reenter_check
>>>>     <- kgdb_handle_exception
>>>>         ...
>>>> 
>>>> Both seems called in exception context, so we can not take a mutex lock.
>>>> I think kgdb needs a special path.
>>> 
>>> You are correct, but I don’t want a special path. Presumably text_mutex is
>>> guaranteed not to be taken according to the code.
>>> 
>>> So I guess the only concern is lockdep. Do you see any problem if I change
>>> mutex_lock() into mutex_trylock()? It should always succeed, and I can add a
>>> warning and a failure path if it fails for some reason.
>> 
>> Err.. This will not work. I think I will drop this patch, since I cannot
>> find a proper yet simple assertion. Creating special path just for the
>> assertion seems wrong.
> 
> It's probably worth expanding the comment for text_poke() to call out
> the kgdb case and reference kgdb_arch_{set,remove}_breakpoint(), whose
> code and comments make it explicitly clear why its safe for them to
> call text_poke() without acquiring the lock.  Might prevent someone
> from going down this path again in the future.

I thought that the whole point of the patch was to avoid comments, and
instead enforce the right behavior. I don’t understand well enough kgdb
code, so I cannot attest it does the right thing. What happens if
kgdb_do_roundup==0?
Sean Christopherson Aug. 29, 2018, 9 p.m. UTC | #6
On Wed, Aug 29, 2018 at 08:44:47PM +0000, Nadav Amit wrote:
> at 1:13 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
> > On Wed, Aug 29, 2018 at 07:36:22PM +0000, Nadav Amit wrote:
> >> at 10:11 AM, Nadav Amit <namit@vmware.com> wrote:
> >> 
> >>> at 1:59 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >>> 
> >>>> On Wed, 29 Aug 2018 01:11:42 -0700
> >>>> Nadav Amit <namit@vmware.com> wrote:
> >>>> 
> >>>>> Use lockdep to ensure that text_mutex is taken when text_poke() is
> >>>>> called.
> >>>>> 
> >>>>> Actually it is not always taken, specifically when it is called by kgdb,
> >>>>> so take the lock in these cases.
> >>>> 
> >>>> Can we really take a mutex in kgdb context?
> >>>> 
> >>>> kgdb_arch_remove_breakpoint
> >>>> <- dbg_deactivate_sw_breakpoints
> >>>>  <- kgdb_reenter_check
> >>>>     <- kgdb_handle_exception
> >>>>        <- __kgdb_notify
> >>>>          <- kgdb_ll_trap
> >>>>            <- do_int3
> >>>>          <- kgdb_notify
> >>>>            <- die notifier
> >>>> 
> >>>> kgdb_arch_set_breakpoint
> >>>> <- dbg_activate_sw_breakpoints
> >>>>  <- kgdb_reenter_check
> >>>>     <- kgdb_handle_exception
> >>>>         ...
> >>>> 
> >>>> Both seems called in exception context, so we can not take a mutex lock.
> >>>> I think kgdb needs a special path.
> >>> 
> >>> You are correct, but I don’t want a special path. Presumably text_mutex is
> >>> guaranteed not to be taken according to the code.
> >>> 
> >>> So I guess the only concern is lockdep. Do you see any problem if I change
> >>> mutex_lock() into mutex_trylock()? It should always succeed, and I can add a
> >>> warning and a failure path if it fails for some reason.
> >> 
> >> Err.. This will not work. I think I will drop this patch, since I cannot
> >> find a proper yet simple assertion. Creating special path just for the
> >> assertion seems wrong.
> > 
> > It's probably worth expanding the comment for text_poke() to call out
> > the kgdb case and reference kgdb_arch_{set,remove}_breakpoint(), whose
> > code and comments make it explicitly clear why its safe for them to
> > call text_poke() without acquiring the lock.  Might prevent someone
> > from going down this path again in the future.
> 
> I thought that the whole point of the patch was to avoid comments, and
> instead enforce the right behavior. I don’t understand well enough kgdb
> code, so I cannot attest it does the right thing. What happens if
> kgdb_do_roundup==0?

As is, the comment is wrong because there are obviously cases where
text_poke() is called without text_mutex being held.  I can't attest
to the kgdb code either.  My thought was to document the exception so
that if someone does want to try and enforce the right behavior they
can dive right into the problem instead of having to learn of the kgdb
gotcha the hard way.  Maybe a FIXME is the right approach?
Nadav Amit Aug. 29, 2018, 10:56 p.m. UTC | #7
at 2:00 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:

> On Wed, Aug 29, 2018 at 08:44:47PM +0000, Nadav Amit wrote:
>> at 1:13 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>> 
>>> On Wed, Aug 29, 2018 at 07:36:22PM +0000, Nadav Amit wrote:
>>>> at 10:11 AM, Nadav Amit <namit@vmware.com> wrote:
>>>> 
>>>>> at 1:59 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote:
>>>>> 
>>>>>> On Wed, 29 Aug 2018 01:11:42 -0700
>>>>>> Nadav Amit <namit@vmware.com> wrote:
>>>>>> 
>>>>>>> Use lockdep to ensure that text_mutex is taken when text_poke() is
>>>>>>> called.
>>>>>>> 
>>>>>>> Actually it is not always taken, specifically when it is called by kgdb,
>>>>>>> so take the lock in these cases.
>>>>>> 
>>>>>> Can we really take a mutex in kgdb context?
>>>>>> 
>>>>>> kgdb_arch_remove_breakpoint
>>>>>> <- dbg_deactivate_sw_breakpoints
>>>>>> <- kgdb_reenter_check
>>>>>>    <- kgdb_handle_exception
>>>>>>       <- __kgdb_notify
>>>>>>         <- kgdb_ll_trap
>>>>>>           <- do_int3
>>>>>>         <- kgdb_notify
>>>>>>           <- die notifier
>>>>>> 
>>>>>> kgdb_arch_set_breakpoint
>>>>>> <- dbg_activate_sw_breakpoints
>>>>>> <- kgdb_reenter_check
>>>>>>    <- kgdb_handle_exception
>>>>>>        ...
>>>>>> 
>>>>>> Both seems called in exception context, so we can not take a mutex lock.
>>>>>> I think kgdb needs a special path.
>>>>> 
>>>>> You are correct, but I don’t want a special path. Presumably text_mutex is
>>>>> guaranteed not to be taken according to the code.
>>>>> 
>>>>> So I guess the only concern is lockdep. Do you see any problem if I change
>>>>> mutex_lock() into mutex_trylock()? It should always succeed, and I can add a
>>>>> warning and a failure path if it fails for some reason.
>>>> 
>>>> Err.. This will not work. I think I will drop this patch, since I cannot
>>>> find a proper yet simple assertion. Creating special path just for the
>>>> assertion seems wrong.
>>> 
>>> It's probably worth expanding the comment for text_poke() to call out
>>> the kgdb case and reference kgdb_arch_{set,remove}_breakpoint(), whose
>>> code and comments make it explicitly clear why its safe for them to
>>> call text_poke() without acquiring the lock.  Might prevent someone
>>> from going down this path again in the future.
>> 
>> I thought that the whole point of the patch was to avoid comments, and
>> instead enforce the right behavior. I don’t understand well enough kgdb
>> code, so I cannot attest it does the right thing. What happens if
>> kgdb_do_roundup==0?
> 
> As is, the comment is wrong because there are obviously cases where
> text_poke() is called without text_mutex being held.  I can't attest
> to the kgdb code either.  My thought was to document the exception so
> that if someone does want to try and enforce the right behavior they
> can dive right into the problem instead of having to learn of the kgdb
> gotcha the hard way.  Maybe a FIXME is the right approach?

Ok. I’ll add a FIXME comment as you propose, but this does not deserve a
separate patch. I’ll squash it into patch 5.
Masami Hiramatsu Aug. 30, 2018, 2:26 a.m. UTC | #8
On Wed, 29 Aug 2018 14:00:06 -0700
Sean Christopherson <sean.j.christopherson@intel.com> wrote:

> On Wed, Aug 29, 2018 at 08:44:47PM +0000, Nadav Amit wrote:
> > at 1:13 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > 
> > > On Wed, Aug 29, 2018 at 07:36:22PM +0000, Nadav Amit wrote:
> > >> at 10:11 AM, Nadav Amit <namit@vmware.com> wrote:
> > >> 
> > >>> at 1:59 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >>> 
> > >>>> On Wed, 29 Aug 2018 01:11:42 -0700
> > >>>> Nadav Amit <namit@vmware.com> wrote:
> > >>>> 
> > >>>>> Use lockdep to ensure that text_mutex is taken when text_poke() is
> > >>>>> called.
> > >>>>> 
> > >>>>> Actually it is not always taken, specifically when it is called by kgdb,
> > >>>>> so take the lock in these cases.
> > >>>> 
> > >>>> Can we really take a mutex in kgdb context?
> > >>>> 
> > >>>> kgdb_arch_remove_breakpoint
> > >>>> <- dbg_deactivate_sw_breakpoints
> > >>>>  <- kgdb_reenter_check
> > >>>>     <- kgdb_handle_exception
> > >>>>        <- __kgdb_notify
> > >>>>          <- kgdb_ll_trap
> > >>>>            <- do_int3
> > >>>>          <- kgdb_notify
> > >>>>            <- die notifier
> > >>>> 
> > >>>> kgdb_arch_set_breakpoint
> > >>>> <- dbg_activate_sw_breakpoints
> > >>>>  <- kgdb_reenter_check
> > >>>>     <- kgdb_handle_exception
> > >>>>         ...
> > >>>> 
> > >>>> Both seems called in exception context, so we can not take a mutex lock.
> > >>>> I think kgdb needs a special path.
> > >>> 
> > >>> You are correct, but I don’t want a special path. Presumably text_mutex is
> > >>> guaranteed not to be taken according to the code.
> > >>> 
> > >>> So I guess the only concern is lockdep. Do you see any problem if I change
> > >>> mutex_lock() into mutex_trylock()? It should always succeed, and I can add a
> > >>> warning and a failure path if it fails for some reason.
> > >> 
> > >> Err.. This will not work. I think I will drop this patch, since I cannot
> > >> find a proper yet simple assertion. Creating special path just for the
> > >> assertion seems wrong.
> > > 
> > > It's probably worth expanding the comment for text_poke() to call out
> > > the kgdb case and reference kgdb_arch_{set,remove}_breakpoint(), whose
> > > code and comments make it explicitly clear why its safe for them to
> > > call text_poke() without acquiring the lock.  Might prevent someone
> > > from going down this path again in the future.
> > 
> > I thought that the whole point of the patch was to avoid comments, and
> > instead enforce the right behavior. I don’t understand well enough kgdb
> > code, so I cannot attest it does the right thing. What happens if
> > kgdb_do_roundup==0?
> 
> As is, the comment is wrong because there are obviously cases where
> text_poke() is called without text_mutex being held.  I can't attest
> to the kgdb code either.  My thought was to document the exception so
> that if someone does want to try and enforce the right behavior they
> can dive right into the problem instead of having to learn of the kgdb
> gotcha the hard way.  Maybe a FIXME is the right approach?

No, kgdb ensures that the text_mutex has not been held right before
calling text_poke. So they also take care the text_mutex. I guess
kgdb_arch_{set,remove}_breakpoint() is supposed to be run under
a special circumstance, like stopping all other threads/cores.
In that case, we can just check the text_mutex is not locked.
 
Anyway, kgdb is a very rare courner case. I think if CONFIG_KGDB is
enabled, lockdep and any assertion should be disabled, since kgdb
can tweak anything in the kernel with unexpected ways...

Thank you,
Nadav Amit Aug. 30, 2018, 5:23 a.m. UTC | #9
at 7:26 PM, Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Wed, 29 Aug 2018 14:00:06 -0700
> Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
>> On Wed, Aug 29, 2018 at 08:44:47PM +0000, Nadav Amit wrote:
>>> at 1:13 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>>> 
>>>> On Wed, Aug 29, 2018 at 07:36:22PM +0000, Nadav Amit wrote:
>>>>> at 10:11 AM, Nadav Amit <namit@vmware.com> wrote:
>>>>> 
>>>>>> at 1:59 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote:
>>>>>> 
>>>>>>> On Wed, 29 Aug 2018 01:11:42 -0700
>>>>>>> Nadav Amit <namit@vmware.com> wrote:
>>>>>>> 
>>>>>>>> Use lockdep to ensure that text_mutex is taken when text_poke() is
>>>>>>>> called.
>>>>>>>> 
>>>>>>>> Actually it is not always taken, specifically when it is called by kgdb,
>>>>>>>> so take the lock in these cases.
>>>>>>> 
>>>>>>> Can we really take a mutex in kgdb context?
>>>>>>> 
>>>>>>> kgdb_arch_remove_breakpoint
>>>>>>> <- dbg_deactivate_sw_breakpoints
>>>>>>> <- kgdb_reenter_check
>>>>>>>    <- kgdb_handle_exception
>>>>>>>       <- __kgdb_notify
>>>>>>>         <- kgdb_ll_trap
>>>>>>>           <- do_int3
>>>>>>>         <- kgdb_notify
>>>>>>>           <- die notifier
>>>>>>> 
>>>>>>> kgdb_arch_set_breakpoint
>>>>>>> <- dbg_activate_sw_breakpoints
>>>>>>> <- kgdb_reenter_check
>>>>>>>    <- kgdb_handle_exception
>>>>>>>        ...
>>>>>>> 
>>>>>>> Both seems called in exception context, so we can not take a mutex lock.
>>>>>>> I think kgdb needs a special path.
>>>>>> 
>>>>>> You are correct, but I don’t want a special path. Presumably text_mutex is
>>>>>> guaranteed not to be taken according to the code.
>>>>>> 
>>>>>> So I guess the only concern is lockdep. Do you see any problem if I change
>>>>>> mutex_lock() into mutex_trylock()? It should always succeed, and I can add a
>>>>>> warning and a failure path if it fails for some reason.
>>>>> 
>>>>> Err.. This will not work. I think I will drop this patch, since I cannot
>>>>> find a proper yet simple assertion. Creating special path just for the
>>>>> assertion seems wrong.
>>>> 
>>>> It's probably worth expanding the comment for text_poke() to call out
>>>> the kgdb case and reference kgdb_arch_{set,remove}_breakpoint(), whose
>>>> code and comments make it explicitly clear why its safe for them to
>>>> call text_poke() without acquiring the lock.  Might prevent someone
>>>> from going down this path again in the future.
>>> 
>>> I thought that the whole point of the patch was to avoid comments, and
>>> instead enforce the right behavior. I don’t understand well enough kgdb
>>> code, so I cannot attest it does the right thing. What happens if
>>> kgdb_do_roundup==0?
>> 
>> As is, the comment is wrong because there are obviously cases where
>> text_poke() is called without text_mutex being held.  I can't attest
>> to the kgdb code either.  My thought was to document the exception so
>> that if someone does want to try and enforce the right behavior they
>> can dive right into the problem instead of having to learn of the kgdb
>> gotcha the hard way.  Maybe a FIXME is the right approach?
> 
> No, kgdb ensures that the text_mutex has not been held right before
> calling text_poke. So they also take care the text_mutex. I guess
> kgdb_arch_{set,remove}_breakpoint() is supposed to be run under
> a special circumstance, like stopping all other threads/cores.
> In that case, we can just check the text_mutex is not locked.

I assumed so too, but after looking at the code, I am not sure that this is
the case when gdb_do_roundup==0.

> Anyway, kgdb is a very rare courner case. I think if CONFIG_KGDB is
> enabled, lockdep and any assertion should be disabled, since kgdb
> can tweak anything in the kernel with unexpected ways...

Call me lazy, but I really do not want to debug syzkaller failures due to
this issue (now or in the future). If the assertion is known to be
incorrect, even in a corner case, I see no reason to have it and I certainly
do not want to be the one that added it…

Patch
diff mbox series

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 014f214da581..916c11b410c4 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -699,6 +699,7 @@  void *text_poke(void *addr, const void *opcode, size_t len)
 	 * pages as they are not yet initialized.
 	 */
 	BUG_ON(!after_bootmem);
+	lockdep_assert_held(&text_mutex);
 
 	if (!core_kernel_text((unsigned long)addr)) {
 		pages[0] = vmalloc_to_page(addr);
diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index 8e36f249646e..60b99c76086c 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -768,8 +768,12 @@  int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
 	 */
 	if (mutex_is_locked(&text_mutex))
 		return -EBUSY;
+
+	/* Take the mutex to avoid lockdep assertion failures. */
+	mutex_lock(&text_mutex);
 	text_poke((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr,
 		  BREAK_INSTR_SIZE);
+	mutex_unlock(&text_mutex);
 	err = probe_kernel_read(opc, (char *)bpt->bpt_addr, BREAK_INSTR_SIZE);
 	if (err)
 		return err;
@@ -793,7 +797,12 @@  int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
 	 */
 	if (mutex_is_locked(&text_mutex))
 		goto knl_write;
+
+	/* Take the mutex to avoid lockdep assertion failures. */
+	mutex_lock(&text_mutex);
 	text_poke((void *)bpt->bpt_addr, bpt->saved_instr, BREAK_INSTR_SIZE);
+	mutex_unlock(&text_mutex);
+
 	err = probe_kernel_read(opc, (char *)bpt->bpt_addr, BREAK_INSTR_SIZE);
 	if (err || memcmp(opc, bpt->saved_instr, BREAK_INSTR_SIZE))
 		goto knl_write;