Re: [PATCH] livepatch: fix race between enabled_store() and klp_unregister_patch()

[Petr Mladek <pmladek@suse.com>]

On Mon 2015-11-30 11:54:37, Li Bin wrote:
> There is a potential race as following:
> 
> CPU0                         |  CPU1
> -----------------------------|-----------------------------------
> enabled_store()              |  klp_unregister_patch()
>                              |  |-mutex_lock(&klp_mutex);
> |-mutex_lock(&klp_mutex);    |  |-klp_free_patch();
>                              |  |-mutex_unlock(&klp_mutex);
> |-[process the patch's state]|
> |-mutex_unlock(&klp_mutex)   |
> 
> Fix this race condition by adding klp_is_patch_registered() check in
> enabled_store() after get the lock klp_mutex.

It seems that you are right but the situation is more complicated.
See below.

> 
> Signed-off-by: Li Bin <huawei.libin@huawei.com>
> ---
>  kernel/livepatch/core.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index db545cb..50af971 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -614,6 +614,11 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
>  
>  	mutex_lock(&klp_mutex);
>  
> +	if (!klp_is_patch_registered(patch)) {

How is it guaranteed that "patch" is still a valid pointer, please?

I think that we also need to modify klp_unregister_patch().
It must not finish until the patch->kobj is destroyed.
Otherwise, the patch module would get removed and
the "patch" would be invalid.

I guess that enabled_store() takes reference of the patch->kobj.
Then kobject_put(&patch->kobj) in klp_free_patch() decrements
the refcount but it does not free the object. It means that
it does not wait for  enabled_store() to finish. I am not 100%
sure because the kobject/sysfs code is quite complex.


Anyway, we should refuse to call klp_unregister_patch() at all
in the current implementation. It is not safe because we
do not know if the patch is still used or not.

Note that if you disable the patch, you are only sure that
the new function will not longer be called. But you
do _not_ know if all the existing calls have already finished.
A process might sleep inside the function from the patch.
I guess that it will be possible after we introduce
a more complex consistency algorithm for switching
the patches.

Best Regards,
Petr



> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
>  	if (val == patch->state) {
>  		/* already in requested state */
>  		ret = -EINVAL;
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html