On Thu 2024-07-25 13:48:06, Miroslav Benes wrote:
> 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...
Right.
> > 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?
No, you are perfectly right.
> We can even have both ways implemented to demonstrate different
> approaches...
I have implemented only your approach ;-)
That said, I am going to keep the callback so that the selftest could
check that it is called at the right time. But the callback will
only print the message. And a comment would explain that is not
really needed.
Also I am going to add a .state_dtor callback so that we could test
the shadow variable is freed. The callback will only print a message.
It is a simple shadow variable and the memory is freed automatically
together with the struct klp_shadow.
Best Regards,
Petr
