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

[Miroslav Benes <mbenes@suse.cz>]

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...
  
>  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