Hi,
On Fri, 10 Nov 2023, Petr Mladek wrote:
> Recent changes in the livepatch core have allowed to connect states,
> shadow variables, and callbacks. Use these new features in
> the state tests.
>
> Use the shadow variable API to store the original loglevel. It is
> better suited for this purpose than directly accessing the .data
> pointer in state klp_state.
>
> Another big advantage is that the shadow variable is preserved
> when the current patch is replaced by a new version. As a result,
> there is not need to copy the pointer.
>
> Finally, the lifetime of the shadow variable is connected with
> the lifetime of the state. It is freed automatically when
> it is not longer supported.
>
> This results into the following changes in the code:
>
> + Rename CONSOLE_LOGLEVEL_STATE -> CONSOLE_LOGLEVEL_FIX_ID
> because it will be used also the for shadow variable
>
> + Remove the extra code for module coming and going states
> because the new callback are per-state.
>
> + Remove callbacks needed to transfer the pointer between
> states.
>
> + Keep the versioning of the state to prevent downgrade.
> The problem is artificial because no callbacks are
> needed to transfer or free the shadow variable anymore.
>
> Signed-off-by: Petr Mladek <pmladek@xxxxxxxx>
it is much cleaner now.
[...]
> static int allocate_loglevel_state(void)
> {
> - struct klp_state *loglevel_state;
> + int *shadow_console_loglevel;
>
> - loglevel_state = klp_get_state(&patch, CONSOLE_LOGLEVEL_STATE);
> - if (!loglevel_state)
> - return -EINVAL;
> + /* Make sure that the shadow variable does not exist yet. */
> + shadow_console_loglevel =
> + klp_shadow_alloc(&console_loglevel, CONSOLE_LOGLEVEL_FIX_ID,
> + sizeof(*shadow_console_loglevel), GFP_KERNEL,
> + NULL, NULL);
>
> - loglevel_state->data = kzalloc(sizeof(console_loglevel), GFP_KERNEL);
> - if (!loglevel_state->data)
> + if (!shadow_console_loglevel) {
> + pr_err("%s: failed to allocated shadow variable for storing original loglevel\n",
> + __func__);
> return -ENOMEM;
> + }
>
> pr_info("%s: allocating space to store console_loglevel\n",
> __func__);
> +
> return 0;
> }
Would it make sense to set is_shadow to 1 here? I mean you would pass
klp_state down to allocate_loglevel_state() from setup callback and set
its is_shadow member here. Because then...
> static void free_loglevel_state(void)
> {
> - struct klp_state *loglevel_state;
> + int *shadow_console_loglevel;
>
> - loglevel_state = klp_get_state(&patch, CONSOLE_LOGLEVEL_STATE);
> - if (!loglevel_state)
> + shadow_console_loglevel =
> + (int *)klp_shadow_get(&console_loglevel, CONSOLE_LOGLEVEL_FIX_ID);
> + if (!shadow_console_loglevel)
> return;
>
> pr_info("%s: freeing space for the stored console_loglevel\n",
> __func__);
> - kfree(loglevel_state->data);
> + klp_shadow_free(&console_loglevel, CONSOLE_LOGLEVEL_FIX_ID, NULL);
> }
would not be needed. And release callback neither.
Or am I wrong?
We can even have both ways implemented to demonstrate different
approaches...
Miroslav
