From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S932525AbcBIMbx (ORCPT <rfc822;w@1wt.eu>);
	Tue, 9 Feb 2016 07:31:53 -0500
Received: from mx2.suse.de ([195.135.220.15]:57044 "EHLO mx2.suse.de"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1755356AbcBIMbv (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Tue, 9 Feb 2016 07:31:51 -0500
Date: Tue, 9 Feb 2016 13:31:48 +0100
From: Petr Mladek <pmladek@suse.com>
To: Jiri Kosina <jikos@kernel.org>
Cc: Jessica Yu <jeyu@redhat.com>, Rusty Russell <rusty@rustcorp.com.au>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Seth Jennings <sjenning@redhat.com>, Vojtech Pavlik <vojtech@suse.com>,
        Jonathan Corbet <corbet@lwn.net>, Miroslav Benes <mbenes@suse.cz>,
        linux-api@vger.kernel.org, live-patching@vger.kernel.org,
        x86@kernel.org, linux-kernel@vger.kernel.org,
        linux-s390@vger.kernel.org, linux-doc@vger.kernel.org
Subject: Re: [RFC PATCH v4 2/6] module: preserve Elf information for
 livepatch modules
Message-ID: <20160209123148.GB12548@pathway.suse.cz>
References: <1454548271-24923-1-git-send-email-jeyu@redhat.com>
 <1454548271-24923-3-git-send-email-jeyu@redhat.com>
 <20160209084443.GA12548@pathway.suse.cz>
 <alpine.LNX.2.00.1602091131510.22727@cbobk.fhfr.pm>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.LNX.2.00.1602091131510.22727@cbobk.fhfr.pm>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue 2016-02-09 11:33:07, Jiri Kosina wrote:
> On Tue, 9 Feb 2016, Petr Mladek wrote:
> 
> > > +#ifdef CONFIG_KALLSYMS
> > > +	/* Make symtab and strtab available prior to module init call */
> > > +	mod->num_symtab = mod->core_num_syms;
> > > +	mod->symtab = mod->core_symtab;
> > > +	mod->strtab = mod->core_strtab;
> > > +#endif
> > 
> > This should be done with module_mutex. Otherwise, it looks racy at least 
> > against module_kallsyms_on_each_symbol().
> 
> Hmm, if this is the case, the comment describing the locking rules for 
> module_mutex should be updated.
> 
> > BTW: I wonder why even the original code is not racy for example against 
> > module_get_kallsym. It is called without the mutex. This code sets the 
> > number of entries before the pointer to the entries.
> > 
> > Note that the module is in the list even in the UNFORMED state.
> 
> module_kallsyms_on_each_symbol() gives up in such case though.

The problem is that we set the three values above in the COMMING
state. They were even set in the LIVE state before this patch.

IMHO, we should reorder the writes and add a write barrier.


#ifdef CONFIG_KALLSYMS
	/* Make symtab and strtab available prior to module init call */
	mod->strtab = mod->core_strtab;
	mod->symtab = mod->core_symtab;
	/* Tables must be availabe before the number is set */
	smp_wmb();
	mod->num_symtab = mod->core_num_syms;
#endif


Then we should add the corresponding read barrier to
the read functions that are protected only by
the disabled preeption. For example:


static unsigned long mod_find_symname(struct module *mod, const char *name)
{
	unsigned int i;

	for (i = 0; i < mod->num_symtab; i++)
		/* Make sure that tables are set for the read num_symtab */
		smp_rmb();
		if (strcmp(name, mod->strtab+mod->symtab[i].st_name) == 0 &&
		    mod->symtab[i].st_info != 'U')
			return mod->symtab[i].st_value;
	return 0;
}


Or am I too paranoid, please?

Best Regards,
Petr