Messages in this thread | | | Date | Tue, 1 Dec 2015 17:57:03 +0100 | From | Petr Mladek <> | Subject | Re: [PATCH] livepatch: fix race between enabled_store() and klp_unregister_patch() |
| |
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
| |