linux-kernel - RE: [PATCH v4 09/29] staging: unisys: visorinput: remove unnecessary locking

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: <9f6604ab8aad4723869272292a4d6a04@US-EXCH13-2.na.uis.unisys.com>
Date:	Thu, 9 Jun 2016 20:30:29 +0000
From:	"Sell, Timothy C" <Timothy.Sell@...sys.com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	"corbet@....net" <corbet@....net>,
	"mingo@...hat.com" <mingo@...hat.com>,
	"hpa@...or.com" <hpa@...or.com>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"Arfvidson, Erik" <Erik.Arfvidson@...sys.com>,
	"hofrat@...dl.org" <hofrat@...dl.org>,
	"dzickus@...hat.com" <dzickus@...hat.com>,
	"jes.sorensen@...hat.com" <jes.sorensen@...hat.com>,
	"Curtin, Alexander Paul" <Alexander.Curtin@...sys.com>,
	"janani.rvchndrn@...il.com" <janani.rvchndrn@...il.com>,
	"sudipm.mukherjee@...il.com" <sudipm.mukherjee@...il.com>,
	"prarit@...hat.com" <prarit@...hat.com>,
	"Binder, David Anthony" <David.Binder@...sys.com>,
	"nhorman@...hat.com" <nhorman@...hat.com>,
	"dan.j.williams@...el.com" <dan.j.williams@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"driverdev-devel@...uxdriverproject.org" 
	<driverdev-devel@...uxdriverproject.org>,
	*S-Par-Maintainer <SParMaintainer@...sys.com>,
	"Kershner, David A" <David.Kershner@...sys.com>
Subject: RE: [PATCH v4 09/29] staging: unisys: visorinput: remove unnecessary
 locking

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@...utronix.de]
> Sent: Thursday, June 09, 2016 3:56 PM
> To: Sell, Timothy C
> Cc: corbet@....net; mingo@...hat.com; hpa@...or.com;
> gregkh@...uxfoundation.org; Arfvidson, Erik; hofrat@...dl.org;
> dzickus@...hat.com; jes.sorensen@...hat.com; Curtin, Alexander Paul;
> janani.rvchndrn@...il.com; sudipm.mukherjee@...il.com;
> prarit@...hat.com; Binder, David Anthony; nhorman@...hat.com;
> dan.j.williams@...el.com; linux-kernel@...r.kernel.org; linux-
> doc@...r.kernel.org; driverdev-devel@...uxdriverproject.org; *S-Par-
> Maintainer; Kershner, David A
> Subject: RE: [PATCH v4 09/29] staging: unisys: visorinput: remove
> unnecessary locking
> 
> On Thu, 9 Jun 2016, Sell, Timothy C wrote:
> > > From: Thomas Gleixner [mailto:tglx@...utronix.de]
> > >
> > > I think I asked this before, but I might have missed the answer.
> > >
> > > Why is this a rw_sempahore? It's never taken with down_read and
> looking
> > > at the usage sites it's simply a mutex, right?
> >
> > If the semaphore --> mutex change would have been as simple as it
> sounds,
> > we would have had NO problem including it with the next version (v3) of
> this
> > patchset.  But unfortunately, this change uncovered a latent defect, which
> > necessitated yet another patch.  (I know... hard to believe that something
> > this simple would do that, but it did.)  Rather than further complicating
> this
> > patchset, we thought it would be better to address the visorinput issues
> via a
> > separate follow-on patchset.
> 
> That makes me curious. What's the issue? Functional is the mutex the same
> thing as the r/w semaphore when the latter is only taken down_write and
> locked
> and released by the same thread, which is the case as far as I can tell.
> 

The issue: using it uninitialized (<blush>).

A semaphore appears to let you get away with it, but a mutex does NOT.
We had to shuffle some things around to get this right.  If you're
interested in a preview, you can find a patch in github at
https://github.com/davidker/unisys/commit/039e6e517b4a17e2d135a9df85cc1e24a39c2670.
The second bullet in that commit comment describes the scenario
where we were attempting to access the lock in visorinput_open()
before we had actually initialized it:

	* we canNOT get into visorinput_open() until the device
	structure is totally  initialized, by delaying the
	input_register_device() until the end of device initialization

I.e., before this patch, we WERE getting into visorinput_open()
during the call to input_register_device() that was done before 
device initialization was complete, which was BEFORE we initialized
the semaphore.

There is a 2nd follow-on patch that actually does the simple
semaphore --> mutex conversion at
https://github.com/davidker/unisys/commit/6f57ed62ae206c23c58ce4a016b08e15639ce9af.

> > Is that acceptable for you?
> 
> Please fix it before moving the drivers out of staging.

Absolutely.  We will probably push that patchset (containing the
2 github patches referenced above) within the next few days,
even if this visorbus patchset hasn't moved.

Thanks.

Tim Sell

> 
> Thanks,
> 
> 	tglx