linux-kernel - Re: [PATCH] net: eepro testing positive EBUSY return by request

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: <1293997892.3167.53.camel@localhost>
Date:	Sun, 02 Jan 2011 19:51:32 +0000
From:	Ben Hutchings <ben@...adent.org.uk>
To:	roel kluin <roel.kluin@...il.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: eepro testing positive EBUSY return by
 request_irq()?

On Sun, 2011-01-02 at 15:52 +0100, roel kluin wrote:
> >> -		if (request_irq (*irqp, NULL, IRQF_SHARED, "bogus", dev) != EBUSY) {
> >> +		if (request_irq (*irqp, NULL, IRQF_SHARED, "bogus", dev) != -EBUSY) {
> 
> > This condition is completely bogus - request_irq() with a NULL handler
> > now returns -EINVAL before even checking whether the IRQ is in use.  The
> > code should be fixed along the lines of what I did for 3c503 in commit
> > b0cf4dfb7cd21556efd9a6a67edcba0840b4d98d.
> > 
> > The e2100 and hp net drivers have the same bug.
> > 
> > Ben.
> 
> I've made an attempt to fix the mentioned issues, but I'm not sure it is complete.
> The commit made some other changes as well which didn't seem appropriate here to me.

They are absolutely necessary.

In the bad old days before plug-and-play, some expansion cards would
have jumpers to configure the IRQ number and no software mechanism to
set or query it.  Users were expected to specify the IRQ to the driver
explicitly.  probe_irq_{on,off}() allow drivers to avoid this by
triggering an IRQ and checking which was received.  That assumes that
there are no IRQ conflicts, and it only works reliably in a quiescent
system.

These drivers have been abusing request_irq(), probe_irq_{on,off}() to
do resource allocation for hardware that *does* have a software
mechanism to set the IRQ number.  This has become less reliable as
driver loading has been made more dynamic, and is now broken completely
due to the change in request_irq().  I believe the correct way to do
this allocation is what I did in 3c503.  Possibly it's worth adding a
generic function.

> And I didn't compile or run test this.

I compiled but didn't have hardware to test.

> As a side note, in the function changed by the commit you mentioned, el2_open(), it
> appears that in current git, boolean `seen' cannot ever become true, can it?
[...]

The IRQ handler data is set to &seen and el2_probe_interrupt() uses that
pointer to set it.

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)