From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757402AbcEEPEI (ORCPT ); Thu, 5 May 2016 11:04:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49628 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756734AbcEEPEF (ORCPT ); Thu, 5 May 2016 11:04:05 -0400 Date: Thu, 5 May 2016 10:04:03 -0500 From: Josh Poimboeuf To: Miroslav Benes Cc: Jiri Kosina , jeyu@redhat.com, pmladek@suse.com, jslaby@suse.cz, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, huawei.libin@huawei.com, minfei.huang@yahoo.com Subject: Re: [RFC PATCH] livepatch: allow removal of a disabled patch Message-ID: <20160505150403.h76mtzyqikitov6f@treble> References: <20160504023948.ttb3ko2wnicwruwy@treble> <20160504033619.5osteklgal3ixcbo@treble> <20160504131423.5yqvie2zy67jspak@treble> <20160504161423.pvupipravfxuyktz@treble> <20160505132748.kwk3pmgetud3c66r@treble> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.0.1 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 05, 2016 at 04:25:48PM +0200, Miroslav Benes wrote: > On Thu, 5 May 2016, Josh Poimboeuf wrote: > > > On Thu, May 05, 2016 at 10:28:12AM +0200, Miroslav Benes wrote: > > > I think it boils down to the following problem. > > > > > > 1. CONFIG_DEBUG_KOBJECT_RELEASE=y > > > > > > 2. we have dynamic kobjects, so there is a pointer in klp_patch to struct > > > kobject > > > > > > 3. it is allocated during klp_init_patch() and all is fine > > > > > > 4. now we want to remove the patch module. It is disabled and module_put() > > > is called. User calls rmmod on the module. > > > > > > 5. klp_unregister_patch() is called in __exit method. > > > > > > 6. klp_free_patch() is called. > > > > > > 7. kobject_put(patch->kobj) is called. > > > > > > ...now it gets interesting... > > > > > > 8. among others kobject_cleanup() is scheduled as a delayed work (this is > > > important). > > > > > > 9. there is no completion, so kobject_put returns and the module goes > > > away. > > > > > > 10. someone calls patch enabled_store attribute (for example). They can > > > because kobject_cleanup() has not been called yet. It is delayed > > > scheduled. > > > > > > ...crash... > > > > But what exactly causes the crash? In enabled_store() we can see that > > the patch isn't in the list, so we can return -EINVAL. > > Ok, bad example. Take enabled_show() instead. It could be fixed in the > same way, but I am not sure it is the right thing to do. It does not scale > because the problem is elsewhere. > > Anyway, it is (even if theoretically) there in my opinion and we > have two options. > > 1. We could forget about CONFIG_DEBUG_KOBJECT_RELEASE and all is ok > without completion and regardless of dynamic/static kobject allocation. > > 2. We introduce completion and we are ok even with > CONFIG_DEBUG_KOBJECT_RELEASE=y and again regardless of dynamic/static > kobject allocation. I would disagree with the statement that the dynamic kobject doesn't scale. We would just need a helper function to get from a kobject to its klp_patch. In fact, to me it seems like the right way to do it. It doesn't make sense for the code which creates the kobject to be different from the code which initializes it. It's slightly out of context, but kobject.txt does say: "Code which creates a kobject must, of course, initialize that object." I view the completion as a hack to compensate for the fact that we're abusing the kobject interface. And so it makes sense to me that CONFIG_DEBUG_KOBJECT_RELEASE would cause problems, because we're using kobjects in the wrong way. So in my view, the two options are: 1. Convert the kobject to dynamic as I described. 2. Change the klp_register() interface so that klp_patch gets allocated in livepatch code. I'd be curious to hear what others think. -- Josh