linux-kernel - Re: [PATCH 1/2] serial: samsung: Move uart_register

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]

Date:	Mon, 20 Jan 2014 16:26:23 -0800
From:	Greg KH <gregkh@...uxfoundation.org>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
Cc:	Tushar Behera <tushar.behera@...aro.org>,
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
	linux-samsung-soc@...r.kernel.org, jslaby@...e.cz,
	ben.dooks@...ethink.co.uk, broonie@...nel.org
Subject: Re: [PATCH 1/2] serial: samsung: Move uart_register_driver call to
 device probe

On Tue, Jan 21, 2014 at 12:07:06AM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 20, 2014 at 03:51:28PM -0800, Greg KH wrote:
> > On Mon, Jan 20, 2014 at 11:16:03PM +0000, Russell King - ARM Linux wrote:
> > > I don't believe the driver model has any locking to prevent a drivers
> > > ->probe function running concurrently with it's ->remove function for
> > > two (or more) devices.
> > 
> > The bus prevents this from happening.
> > 
> > > The locking against this is done on a per-device basis, not a per-driver
> > > basis.
> > 
> > No, on a per-bus basis.
> 
> I don't see it.
> 
> Let's start from driver_register().

Which happens from module probing, which is single-threaded, right?

Or from module_init callbacks, which is single-threaded.

Normally, busses never add devices (which is what drivers bind to),
except in a single-at-a-time fashion, unless they really know what they
are doing (i.e. PCI had multi-threaded device probing for a while, don't
remember if it still does...)


> This takes no locks and calls bus_add_driver().
> This also takes no locks and calls driver_attach().
> This walks the list of devices calling __driver_attach() for each.
> __driver_attach() tries to match the device against the driver,
> locks the parent device if one exists, and the device which is about
> to be probed.  It then calls driver_probe_device().
> driver_probe_device() inserts a runtime barrier and calls really_probe().
> really_probe() ultimately calls either the bus ->probe method or the
> driver ->probe method.
> 
> At no point in that sequence do I see anything which does any locking
> on a per-driver basis.  Let's look at device_add().
> 
> device_add() calls bus_probe_device(), which then calls device_attach().
> device_attach() takes the device's lock, and walks the list of drivers
> calling __device_attach() on each driver.  This then calls down into
> driver_probe_device(), and the path is the same as the above.
> 
> I don't see any per-driver locking here either.
> 
> I've checked the klist stuff, don't see anything there.  Ditto for
> bus_for_each_drv().
> 
> If you think there's a per-driver lock that's held over probes or removes,
> please point it out.  I'm fairly certain that there isn't, because we have
> to be able to deal with recursive probes (yes, we've had to deal with
> those in the past.)

Hm, you are right, I think that's why we had to remove the locks.  The
klist stuff handles us getting the needed locks for managing our
internal lists of devices and drivers, and those should be fine.

So, let's go back to your original worry, what are you concerned about?
A device being removed while probe() is called?

thanks,

greg k-h
--
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/