linux-kernel - Re: [PATCH] Prevent nested interrupts when the IRQ stack is near overflowing v2

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: <alpine.LFD.2.00.1003251338130.3147@localhost.localdomain>
Date:	Thu, 25 Mar 2010 14:17:59 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Andi Kleen <andi@...stfloor.org>
cc:	x86@...nel.org, LKML <linux-kernel@...r.kernel.org>,
	jesse.brandeburg@...el.com,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] Prevent nested interrupts when the IRQ stack is near
 overflowing v2

On Thu, 25 Mar 2010, Andi Kleen wrote:

> On Thu, Mar 25, 2010 at 12:09:10PM +0100, Thomas Gleixner wrote:
> > On Thu, 25 Mar 2010, Andi Kleen wrote:
> > > On Thu, Mar 25, 2010 at 02:46:42AM +0100, Thomas Gleixner wrote:
> > > > 3) Why does the NIC driver code not set IRQF_DISABLED in the first
> > > >    place?  AFAICT the network drivers just kick off NAPI, so whats the
> > > >    point to run those handlers with IRQs enabled at all ?
> > > 
> > > I think the idea was to minimize irq latency for other interrupts
> > 
> > So what's the point ? Is the irq handler of that card so long running,
> > that it causes trouble ? 
> 
> I believe it's more like that you have a lot of them (even with interrupt
> mitigation and NAPI polling) and they have to scale up the work to handle
> high bandwidths, so it ends up with a lot of time in them anyways,
>  ending with skew in the timers and similar issues.

Timekeeping is an absolute non issue here. The time keeping code can
cope with pretty large delays and there is nothing which 
 
> Anyways my goal here was simply to have a least intrusive fix, not
> change semantics for everyone.
>
> Yes it could work out this. Or it could not. I'm not sure, so I chose
> the safe option.

Oh it's safe because it solves the problem on that particular machine
and workload?

No, it's not safe at all. 

    - it does not prevent a driver reenabling interrupts

    - it will cause real large latencies when the interrupt which
      happens to trigger that check is one of the long running
      ones. Are you going to deal with the "oh we see >500us" latencies
      bugreports ?

    - worst case it'll brick your machine if the interrupt which
      happens to trigger that check relies on irq enabled semantics
      (e.g. missing jiffies update). Yes, such a driver is broken by
      design, but we have such crap lurking. So much for not changing
      semantics.
 
> > So what's the point of running a well written (short) interrupt
> > handler with interrupts enabled ? Nothing at all. It just makes us
> > deal with crap like stacks overflowing for no good reason.
> 
> Ok so you're just proposing to always set IRQF_DISABLED? 

For any sane written driver, yes. We have crap irq handlers which need
to be fixed, so we can't enforce it right away. See above.
 
> I don't know what problems it would cause. My fear is that any latency problems
> would be answered with a "move to RT, moron" from your mouth though.

Oh well.
 
> > > Anyways if such a thing was done it would be a long term project
> > > and that short term fix would be still needed.
> > 
> > Your patch is not a fix, It's a lousy, horrible and unreliable
> > workaround. It's not fixing the root cause of the problem at hand.
> 
> It fixes the bug in a minimally intrusive way.

It papers over the problem. We already know that the NIC driver floods
the machine with interrupts, so why are you insisting that we need to
bandaid that problem ?

The minimal intrusive way is a one liner in that very driver code and
if it causes problems for that very driver then we don't fix them with
adding a callback in the generic interrupt code path.

The message which we would send out with applying that band aid would
be simply: Go ahead driver writers and let your handlers run as long
as they want, we'll safe you in 99.9% of the cases and we'll happily
go and debug the 0.1% of completely undebuggable shit which will
result out of that.

No thanks,

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