netdev - Re: [PATCH 0/3]: via-velocity: Fixes for locking issues

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:	Tue, 09 Feb 2010 16:31:33 -0800 (PST)
From:	David Miller <davem@...emloft.net>
To:	simon.kagstrom@...insight.net
Cc:	netdev@...r.kernel.org, davej@...hat.com, ben@...adent.org.uk
Subject: Re: [PATCH 0/3]: via-velocity: Fixes for locking issues

From: Simon Kagstrom <simon.kagstrom@...insight.net>
Date: Fri, 5 Feb 2010 16:52:53 +0100

> The patch uses spin_trylock in the interrupt handler, and returns
> IRQ_NONE if it's already held. Here is a place where I'm unsure about
> the right way though. Another alternative is to use spin_lock_irqsave
> in velocity_poll(), but I'd like to avoid turning the interrupts off
> for "long" periods of time when the actual velocity interrupt can't
> happen anyway (since it is turned off while executing velocity_poll().

It can happen in this situation and this is not the right way to
handle this.

You will lose interrupt events if the situation is that the interrupt
for the VIA has migrated from one cpu to another and another cpu is in
the VIA interrupt handler when we see the lock held.

Consider also the case where another cpu has the VIA lock as
a result of a device sharing the IRQ with VIA firing.

You'll lose events and jam the chip in that case too.

What this comes down to is not using the lock correctly, you
can't paper around it with this trylock stuff.

The proper thing to do is make the ->poll() handler use the lock
correctly by disabling interrupts.

If the VIA driver wants to avoid holding interrupts disabled for long
periods of time in ->poll(), it's locking mechanisms will need to be
rewritten completely.  The only safe way to do this is to do something
like how the tg3 driver works, wherein the interrupt handler runs
lockless, there is a special IRQ quiesce sequence when shutting down
the chip, and the ->poll() handler et al. use softirq safe locks
instead of hardirq safe ones.

For now you're going to have to accept interrupts being disabled in
order to fix this bug for the time being.

Please resubmit this patch series once you've worked this out.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html