Re: [POC 3/7] livepatch: Use per-state callbacks in state API tests

[Petr Mladek <pmladek@suse.com>]

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@suse.com>
> 
> 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