linux-kernel - Re: lockdep and threaded IRQs (was: ...)

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: <200903021520.30826.david-b@pacbell.net>
Date:	Mon, 2 Mar 2009 15:20:30 -0800
From:	David Brownell <david-b@...bell.net>
To:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Cc:	Andrew Morton <akpm@...ux-foundation.org>, me@...ipebalbi.com,
	linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
	felipe.balbi@...ia.com, dmitry.torokhov@...il.com,
	sameo@...nedhand.com, tglx@...utronix.de
Subject: Re: lockdep and threaded IRQs (was: ...)

On Monday 02 March 2009, Peter Zijlstra wrote:
> On Mon, 2009-03-02 at 14:10 -0800, David Brownell wrote:
> 
> > What's unfortunate is that you prefer not to fix that
> > IRQF_DISABLED bug in lockdep, which you co-"maintain".
> > When running with lockdep, that bug (a) introduces bugs
> > in some drivers and (b) hides bugs in others.  You've
> > rejected even a minimal warning fix, to help minimize
> > the amount of time developers waste on (a) and (b).
> 
> I've come to the conclusion that the only technically sound solution is
> to do as I proposed today, utterly eliminate !IRQF_DISABLED handlers.

As you announced today.  If you truly believe that, then
you should at least submit a warning patch for 2.6.29-rc
("driver X isn't setting IRQF_DISABLED, reimplement!")
with a Documentation/feature-removal-schedule.txt plan
for removing that mechanism.  And maybe updating the
handling of IRQF_SHARED at the same time... most shared
IRQs don't want IRQF_DISABLED.  And ... surely more,
even just to start phasing out such a longstanding
mechanism.

(Or more likely, start a real LKML discussion on that
specific topic.  I suspect it won't complete in time
to merge such a patch with any sort of consensus.)

> Apparently you had enough time to come up with the creative genirq abuse
> of twl4030, I think that with a similar effort you could have
> implemented generic threaded irq stuff like proposed by Thomas.

Actually, I made time to clean that code up a lot;
I didn't come up with that original stuff at all.

Why would you think otherwise, after reading the
copyrights at the top of twl4030-irq.c ??

And when I was doing that cleanup, the Official Plan
was that Thomas' code would be ready for merge into
at least the -next tree soon.  I don't know about
you, but stepping all over his work didn't seem like
it would be at all constructive.  Having a significant
driver be ready to try using it ... seemed like a
more productive approach.

> > Attacking folk for having to cope with such bugs escalates
> > things beyond "unfortunate".  If lockdep is "maintained",
> > your response should be fixing that lockdep bug.  Once
> > that's done, all workarounds for that bug can be removed.
> 
> I state there is no lockdep bug in this respect. The bug is trying to
> enable interrupts from hardirq context and running code that assumes
> hardirq context from task context.

Well, I state that you're wrong about those points...

Regardless of what either of us states, the truth of
the matter is that there are drivers that don't work
with CONFIG_LOCKDEP and the *only* reason is the code
removed by the (untested) patchlet below:  semantics
of irqs change when lockdep is enabled.  (That is, your
characterization above is incorrect.)  I call that a
lockdep bug; you don't want to accept that label.

Are those incorrect lockdep assumptions really so hard
to fix?  Where do they surface?  I scanned lockdep.c
briefly, and it talks about "hardirq" in several places.
Is it conflating that with "irqs disabled"?  The two
concepts should be distinct.

- Dave

---
 kernel/irq/manage.c |    6 ------
 1 file changed, 6 deletions(-)

--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -816,12 +816,6 @@ int request_irq_quickcheck(unsigned int 
 				"guaranteed on shared IRQs\n",
 				irq, devname);

-#ifdef CONFIG_LOCKDEP
-	/*
-	 * Lockdep wants atomic interrupt handlers:
-	 */
-	irqflags |= IRQF_DISABLED;
-#endif
 	/*
 	 * Sanity-check: shared interrupts must pass in a real dev-ID,
 	 * otherwise we'll have trouble later trying to figure out
--
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/