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

[Andrea Parri <parri.andrea@gmail.com>]

Hi Petr,

On Wed, Oct 14, 2026 at 04:30:03PM +0200, Petr Mladek wrote:
> 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().

I like this.  With every change the stack check can see made under one
lock, it sees a consistent state, and the READ_ONCE()s and NULL checks
in v1 go away.

I don't think the lock can cover all of __klp_unpatch_object() though:
klp_unpatch_func() calls unregister_ftrace_function() and
ftrace_set_filter_ip(), which sleep.  So I'd take it in
klp_unpatch_func() only around the list_del()s, after the ftrace calls,
and free the ops after dropping it.  obj->patched would be cleared
under the lock as well, before the funcs are removed, so that the check
skips the object from then on.

> 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.

The store-release covers the objects of the transition patch, but
klp_find_ops() walks the global klp_ops list, and klp_patch_func() adds
to it when a module patched by any patch is loaded.  I'd take the lock
in klp_patch_func() just around the list_add()s, once the ops is
allocated and its func_stack set up, and around the list_del()s in the
error path.  That should be short enough.  obj->patched can then be set
under the lock too, after all the funcs are added, so the store-release
is not needed.

klp_find_ops() could then assert that klp_mutex or the new lock is held,
which also takes care of Peter's comment.

One more detail: klp_try_switch_task() reports -EBUSY as "is running",
so a failed trylock would want a different error code or message.

The trylock also fails when another CPU is checking a stack at the same
time, so concurrent checks are serialized and the loser retries the
task later.  The lock would only be held for the walk, not while saving
the stack trace, so I expect this to be fine, but it might slow down a
transition on a large machine.  Would that be acceptable?

If this matches what you had in mind, I'll send v2 along these lines.

Thanks,
  Andrea