Hi Tao,
Thank you for the review and comments.
The TRANSITION mark indicating a live patch is activating or
deactivating or either? I mean, when people see the TRANSITION flag,
do they know the exact operation that is on the system, like it is
patching, or unpatching, or it doesn't distinguish the operation at
all?
[TRANSITION] now explicitly means “a livepatch transition is in
progress,” without distinguishing enable vs. disable. The intent is to
surface that the system is in a transient state.
Your above code is 1) check if transition_patch is null. 2) check if
transition_patch is within the klp_patches list.
But I see in the kernel source, there are only lists to be added into
the klp_patches list, no one is to be deleted [1]. So my assumption
is, transition_patch will always be found within klp_patches. Because
for all livepatches, they must go through klp_enable_patch() ->
klp_init_patch() -> list_add_tail(&patch->list, &klp_patches), and no
one to be deleted from klp_patches afterwards, so transition_patch
must be one of those. If my assumption is correct, is step 2)
necessary?
Step 2 is still necessary.
A patch can be removed from the klp_patches list by
klp_free_patch_async(), which can be reached via the path:
enabled_store() -> __klp_disable_patch().
Although it is unlikely that klp_transition_patch is not in klp_patches,
this is not guaranteed, so it is safer to keep the additional
list membership check.
And also for the list iteration in step 2), I suggest using the
do_list() function of crash utility, you can refer to
memory.c:dump_vmap_area() to get an example of do_list() usage.
That's a good idea! I agree with you.
I wasn’t aware of this helper in the crash utility, so I will update the code
accordingly and send a v2 patch.
Best regards,
Motomasa Suzuki