Re: [PATCH] livepatch: Fix races between stack check and module load/unload

[Petr Mladek <pmladek@suse.com>]

On Wed 2026-10-14 14:23:50, Miroslav Benes wrote:
> Hi Andrea,
> 
> Cc+ Petr Pavlu (you can find the whole thread at 
> https://lore.kernel.org/live-patching/20261012150758.207983-1-parri.andrea@gmail.com/T/#t)
> 
> On Mon, 12 Oct 2026, Andrea Parri wrote:
> 
> > Unloading a patched module while a livepatch transition is in progress
> > can crash the kernel in klp_find_ops(), called from the scheduler.
> > KCSAN reports
> > 
> >   BUG: KCSAN: data-race in klp_find_ops / klp_unpatch_func
> > 
> >   write (marked) to 0xffff9094c09ece10 of 8 bytes by task 381 on cpu 2:
> >    klp_unpatch_func+0x1bd/0x280
> >    klp_unpatch_object+0x6a/0x90
> >    klp_cleanup_module_patches_limited.cold+0x4a/0xd4
> >    klp_module_going+0x54/0x80
> >    __do_sys_delete_module+0x247/0x380
> >    __x64_sys_delete_module+0x39/0x50
> >    ...
> > 
> >   read to 0xffff9094c09ece10 of 8 bytes by task 146 on cpu 1:
> >    klp_find_ops+0x55/0x90
> >    klp_check_and_switch_task+0x228/0x320
> >    klp_try_switch_task+0xb4/0xc0
> >    __klp_sched_try_switch+0x9a/0xb0
> >    __schedule+0x60a/0x1450
> >    schedule+0x3b/0x100
> >    schedule_timeout+0xc0/0x160
> >    ra_target_a_loop+0x97/0xa0 [klp_race_target_a]> >    ...
> > 
> > followed by
> > 
> >   Oops: general protection fault, probably for non-canonical address
> >   0xdead000000000110: 0000 [#2] SMP NOPTI
> >   RIP: 0010:klp_find_ops+0x55/0x90
> >    ...
> >    klp_check_and_switch_task+0x228/0x320
> >    klp_try_switch_task+0xb4/0xc0
> >    __klp_sched_try_switch+0x9a/0xb0
> >    __schedule+0x60a/0x1450
> 
> Interesting. So to sum it up... you load a live patch which contains a fix 
> for a module and while the transition is on, you remove the module and it 
> crashes (btw, I do not think that those traces are necessary here. More 
> btw, it would be nice to have a human readable changelog instead of what 
> looks like AI generated description of the patch itself).
> 
> How did you find the issue? Is it a real problem in production?
> 
> I wonder whether we could just decline delete_module() in this case and 
> return -EBUSY. It should be a rare operation anyway. Alternatively we 
> could increase refcounts of all patched modules during a transition and 
> decrease them after. Petr P., what do you think?
> 
> I am not saying we should not do the RCU protection but it feels like a 
> can of worms a bit so if there is an easier way...

I agree that it is a can of worms. This patch is not super complicated.
It seems that it can't break anything. The bigger problem is whether
it solves all possible racy accesses. And how to prevent regressions
in the future.

Preventing the module unload during a livepatch transition might
be one solution. But I am afraid that people might want a better solution
sooner or later because the transition might never finish, ...

I wonder if we could use some more strict locking for the two
operations, e.g. (raw_)spin_lock. And then use

		if (!raw_spin_trylock(&klp_unpatch_object_lock))
			return -EBUSY;

in klp_check_stack(). Plus take the lock in __klp_unpatch_object().

Hmm, we could not take the spin_lock around klp_patch_object()
because it does allocations and might be running for a long time
when there are many patches objects. But this might be avoided by the
smp_store_release(&obj->patched, true) trick.

It still might be conceptually easier than making all the
accesses RCU-safe.

What do you think, please?

Best Regards,
Petr