linux-kernel - Re: [PATCH 5/6] livepatch v5: split init and free code that is done only for loaded modules

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: <20141210153448.GC32670@treble.redhat.com>
Date:	Wed, 10 Dec 2014 09:34:48 -0600
From:	Josh Poimboeuf <jpoimboe@...hat.com>
To:	Petr Mladek <pmladek@...e.cz>
Cc:	Seth Jennings <sjenning@...hat.com>, Jiri Kosina <jkosina@...e.cz>,
	Vojtech Pavlik <vojtech@...e.cz>,
	Steven Rostedt <rostedt@...dmis.org>,
	Miroslav Benes <mbenes@...e.cz>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Christoph Hellwig <hch@...radead.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	Andy Lutomirski <luto@...capital.net>,
	live-patching@...r.kernel.org, x86@...nel.org, kpatch@...hat.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/6] livepatch v5: split init and free code that is done
 only for loaded modules

On Wed, Dec 10, 2014 at 11:30:16AM +0100, Petr Mladek wrote:
> On Tue 2014-12-09 12:51:41, Josh Poimboeuf wrote:
> > On Tue, Dec 09, 2014 at 07:05:06PM +0100, Petr Mladek wrote:
> > > This patch makes it clear what initialization and freeing steps need to be done
> > > when an object (module) is being loaded or removed. It will help to maintain
> > > the module coming and going handlers. Also it will remove duplicated
> > > code from these handlers.
> > > 
> > > Signed-off-by: Petr Mladek <pmladek@...e.cz>
> > > ---
> > >  kernel/livepatch/core.c | 92 ++++++++++++++++++++++++++++++++-----------------
> > >  1 file changed, 61 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index 97a8d4a3d6d8..fe312b9ada78 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -590,6 +590,12 @@ static struct kobj_type klp_ktype_func = {
> > >  	.sysfs_ops = &kobj_sysfs_ops,
> > >  };
> > >  
> > > +/* Clean up when a patched object is unloaded */
> > > +static void klp_free_func_loaded(struct klp_func *func)
> > > +{
> > > +	func->old_addr = 0;
> > > +}
> > > +
> > >  /*
> > >   * Free all functions' kobjects in the array up to some limit. When limit is
> > >   * NULL, all kobjects are freed.
> > > @@ -603,6 +609,17 @@ static void klp_free_funcs_limited(struct klp_object *obj,
> > >  		kobject_put(&func->kobj);
> > >  }
> > >  
> > > +/* Clean up when a patched object is unloaded */
> > > +static void klp_free_object_loaded(struct klp_object *obj)
> > > +{
> > > +	struct klp_func *func;
> > > +
> > > +	obj->mod = NULL;
> > > +
> > > +	for (func = obj->funcs; func->old_name; func++)
> > > +		klp_free_func_loaded(func);
> > > +}
> > > +
> > >  /*
> > >   * Free all objects' kobjects in the array up to some limit. When limit is
> > >   * NULL, all kobjects are freed.
> > > @@ -626,6 +643,12 @@ static void klp_free_patch(struct klp_patch *patch)
> > >  	kobject_put(&patch->kobj);
> > >  }
> > >  
> > > +/* parts of the initialization that is done only when the object is loaded */
> > > +static int klp_init_func_loaded(struct klp_object *obj, struct klp_func *func)
> > > +{
> > > +	return klp_find_verify_func_addr(obj, func);
> > > +}
> > > +
> > 
> > Creating a new function here for one line of code, which is only called
> > once, seems excessive, and makes the code harder to understand IMO.
> 
> I see your point. Well, note that the split code is code is called
> twice from klp_init_object() and klp_module_coming(), so it helps
> to remove the code duplicity.
> 
> Also it clearly separates the operations that are always done
> and that are done only when the object is loaded.
> 
> If you suggest to call klp_find_verify_func_addr() directly
> from klp_init_object_loaded(), I am fine with it. We could always
> create it later if there are more operations needed for
> struct func.

Thanks, I'll merge the rest of this patch (except for the separate
klp_init_func_loaded and klp_free_func_loaded).

I should have v6 soon.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/