linux-kernel - Re: [PATCH 2/5] drivercore: Add driver probe deferral mechanism

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:	Fri, 7 Oct 2011 15:28:14 -0600
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Greg KH <greg@...ah.com>
Cc:	"G, Manjunath Kondaiah" <manjugk@...com>,
	linux-arm-kernel@...ts.infradead.org, linux-omap@...r.kernel.org,
	linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org,
	Dilan Lee <dilee@...dia.com>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Manjunath GKondaiah <manjunath.gkondaiah@...aro.org>,
	Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH 2/5] drivercore: Add driver probe deferral mechanism

On Fri, Oct 7, 2011 at 12:49 AM, Greg KH <greg@...ah.com> wrote:
> On Fri, Oct 07, 2011 at 10:33:07AM +0500, G, Manjunath Kondaiah wrote:
>> +config PROBE_DEFER
>> +     bool "Deferred Driver Probe"
>> +     default y
>> +     help
>> +       This option provides deferring driver probe if it has dependency on
>> +       other driver. Without this feature, initcall ordering should be done
>> +       manually to resolve driver dependencies. This feature completely side
>> +       steps the issues by allowing driver registration to occur in any
>> +       order, and any driver can request to be retried after a few more other
>> +       drivers get probed.
>
> Why is this even an option?  Why would you ever want it disabled?  Why
> does it need to be selected?
>
> If you are going to default something to 'y' then just make it so it
> can't be turned off any other way by just not making it an option at
> all.
>
> It also cleans up this diff a lot, as you really don't want #ifdef in .c
> files.

Okay, we'll drop the kconfig

>> +      * This bit is tricky.  We want to process every device in the
>> +      * deferred list, but devices can be removed from the list at any
>> +      * time while inside this for-each loop.  There are two things that
>> +      * need to be protected against:
>
> That's what the klist structure is supposed to make easier, why not use
> that here?
>
>> +      * - if the device is removed from the deferred_probe_list, then we
>> +      *   loose our place in the loop.  Since any device can be removed
>> +      *   asynchronously, list_for_each_entry_safe() wouldn't make things
>> +      *   much better.  Simplest solution is to restart walking the list
>> +      *   whenever the current device gets removed.  Not the most efficient,
>> +      *   but is simple to implement and easy to audit for correctness.
>> +      * - if the device is unregistered, and freed, then there is a risk
>> +      *   of a null pointer dereference.  This code uses get/put_device()
>> +      *   to ensure the device cannot disappear from under our feet.
>
> Ick, use a klist, it was created to handle this exact problem in the
> driver core, so to not use it would be wrong, right?

This comment block is stale.  I reworked the code before I handed it
off to Manjunath, but I never rewrote the comment.  The current code
uses a pair of list to keep deferred devices separate from devices
currently being probed, and the problem described above no longer
exists.  However, Manjunath and I will look into switching from the
two list design to using klist.

Thanks for the feedback.
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
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/