linux-kernel - Re: livepatch: Avoid possible race when releasing the patch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LNX.2.00.1605251047580.31648@pobox.suse.cz>
Date:	Wed, 25 May 2016 10:58:38 +0200 (CEST)
From:	Miroslav Benes <mbenes@...e.cz>
To:	Jessica Yu <jeyu@...hat.com>
cc:	Petr Mladek <pmladek@...e.com>, 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

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. 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