Messages in this thread | | | From | Doug Anderson <> | Date | Mon, 29 Jun 2020 14:03:52 -0700 | Subject | Re: [PATCH] kgdb: Resolve races during kgdb_io_register/unregister_module |
| |
Hi,
On Mon, Jun 29, 2020 at 10:15 AM Daniel Thompson <daniel.thompson@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.thompson@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
| |