On Wed 2018-12-05 14:02:20, Joe Lawrence wrote:
> On Thu, Nov 29, 2018 at 10:44:23AM +0100, Petr Mladek wrote:
> > The code for freeing livepatch structures is a bit scattered and tricky:
> >
> > [ ... snip ... ]
> >
> > +static int klp_init_patch(struct klp_patch *patch)
> > +{
> > + struct klp_object *obj;
> > + int ret;
> > +
> > + mutex_lock(&klp_mutex);
> > +
> > + ret = klp_init_patch_before_free(patch);
> > if (ret) {
> > mutex_unlock(&klp_mutex);
> > return ret;
> > }
> >
>
> I believe klp_init_patch_before_free() accumulates more responsibilities
> later in the patchset, but I'll ask here: does it really need the
> klp_mutex since it looks to be operating only on the klp_patch, its
> objects and functions?
I do not have a strong opinion about it.
On one hand, we are manipulating all the structures and should prevent
any parallel use. On the other hand, the rest of the code will not
touch the patch until it is added into klp_patches list or until
the sysfs interface is created.
If you think that it might cause false expectations and confusions
then I could move it out of the lock.
Well, in the final version we need to call klp_check_patch_conflict()
under the mutex.
Best Regards,
Petr
