linux-kernel - Re: new driver for drivers/virt/?

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: <alpine.DEB.2.11.1605182347080.3851@nanos>
Date:	Thu, 19 May 2016 00:11:46 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	"Sell, Timothy C" <Timothy.Sell@...sys.com>
cc:	"mingo@...nel.org" <mingo@...nel.org>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"timur@...escale.com" <timur@...escale.com>,
	"galak@...nel.crashing.org" <galak@...nel.crashing.org>,
	"Kershner, David A" <David.Kershner@...sys.com>,
	"corbet@....net" <corbet@....net>,
	"mingo@...hat.com" <mingo@...hat.com>,
	"hpa@...or.com" <hpa@...or.com>,
	"Arfvidson, Erik" <Erik.Arfvidson@...sys.com>,
	"hofrat@...dl.org" <hofrat@...dl.org>,
	"dzickus@...hat.com" <dzickus@...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>,
	Greg KH <gregkh@...uxfoundation.org>,
	Jes Sorensen <Jes.Sorensen@...hat.com>
Subject: Re: new driver for drivers/virt/?

On Wed, 18 May 2016, Sell, Timothy C wrote:
> We have a bus driver currently in drivers/staging/unisys/visorbus/ that
> we are trying to get out of staging and into the kernel proper.  Since
> "visorbus" is a driver to host a virtual bus presented to a Linux guest
> in a hypervisor environment (refer to 
> drivers/staging/unisys/Documentation/overview.txt for more details),
> Greg KH and Jes Sorensen have suggested the possibility that drivers/virt/
> might be a good place for visorbus.  But right now, we see that the only
> driver under drivers/virt/ is the Freescale hypervisor environment, which
> made us wonder whether this was really the correct place.
> 
> Would you have any guidance for us?
> Our intent is to push our visorbus out of staging immediately following
> the current merge window.

What's the problem with Gregs and Jes suggestion? I don't see any.

There is bigger fish to fry than the final place of this driver. I had just a
peek at the staging code and there is enough stuff which wants to be cleaned
up before moving anywhere. I don't have time to do a proper review now, but
here are a few hints upfront:

1) Locking:

   visordriver_callback_lock: 

      That should be a mutex, not a semaphore

   periodic_work->lock: 

      Why is this a rw_lock if it's only locked with write_lock? And what's
      the purpose of this lock at all?

2) Memory barriers:

   Completely undocumented wmb()s without corresponding rmb()s to do obscure
   protection of that periodic work stuff.

3) periodic_work:

   That set of functions is obscure. Especially visor_periodic_work_stop()
   makes me shudder. See also #2. 

   That work->lock does not inspire my confidence further.

4) Exports:

   A gazillion of exports which are just wrappers around another set of
   exports

5) Function comments:

   Try to mimic kerneldoc comments, i.e. start with: /**
   but do not implement any of the kerneldoc requirements.

I'll try do find a time slot for a proper review of that thing, but don't
expect that to happen in the next days.

Thanks,

	tglx