linux-kernel - Re: [PATCH v2 1/6] misc: sram: fix error path in sram

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] [day] [month] [year] [list]

Message-ID: <1372952511.3974.38.camel@pizza.hi.pengutronix.de>
Date:	Thu, 04 Jul 2013 17:41:51 +0200
From:	Philipp Zabel <p.zabel@...gutronix.de>
To:	Heiko Stübner <heiko@...ech.de>
Cc:	Arnd Bergmann <arnd@...db.de>, Olof Johansson <olof@...om.net>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <robherring2@...il.com>,
	devicetree-discuss@...ts.ozlabs.org,
	Russell King <linux@....linux.org.uk>,
	linux-kernel@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v2 1/6] misc: sram: fix error path in sram_probe

Hi Heiko,

Am Donnerstag, den 04.07.2013, 16:34 +0200 schrieb Heiko Stübner:
> Hi Philipp,
> 
> Am Dienstag, 25. Juni 2013, 11:04:34 schrieb Philipp Zabel:
> > Hi Heiko,
> > 
> > Am Dienstag, den 25.06.2013, 10:46 +0200 schrieb Heiko Stübner:
> > > The pool is created thru devm_gen_pool_create, so the call to
> > > gen_pool_destroy is not necessary.
> > > Instead the sram-clock must be turned off again if it exists.
> > > 
> > > Signed-off-by: Heiko Stuebner <heiko@...ech.de>
> > > ---
> > > 
> > >  drivers/misc/sram.c |    3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> > > index d87cc91..afe66571 100644
> > > --- a/drivers/misc/sram.c
> > > +++ b/drivers/misc/sram.c
> > > @@ -68,7 +68,8 @@ static int sram_probe(struct platform_device *pdev)
> > > 
> > >  	ret = gen_pool_add_virt(sram->pool, (unsigned long)virt_base,
> > >  	
> > >  				res->start, size, -1);
> > >  	
> > >  	if (ret < 0) {
> > > 
> > > -		gen_pool_destroy(sram->pool);
> > 
> > Right, thanks.
> > 
> > > +		if (sram->clk)
> > > +			clk_disable_unprepare(sram->clk);
> > > 
> > >  		return ret;
> > >  	
> > >  	}
> > 
> > In light of the following patch, I'd rather move the
> > clk_prepare_enable() call after gen_pool_add_virt() and its error path.
> 
> I'm not sure, but isn't moving the clock enablement below the pool allocation 
> producing a race condition?

since this is a platform device, and the driver is not allowed to be
compiled as a module, it is probed at the time platform_driver_register
is called from postcore_initcall.
You'd have to go to quite some lengths to make this race by probing your
driver before mmio-sram and then asynchronously trying to allocate
memory from the pool some time later, without having made sure that the
pool exists during probe.

It would be nice to have callbacks from gen_pool_alloc/free to
enable/disable the clock.
For the time being, maybe it's the safest bet to just enable the clock
before, especially if you reorder the loop so there is only one
gen_pool_add_virt and corresponding error path.

> I.e. can the case happen that some other part wants to allocate part of the 
> newly generated pool already, while the subsequent gen_pool_add_virt calls 
> from the following patch are still running? ... And what will happen in this 
> case, when the sram clock is still disabled?

On i.MX6, the system will hang.

regards
Philipp

--
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/