[lkml]   [2020]   [Jun]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] kgdb: Resolve races during kgdb_io_register/unregister_module

On Mon, Jun 29, 2020 at 10:15 AM Daniel Thompson
<> wrote:
> Currently kgdb_register_callbacks() and kgdb_unregister_callbacks()
> are called outside the scope of the kgdb_registration_lock. This
> allows them to race with each other. This could do all sorts of crazy
> things up to and including dbg_io_ops becoming NULL partway through the
> execution of the kgdb trap handler (which isn't allowed and would be
> fatal).
> Fix this by bringing the trap handler setup and teardown into the scope
> of the registration lock.
> Signed-off-by: Daniel Thompson <>
> ---
> kernel/debug/debug_core.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index 9e5934780f41..9799f2c6dc94 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -1117,9 +1117,8 @@ int kgdb_register_io_module(struct kgdb_io *new_dbg_io_ops)
> dbg_io_ops = new_dbg_io_ops;
> - spin_unlock(&kgdb_registration_lock);
> -
> if (old_dbg_io_ops) {
> + spin_unlock(&kgdb_registration_lock);
> old_dbg_io_ops->deinit();
> return 0;
> }
> @@ -1129,6 +1128,8 @@ int kgdb_register_io_module(struct kgdb_io *new_dbg_io_ops)
> /* Arm KGDB now. */
> kgdb_register_callbacks();
> + spin_unlock(&kgdb_registration_lock);

From looking at code paths, I think this is illegal, isn't it? You're
now calling kgdb_register_callbacks() while holding a spinlock, but:

-> register_console()
--> console_lock()
---> might_sleep()
----> <boom!>

I'm a little curious about the exact race we're trying to solve.
Calling unregister on an IO module before register even finished seems
like an error on the caller, so I guess it would be calling register
from a 2nd thread for a different IO module while the first thread was
partway through unregistering? Even that seems awfully sketchy since
you're risking registering a 2nd IO ops while the first is still there
and that's illegal enough that we do a pr_err() for it (though we
don't crash), but let's say we're trying to solve that one.

Looking at it closely, I _think_ the only race in this case is if the
one we're trying to unregister had a deinit() function and we going to
replace it? If it didn't have a deinit function:

cpu1 (unregister) cpu2 (register):
----------------- ----------------------
spin_lock() <got>
spin_lock() <blocked>
if (old_dbg_io_ops) <true>
if (has dinit) <false>
print error
return -EBUSY
<finish unregister>

The above is fine and is the same thing that would happen if the
whole register function ran before the unregister even started, right?

Also: if the unregister won the race that should also be fine.

So really the problem is this:

cpu1 (unregister) cpu2 (register):
----------------- ----------------------
spin_lock() <got>
spin_lock() <blocked>
if (old_dbg_io_ops) <true>
if (has dinit) <true>
print Replacing
init new IO ops
if (old_dbg_io_ops) <true>
finish deinit of old
return true
WARN_ON() <hits and shouts!>
dbg_io_ops = NULL
if (deinit) <true>
double-call to deinit of old

So in this case we'll hit a WARN_ON(), incorrectly unregister the new
IO ops, and call deinit twice.


 \ /
  Last update: 2020-06-29 23:04    [W:0.057 / U:11.276 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site