On Wed, Feb 22, 2023 at 03:00:33PM +0100, Petr Mladek wrote: > > + /* All patching has stopped, now start the reverse transition. */ > > + klp_transition_patch->enabled = !klp_transition_patch->enabled; > > + klp_target_state = !klp_target_state; > > I have double checked the synchronization and we need here: > > /* > * Make sure klp_update_patch_state() and __klp_sched_try_switch() > * see the updated klp_target_state before TIF_PATCH_PENDING > * is set again in klp_start_transition(). > */ > smp_wmb(); > > The same is achieved by smp_wmb() in klp_init_transition(). > > Note that the extra barrier was missing here because klp_target_state > was set before klp_synchronize_transition(). It was fine because > klp_update_patch_state() was called on locations where a transition > in any direction was always safe. > > Just for record. We need to modify @klp_target_state after > klp_synchronize_transition() now. The value is used by > __klp_sched_try_switch() to decide when the transition > is safe. It defines what functions must not be on the stack. Yes, makes sense. And we need a corresponding smp_rmb() in __klp_sched_try_switch() before the call to klp_try_switch_task(), right? Something like this on top? Also updated a few more comments. diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index 201f0c0482fb..3f79265dd6e5 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -33,6 +33,7 @@ * * - klp_ftrace_handler() * - klp_update_patch_state() + * - __klp_sched_try_switch() */ DEFINE_MUTEX(klp_mutex); diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index b9e006632124..218ef4a5d575 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -192,8 +192,8 @@ void klp_update_patch_state(struct task_struct *task) * barrier (smp_rmb) for two cases: * * 1) Enforce the order of the TIF_PATCH_PENDING read and the - * klp_target_state read. The corresponding write barrier is in - * klp_init_transition(). + * klp_target_state read. The corresponding write barriers are in + * klp_init_transition() and klp_reverse_transition(). * * 2) Enforce the order of the TIF_PATCH_PENDING read and a future read * of func->transition, if klp_ftrace_handler() is called later on @@ -381,6 +381,14 @@ void __klp_sched_try_switch(void) if (unlikely(!klp_patch_pending(current))) goto out; + /* + * Enforce the order of the TIF_PATCH_PENDING read above and the + * klp_target_state read in klp_try_switch_task(). The corresponding + * write barriers are in klp_init_transition() and + * klp_reverse_transition(). + */ + smp_rmb(); + klp_try_switch_task(current); out: @@ -604,8 +612,9 @@ void klp_init_transition(struct klp_patch *patch, int state) * see a func in transition with a task->patch_state of KLP_UNDEFINED. * * Also enforce the order of the klp_target_state write and future - * TIF_PATCH_PENDING writes to ensure klp_update_patch_state() doesn't - * set a task->patch_state to KLP_UNDEFINED. + * TIF_PATCH_PENDING writes to ensure klp_update_patch_state() and + * __klp_sched_try_switch() don't set a task->patch_state to + * KLP_UNDEFINED. */ smp_wmb(); @@ -661,9 +670,19 @@ void klp_reverse_transition(void) */ klp_synchronize_transition(); - /* All patching has stopped, now start the reverse transition. */ + /* All patching has stopped, now start the reverse transition: */ + klp_transition_patch->enabled = !klp_transition_patch->enabled; klp_target_state = !klp_target_state; + + /* + * Enforce the order of the klp_target_state write and the + * TIF_PATCH_PENDING writes in klp_start_transition() to ensure + * klp_update_patch_state() and __klp_sched_try_switch() don't set + * task->patch_state to the wrong value. + */ + smp_wmb(); + klp_start_transition(); }
