Re: [PATCH 3/4] livepatch/shadow: Introduce klp_shadow_type structure

[Petr Mladek <pmladek@suse.com>]

On Fri 2022-07-01 16:48:16, Marcos Paulo de Souza wrote:
> The shadow variable type will be used in klp_shadow_alloc/get/free
> functions instead of id/ctor/dtor parameters. As a result, all callers
> use the same callbacks consistently[*][**].
> 
> The structure will be used in the next patch that will manage the
> lifetime of shadow variables and execute garbage collection automatically.
> 
> [*] From the user POV, it might have been easier to pass $id instead
>     of pointer to struct klp_shadow_type.
>
>     The problem is that each livepatch registers its own struct
>     klp_shadow_type and defines its own @ctor/@dtor callbacks. It would
>     be unclear what callback should be used. They should be compatible.

This probably is not clear enough. The following might be better:

[*] From the user POV, it might have been easier to pass $id instead
    of pointer to struct klp_shadow_type.

    It would require registering the klp_shadow_type so that
    the klp_shadow API could find ctor/dtor for the given id.
    It actually will be needed for the garbage collection anyway
    because it will define the lifetime of the variables.

    The bigger problem is that the same klp_shadow_type might be
    used by more livepatch modules. Each livepatch module need
    to duplicate the definition of klp_shadow_type and ctor/dtor
    callbacks. The klp_shadow API would need to choose one registered
    copy.

    They definitions should be compatible and they should stay as long
    as the type is registered. But it still feels more safe when
    klp_shadow API callers use struct klp_shadow_type and ctor/dtor
    callbacks defined in the same livepatch module.

>     This problem is gone when each livepatch explicitly uses its
>     own struct klp_shadow_type pointing to its own callbacks.
> 
> [**] test_klp_shadow_vars.c uses a custom @dtor to show that it was called.
>     The message must be disabled when called via klp_shadow_free_all()
>     because the ordering of freed variables is not well defined there.
>     It has to be done using another hack after switching to
>     klp_shadow_types.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> Signed-off-by: Petr Mladek <pmladek@suse.com>

This should be:

Co-developed-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>

> --- a/kernel/livepatch/shadow.c
> +++ b/kernel/livepatch/shadow.c
> @@ -156,22 +154,25 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
>  	 * More complex setting can be done by @ctor function.  But it is
>  	 * called only when the buffer is really used (under klp_shadow_lock).
>  	 */
> -	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
> +	new_shadow = kzalloc(size + sizeof(struct klp_shadow), gfp_flags);
>  	if (!new_shadow)
>  		return NULL;
>  
>  	/* Look for <obj, id> again under the lock */
>  	spin_lock_irqsave(&klp_shadow_lock, flags);
> -	shadow_data = __klp_shadow_get_or_use(obj, id, new_shadow,
> -					      ctor, ctor_data, &exist);
> +	shadow_data = __klp_shadow_get_or_use(obj, shadow_type,
> +					      new_shadow, ctor_data, &exist);
>  	spin_unlock_irqrestore(&klp_shadow_lock, flags);
>  
> -	/* Throw away unused speculative allocation. */
> +	/*
> +	 * Throw away unused speculative allocation if the shadow variable
> +	 * exists or if the ctor function failed.
> +	 */

The ordering is confusing because it does not match the code. Please,
either use:

	 * Throw away the unused speculative allocation if ctor() failed
	 * or the variable already existed.

or just keep the original comment.

>  	if (!shadow_data || exist)
>  		kfree(new_shadow);
>  
>  	if (exist && warn_on_exist) {
> -		WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id);
> +		WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, shadow_type->id);
>  		return NULL;
>  	}

Best Regards,
Petr