Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 11 Jun 2018 13:41:21 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     Miroslav Benes <mbenes@suse.cz>
Cc:     jikos@kernel.org, jpoimboe@redhat.com, jeyu@kernel.org,
        live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] livepatch: Deny the patched modules to be removed
Message-ID: <20180611114121.2u7wt6d6xu2cm6it@pathway.suse.cz>
References: <20180607092949.1706-1-mbenes@suse.cz>
 <20180607092949.1706-3-mbenes@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180607092949.1706-3-mbenes@suse.cz>
User-Agent: NeoMutt/20170421 (1.8.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu 2018-06-07 11:29:48, Miroslav Benes wrote:
> We decided to deny the patched modules to be removed. If it proves to be
> a major drawback for users, we can still implement a different approach.
> 
> The reference of a patched module has to be taken regardless of a
> patch's state. Thus it is not taken and dropped in enable/disable paths,
> but in register/unregister paths.

> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -739,6 +746,14 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>  		}
>  	}
>  
> +	/*
> +	 * Do not allow patched modules to be removed.
> +	 *
> +	 * All callers of klp_init_object_loaded() set obj->mod.
> +	 */
> +	if (klp_is_module(obj) && !try_module_get(obj->mod))
> +		return -ENODEV;
> +
>  	return 0;
>  }

This looks strange. We try to take the reference after we do all
actions needed for a loaded module. There is nothing that would
prevent obj->mod to get into the going state.

Note that it was safe (except for the relocated symbols) because
the module was not able to really go until it called
klp_module_going(). But you removed this last break
by the 3rd patch.


I suggest another approach:

I would rename klp_find_object_module() to klp_get_object_module()
and get the reference there. Note that it should be fine to call
it later in klp_init_object() (before klp_init_object_loaded()).
This would solve klp_init_patch().

We will also need to get the reference in klp_module_coming().
It can be called anywhere there. If it fails, we will block
loading the module.


Note that the mod->klp_alive logic will still be necessary.
It solves various races when both the patch and the patched
module are loaded in parallel.

Sigh, this also means that we still could call klp_init_object_loaded()
and apply reloacations even when the patched module fails to loaded
later from other reasons. This means that this approach does not
solve the original problem completely.

I wonder how complicated would be the arch-specific part that
would clean up relocations when the module is unloaded.

Best Regards,
Petr