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
