Re: [PATCH 2/2] serial: pl011: Move uart_register_driver call to device probe

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Thu, Feb 13, 2014 at 03:26:06PM -0800, Greg KH wrote:
> On Thu, Feb 13, 2014 at 06:42:49PM +0000, Russell King - ARM Linux wrote:
> > We went through this before, and I stated the paths, and no one disagreed
> > with that.
> > 
> > It /is/ racy.
> 
> Ok, I just went and looked at the uart driver register path, and I don't
> see the race (note, if there is one, it's there today, regardless of
> this patch).

The race isn't the uart code, it's the driver model.

Consider what happens when this happens:

* Two pl011 devices get registered at the same time by two different
  threads.

* Both devices have a lock taken on the _device_ itself before matching
  against the driver.

* Both devices get matched to the same driver.

* Both devices are passed into the driver's probe function.

* Both check uart_reg.state, both call uart_register_driver() on that
  at the same time, which results in two allocations inside
  uart_register_driver(), one gets overwritten...

So, the /only/ thing which stops this happening is that the devices
are generally available before the driver is registered, and driver
registration results in devices being probed serially.  Moreover, both
attempt to call tty_register_driver()... one succeeds, the other fails.

However, what about the userspace bind/unbind methods.  Yes, userspace
can ask the driver core to unbind devices from a driver or bind - and
again, there's no per-driver locking here.  So, if you can trigger two
concurrent binds from userspace, you hit the same race as above.

So, if you want to accept these patches, go ahead, introduce races, but
personally I'd recommend plugging these races.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".