Re: livepatch: Avoid possible race when releasing the patch

[Miroslav Benes]

On Mon, 23 May 2016, Jessica Yu wrote:

> +++ Petr Mladek [23/05/16 17:54 +0200]:
> > There was a long discussion about a possible race with sysfs, kobjects
> > when removing an unused livepatch, see
> > https://lkml.kernel.org/g/%3C1462190242-24731-1-git-send-email-mbenes@xxxxxxx%3E
> > 
> > This patch set tries to implement what looked the most preferred solution
> > from the discussion. I did my best to keep the patch definition simple.
> > But I am not super happy with the result.
> > 
> > I send the current state before I spent even more time on different
> > approaches.
> > 
> > I personally think that we might get better result if we declare
> > some limited structures, define them statically and then copy all
> > data into the final structures in a single call. I did not implement
> > this because it was weird on the first look but I am not sure now.
> > 
> > But even more I would prefer the solution with the completion.
> > It is already used by the module framework. It does not look
> > that hacky to me after all.
> 
> Hi Petr, thanks a lot for the RFC and for exploring this possible
> solution. I haven't reviewed the patches thoroughly yet, but at first
> glance I admit that I did not think through how much this approach
> would complicate the livepatch API, and the new intermediary functions
> do seem like overkill in response to the original kobject problem..
>
> I looked at how the module loader used the completion, and in fact
> it is used to remedy a nearly identical problem with
> DEBUG_KOBJ_RELEASE (see commit 942e443 "Fix mod->mkobj.kobj
> potentially freed too early"), and Miroslav's original solution pretty
> much took the same approach. We could even mirror that approach and
> have something like klp_kobject_put() (much like mod_kobject_put()) to
> package up the kobject_put/wait_for_completion calls, but that is
> purely a matter of taste.

Hi,

I'm biased here so it is not surprising that I'd go with completion. There 
is even one more thing to be aware of. We have 'struct module *mod' in 
klp_patch and we use it throughout the code. We still need to be careful 
with it even with Petr's approach. The problem stays but it is greatly
diminished to just this one pointer. That is one can call our sysfs 
function which potentially uses mod pointer and the module could go away 
just before that.
 
> Anyway, I am just beginning to lean towards the completion solution
> again (sorry for jumping back and forth :-/), but we can play with
> this patchset a bit more and see if we can come up with something
> reasonable.

Yes. In fact it does not look that bad. Thanks Petr for doing this.

Josh, I agree that we could return to Seth's approach but it still seems a 
bit awkward. Completion is only a small modification and since module 
loader uses it itself it does not look like serious hack to me anymore.

Regards,
Miroslav