Hi,
On Mon, Jun 29, 2020 at 10:15 AM Daniel Thompson
<daniel.thomp...@linaro.org> 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 <daniel.thomp...@linaro.org>
> ---
> 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:
kgdb_register_callbacks()
-> 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):
----------------- ----------------------
kgdb_unregister_callbacks()
spin_lock() <got>
spin_lock() <blocked>
if (old_dbg_io_ops) <true>
if (has dinit) <false>
print error
spin_unlock()
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):
----------------- ----------------------
kgdb_unregister_callbacks()
spin_lock() <got>
spin_lock() <blocked>
if (old_dbg_io_ops) <true>
if (has dinit) <true>
print Replacing
init new IO ops
spin_unlock()
if (old_dbg_io_ops) <true>
finish deinit of old
return true
WARN_ON() <hits and shouts!>
dbg_io_ops = NULL
spin_unlock()
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.
-Doug
_______________________________________________
Kgdb-bugreport mailing list
Kgdb-bugreport@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
