Message-ID: <20160531190026.GB23506@packer-debian-8-amd64.digitalocean.com>
Date:	Tue, 31 May 2016 15:00:27 -0400
From:	Jessica Yu <jeyu@...hat.com>
To:	Petr Mladek <pmladek@...e.com>
Cc:	Miroslav Benes <mbenes@...e.cz>, jpoimboe@...hat.com,
	jikos@...nel.org, jslaby@...e.cz, live-patching@...r.kernel.org,
	linux-kernel@...r.kernel.org, huawei.libin@...wei.com,
	minfei.huang@...oo.com
Subject: Re: livepatch: Avoid possible race when releasing the patch

+++ Petr Mladek [30/05/16 17:31 +0200]:
>On Wed 2016-05-25 10:58:38, Miroslav Benes wrote:
>> 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@suse.cz%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.
>
>Yeah, I forgot to mention that we are actually not able to make
>patch->mod pointer valid without the completion.
>
>I gave it another week and I have got even more convinced that the
>completion would be the right way to go. It is not that hacky after all
>and it might safe us some headaches in the future. IMHO,
>it is too painful to copy all information from the module just
>to make kobjects happy.

Ah yeah, good catch. So even if livepatch internally managed these
structures, the mod pointer in klp_patch still wouldn't be protected,
which the completion trivially fixes. So I'd be OK with the completion
as well. Thanks for the RFC though Petr, in my head it sounded like a
good idea in theory, but in practice turned out to be too cumbersome.

Jessica

Powered by blists - more mailing lists