Date: Tue, 3 Nov 2015 13:44:41 +0100
From: Petr Mladek <pmladek@suse.com>
To: Miroslav Benes <mbenes@suse.cz>
Cc: Chris J Arges <chris.j.arges@canonical.com>, live-patching@vger.kernel.org,
        jeyu@redhat.com, Josh Poimboeuf <jpoimboe@redhat.com>,
        Seth Jennings <sjenning@redhat.com>, Jiri Kosina <jikos@kernel.org>,
        Vojtech Pavlik <vojtech@suse.com>, linux-api@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs
 directory
Message-ID: <20151103124440.GK2599@pathway.suse.cz>
References: <20151102203241.GF27488@treble.redhat.com>
 <1446505187-28970-1-git-send-email-chris.j.arges@canonical.com>
 <alpine.LNX.2.00.1511031146070.6257@pobox.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.LNX.2.00.1511031146070.6257@pobox.suse.cz>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2651
Lines: 78

On Tue 2015-11-03 11:52:08, Miroslav Benes wrote:
> On Mon, 2 Nov 2015, Chris J Arges wrote:
> 
> [...]
> 
> > +static int klp_get_func_pos_callback(void *data, const char *name,
> > +				      struct module *mod, unsigned long addr)
> > +{
> > +	struct klp_find_arg *args = data;
> > +
> > +	if ((mod && !args->objname) || (!mod && args->objname))
> > +		return 0;
> > +
> > +	if (strcmp(args->name, name))
> > +		return 0;
> > +
> > +	if (args->objname && strcmp(args->objname, mod->name))
> > +		return 0;
> > +
> > +	/* on address match, return 1 to break kallsyms_on_each_symbol loop */
> > +	if (args->addr == addr)
> > +		return 1;
> > +
> > +	/* if we don't match addr, count instance of named symbol */
> > +	args->count++;
> > +
> > +	return 0;
> > +}
> > +
> > +static int klp_get_func_pos(struct klp_object *obj, struct klp_func *func)
> > +{
> > +	struct klp_find_arg args = {
> > +		.objname = obj->name,
> > +		.name = func->old_name,
> > +		.addr = func->old_addr,
> > +		.count = 0,
> > +	};
> > +
> > +	mutex_lock(&module_mutex);
> > +	kallsyms_on_each_symbol(klp_get_func_pos_callback, &args);
> > +	mutex_unlock(&module_mutex);
> > +
> > +	return args.count;
> > +}
> > +
> >  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> >  {
> >  	INIT_LIST_HEAD(&func->stack_node);
> >  	func->state = KLP_DISABLED;
> >  
> >  	return kobject_init_and_add(&func->kobj, &klp_ktype_func,
> > -				    &obj->kobj, "%s", func->old_name);
> > +				    &obj->kobj, "%s,%d", func->old_name,
> > +				    klp_get_func_pos(obj, func));
> >  }
> 
> There is a problem which I missed before. klp_init_func() is called before 
> klp_find_verify_func_addr() in klp_init_object(). This means that 
> func->old_addr is either not verified yet or worse it is still 0. This 
> means that klp_get_func_pos_callback() never returns 1 and is thus called 
> on each symbol. So if you for example patched cmdline_proc_show the 
> resulting directory in sysfs would be called cmdline_proc_show,1 because 
> addr is never matched. Had old_addr been specified the name would have 
> been probably correct, but not for sure.

This might happen when the function name is unique. Then we might but
we do not need to pre-define the address in the patch.

Also I would omit the suffix at all when it is the first occurrence.
It will cause that unique symbols will not be numbered.

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