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 for Android: free password hash cracker in your pocket

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

Message-ID: <20160530153103.GK23103@pathway.suse.cz>
Date:	Mon, 30 May 2016 17:31:03 +0200
From:	Petr Mladek <pmladek@...e.com>
To:	Miroslav Benes <mbenes@...e.cz>
Cc:	Jessica Yu <jeyu@...hat.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 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.

Best Regards,
Petr