Re: [PATCH 1/4] livepatch/shadow: Separate code to get or use pre-allocated shadow variable

[Petr Mladek <pmladek@suse.com>]

On Fri 2022-07-01 16:48:14, Marcos Paulo de Souza wrote:
> From: Petr Mladek <pmladek@suse.com>
> 
> Separate code that is used in klp_shadow_get_or_alloc() under klp_mutex.
> It splits a long spaghetti function into two. Also it unifies the error
> handling. The old used a mix of duplicated code, direct returns,
> and goto. The new code has only one unlock, free, and return calls.
> 
> Background: The change was needed by an earlier variant of the code adding
> 	garbage collection of shadow variables. It is not needed in
> 	the end. But the change still looks like an useful clean up.

Maybe the above paragraph is superfluous. I would remove it after all.

> It is code refactoring without any functional changes.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>

(It is fun to review my own RFC patch).

> ---
>  kernel/livepatch/shadow.c | 77 ++++++++++++++++++++++-----------------
>  1 file changed, 43 insertions(+), 34 deletions(-)
> 
> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> index c2e724d97ddf..67c1313f6831 100644
> --- a/kernel/livepatch/shadow.c
> +++ b/kernel/livepatch/shadow.c
> @@ -101,41 +101,19 @@ void *klp_shadow_get(void *obj, unsigned long id)
>  }
>  EXPORT_SYMBOL_GPL(klp_shadow_get);
>  
> -static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
> -				       size_t size, gfp_t gfp_flags,
> -				       klp_shadow_ctor_t ctor, void *ctor_data,
> -				       bool warn_on_exist)
> +static void *__klp_shadow_get_or_use(void *obj, unsigned long id,
> +				     struct klp_shadow *new_shadow,
> +				     klp_shadow_ctor_t ctor, void *ctor_data,
> +				     bool *exist)

I have to admit that the name is a bit misleading. The real meaning
is "get existing" or "use newly allocated one".

I think that the following might better describe the real meaning
in the context where it is used:

     __klp_shadow_get_or_alloc_locked()
     __klp_shadow_get_or_add()

Anyway, it would deserve some comment above the function definition.
For example:

/*
 * Check if the variable already exists. Otherwise, add
 * the pre-allocated one.
 */

>  {
> -	struct klp_shadow *new_shadow;
>  	void *shadow_data;
> -	unsigned long flags;

It would deserve:

	lockdep_assert_held(&klp_shadow_lock);

> -
> -	/* Check if the shadow variable already exists */
> -	shadow_data = klp_shadow_get(obj, id);
> -	if (shadow_data)
> -		goto exists;
> -

Best Regards,
Petr