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

Date:	Sat, 11 Jun 2016 03:34:49 +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

> > > > 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/039e6e517b4a17e2d135a9df85
> cc1e24a39c2670.
> 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/6f57ed62ae206c23c58ce4a016
> b08e15639ce9af.
> 
> > > 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.
> 

The 2 patches referenced above (which include the bug fix, and the change
from semaphore to mutex) are included as patches #27 and #28 in the
patchset just submitted by David Kershner:

	[PATCH RESEND 00/28] staging: unisys: fix visorbus & visorinput issues raised by tglx

Thanks.

Tim Sell