linux-kernel - Re: [PATCH] device model: Do a quickcheck for driver binding before doing an expensive check

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:	Mon, 15 Sep 2008 16:53:42 +0200
From:	Cornelia Huck <cornelia.huck@...ibm.com>
To:	Arjan van de Ven <arjan@...radead.org>
Cc:	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	greg@...ah.com
Subject: Re: [PATCH] device model: Do a quickcheck for driver binding before
 doing an expensive check

On Mon, 15 Sep 2008 07:06:44 -0700,
Arjan van de Ven <arjan@...radead.org> wrote:

> On Mon, 15 Sep 2008 13:32:26 +0200
> Cornelia Huck <cornelia.huck@...ibm.com> wrote:
> 
> > On Sun, 14 Sep 2008 08:32:06 -0700,
> > Arjan van de Ven <arjan@...radead.org> wrote:
> > 
> > > This patch adds a quick check for the driver<->device match before
> > > taking the locks and doin gthe expensive checks. Taking the lock
> > > hurts in asynchronous boot context where the device lock gets hit;
> > > one of the init functions takes the lock and goes to do an
> > > expensive hardware init; the other init functions walk the same PCI
> > > list and get stuck on the lock as a result.
> > 
> > Hm, you call bus->match twice now; once without dev->sem held and once
> > with it. For the busses I'm familiar with that shouldn't be a problem,
> > but are you sure there aren't busses which want dev->sem held?
> > (Although I think not relying on dev->sem would be the sane thing...)
> 
> As far as I can see it's ok, but if not I obviously like to hear about
> it SOON :)

I don't see any problem on the s390 busses, and pci and usb look OK as
well at a glance.

> 
> 
> > > 
> > > For the common case, we can know there's no chance whatsoever of a
> > > match if the device isn't in the drivers ID table... so this patch
> > > does that check as a best-effort-avoid-the-lock approach.
> > 
> > I've always thought of ->match being a quick check which just looks at
> > the IDs with ->probe doing the heavier stuff, so this should be
> > reasonable (if all busses play nicely). But driver_probe_device()
> > still calls ->match a second time, and device_attach() will thus
> > always call ->match under the lock. Should it be moved out of the
> > lock there as well?
> 
> having a second check is actually not a bad thing per se; in terms of
> programming pattern, doing the quick checks before the lock, but doing
> the final check inside the lock makes sense to me. If there's real
> objections to doing the match the second time (it's cheap!) I'll remove
> it, but this way, you can call the "heavy" function always and from
> anywhere, and it'll just do the right thing no matter what. I kinda like
> that as concept ;)

OK, you have a point. I just find it a bit ugly; especially as the
->probe function will check if the device matches as well (by poking at
the device).

But I'd be fine with either way :)

--
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/