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

Petr Mladek
2015-12-01T16:57:10Z

On Tue 2015-12-01 15:28:19, Jiri Slaby wrote:
> On 12/01/2015, 03:13 PM, Petr Mladek wrote:
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -612,7 +612,19 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> >
> > patch = container_of(kobj, struct klp_patch, kobj);
> >
> > - mutex_lock(&klp_mutex);
> > + /*
> > + * Avoid a deadlock with kobject_put(&patch->kobj) that is
> > + * called under klp_mutex. Bail out when the patch is not
> > + * longer registered.
> > + */
> > + if (!mutex_trylock(&klp_mutex)) {
>
> This introduces false positives.
> Deleting/enabling/disabling/other_op_under_klp_mutex of an unrelated
> patch may now cause enabled_store to fail. Hence I don't like this
> approach at all.

Ah, there should have been

while (!mutex_trylock(&klp_mutex)) {
if (!klp_is_patch_registered(patch))
return -EINVAL;
/* Do not spin with trylock that bounce cache lines. */
while (mutex_is_locked(&klp_mutex) &&
klp_is_patch_registered(patch))
cond_resched();
}

, so it should not produce false positives.

But I do not have a strong opinion about it.

Best Regards,
Petr