module_put() is not called for a patch with "forced" flag. It should
block the removal of the livepatch module when the code might still
be in use after forced transition.
klp_force_transition() currently sets "forced" flag for all patches on
the list.
In fact, any patch can be safely unloaded when it passed through
the consistency model in KLP_UNPATCHED transition.
By other words, the "forced" flag must be set only for livepatches
that are being removed. In particular, set the "forced" flag:
+ only for klp_transition_patch when the transition to KLP_UNPATCHED
state was forced.
+ all replaced patches when the transition to KLP_PATCHED state was
forced and the patch was replacing the existing patches.
Signed-off-by: Chengming Zhou <[email protected]>
---
Changes in v3:
- rewrite more clear commit message by Petr.
Changes in v2:
- interact nicely with the atomic replace feature noted by Miroslav.
---
kernel/livepatch/transition.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 5683ac0d2566..7f25a5ae89f6 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -641,6 +641,18 @@ void klp_force_transition(void)
for_each_possible_cpu(cpu)
klp_update_patch_state(idle_task(cpu));
- klp_for_each_patch(patch)
- patch->forced = true;
+ /*
+ * Only need to set forced flag for the transition patch
+ * when force transition to KLP_UNPATCHED state, but
+ * have to set forced flag for all replaced patches
+ * when force atomic replace transition.
+ */
+ if (klp_target_state == KLP_UNPATCHED)
+ klp_transition_patch->forced = true;
+ else if (klp_transition_patch->replace) {
+ klp_for_each_patch(patch) {
+ if (patch != klp_transition_patch)
+ patch->forced = true;
+ }
+ }
}
--
2.20.1
On 3/16/22 11:03 AM, Miroslav Benes wrote:
>>>> + /*
>>>> + * Only need to set forced flag for the transition patch
>>>> + * when force transition to KLP_UNPATCHED state, but
>>>> + * have to set forced flag for all replaced patches
>>>> + * when force atomic replace transition.
>>>> + */
>>>
>>> How about something like
>>>
>>> /*
>>> * Set forced flag for patches being removed, which is the transition
>>> * patch in KLP_UNPATCHED state or all replaced patches when forcing
>>> * the atomic replace transition.
>>> */
>>
>> Or just the first sentence:
>>
>> /* Set forced flag for patches being removed */
>>
>> The rest is visible from the code.
>
> True. This would work for me as well.
>
Sorry for not following this one more closely as we don't use force nor
atomic replace patches (yet) ... but the code and use case seems clear
enough for the shorter comment.
Acked-by: Joe Lawrence <[email protected]>
--
Joe
> > > + /*
> > > + * Only need to set forced flag for the transition patch
> > > + * when force transition to KLP_UNPATCHED state, but
> > > + * have to set forced flag for all replaced patches
> > > + * when force atomic replace transition.
> > > + */
> >
> > How about something like
> >
> > /*
> > * Set forced flag for patches being removed, which is the transition
> > * patch in KLP_UNPATCHED state or all replaced patches when forcing
> > * the atomic replace transition.
> > */
>
> Or just the first sentence:
>
> /* Set forced flag for patches being removed */
>
> The rest is visible from the code.
True. This would work for me as well.
Miroslav
On Sat 2022-03-12 23:22:20, Chengming Zhou wrote:
> module_put() is not called for a patch with "forced" flag. It should
> block the removal of the livepatch module when the code might still
> be in use after forced transition.
>
> klp_force_transition() currently sets "forced" flag for all patches on
> the list.
>
> In fact, any patch can be safely unloaded when it passed through
> the consistency model in KLP_UNPATCHED transition.
>
> By other words, the "forced" flag must be set only for livepatches
> that are being removed. In particular, set the "forced" flag:
>
> + only for klp_transition_patch when the transition to KLP_UNPATCHED
> state was forced.
>
> + all replaced patches when the transition to KLP_PATCHED state was
> forced and the patch was replacing the existing patches.
>
> Signed-off-by: Chengming Zhou <[email protected]>
Looks reasonable, passes livepatch tests:
Reviewed-by: Petr Mladek <[email protected]>
Tested-by: Petr Mladek <[email protected]>
Just let me repeat. The "force" flags must be used carefully because
it breaks the consistency model.
Best Regards,
Petr
On Wed 2022-03-16 15:48:25, Miroslav Benes wrote:
> On Sat, 12 Mar 2022, Chengming Zhou wrote:
>
> > module_put() is not called for a patch with "forced" flag. It should
> > block the removal of the livepatch module when the code might still
> > be in use after forced transition.
> >
> > klp_force_transition() currently sets "forced" flag for all patches on
> > the list.
> >
> > In fact, any patch can be safely unloaded when it passed through
> > the consistency model in KLP_UNPATCHED transition.
> >
> > By other words, the "forced" flag must be set only for livepatches
>
> s/By/In/
>
> > that are being removed. In particular, set the "forced" flag:
> >
> > + only for klp_transition_patch when the transition to KLP_UNPATCHED
> > state was forced.
> >
> > + all replaced patches when the transition to KLP_PATCHED state was
> > forced and the patch was replacing the existing patches.
> >
> > index 5683ac0d2566..7f25a5ae89f6 100644
> > --- a/kernel/livepatch/transition.c
> > +++ b/kernel/livepatch/transition.c
> > @@ -641,6 +641,18 @@ void klp_force_transition(void)
> > for_each_possible_cpu(cpu)
> > klp_update_patch_state(idle_task(cpu));
> >
> > - klp_for_each_patch(patch)
> > - patch->forced = true;
> > + /*
> > + * Only need to set forced flag for the transition patch
> > + * when force transition to KLP_UNPATCHED state, but
> > + * have to set forced flag for all replaced patches
> > + * when force atomic replace transition.
> > + */
>
> How about something like
>
> /*
> * Set forced flag for patches being removed, which is the transition
> * patch in KLP_UNPATCHED state or all replaced patches when forcing
> * the atomic replace transition.
> */
Or just the first sentence:
/* Set forced flag for patches being removed */
The rest is visible from the code.
Either version works for me. If we agree on it then I update
the text when pushing the patch.
Best Regards,
Petr
On Sat, 12 Mar 2022, Chengming Zhou wrote:
> module_put() is not called for a patch with "forced" flag. It should
> block the removal of the livepatch module when the code might still
> be in use after forced transition.
>
> klp_force_transition() currently sets "forced" flag for all patches on
> the list.
>
> In fact, any patch can be safely unloaded when it passed through
> the consistency model in KLP_UNPATCHED transition.
>
> By other words, the "forced" flag must be set only for livepatches
s/By/In/
> that are being removed. In particular, set the "forced" flag:
>
> + only for klp_transition_patch when the transition to KLP_UNPATCHED
> state was forced.
>
> + all replaced patches when the transition to KLP_PATCHED state was
> forced and the patch was replacing the existing patches.
>
> Signed-off-by: Chengming Zhou <[email protected]>
> ---
> Changes in v3:
> - rewrite more clear commit message by Petr.
>
> Changes in v2:
> - interact nicely with the atomic replace feature noted by Miroslav.
> ---
> kernel/livepatch/transition.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 5683ac0d2566..7f25a5ae89f6 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -641,6 +641,18 @@ void klp_force_transition(void)
> for_each_possible_cpu(cpu)
> klp_update_patch_state(idle_task(cpu));
>
> - klp_for_each_patch(patch)
> - patch->forced = true;
> + /*
> + * Only need to set forced flag for the transition patch
> + * when force transition to KLP_UNPATCHED state, but
> + * have to set forced flag for all replaced patches
> + * when force atomic replace transition.
> + */
How about something like
/*
* Set forced flag for patches being removed, which is the transition
* patch in KLP_UNPATCHED state or all replaced patches when forcing
* the atomic replace transition.
*/
?
> + if (klp_target_state == KLP_UNPATCHED)
> + klp_transition_patch->forced = true;
> + else if (klp_transition_patch->replace) {
> + klp_for_each_patch(patch) {
> + if (patch != klp_transition_patch)
> + patch->forced = true;
> + }
> + }
Looks good to me.
Miroslav
On Sat 2022-03-12 23:22:20, Chengming Zhou wrote:
> module_put() is not called for a patch with "forced" flag. It should
> block the removal of the livepatch module when the code might still
> be in use after forced transition.
>
> klp_force_transition() currently sets "forced" flag for all patches on
> the list.
>
> In fact, any patch can be safely unloaded when it passed through
> the consistency model in KLP_UNPATCHED transition.
>
> By other words, the "forced" flag must be set only for livepatches
> that are being removed. In particular, set the "forced" flag:
>
> + only for klp_transition_patch when the transition to KLP_UNPATCHED
> state was forced.
>
> + all replaced patches when the transition to KLP_PATCHED state was
> forced and the patch was replacing the existing patches.
>
> Signed-off-by: Chengming Zhou <[email protected]>
The patch has been committed, with the proposed wording changes,
into livepatching/livepatching.git, branch for-5.18/fixes,
see
https://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching.git/commit/?h=for-5.18/fixes&id=2957308343fa7c621df9f342fab88cb970b8d5f3
Best Regards,
Petr