netdev - Re: [PATCH v6 1/2] Ethernet driver for the WIZnet W5300 chip

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:	Wed, 4 Apr 2012 11:01:49 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Mike Sinkovsky <msink@...monline.ru>
Cc:	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 1/2] Ethernet driver for the WIZnet W5300 chip

On Wed, Apr 04, 2012 at 12:18:12PM +0600, Mike Sinkovsky wrote:
> 03.04.2012 19:36, Mark Brown написал:

> >...you use devm_request_threaded_irq() here and rely on it for cleanup.
> >Are you sure there's no possibility of the interrupt firing after you
> >start to tear down the device?

> >By using a specifically threaded IRQ you're also adding a performance
> >overhead for no good reason if you can call netdev_carrier_*() from IRQ
> >context and the GPIO is capable of generating a hard IRQ.  If you use
> >request_any_context_irq() instead then the driver will get a hard IRQ if
> >that's supported.

> There isn't devm* variant of request_any_context_irq(), and using
> plain version looks inconsistent with other resources handling.
> Anyway, this is not performance critical procedure, and latency
> around 100 millisecond is acceptable. Some our boards even don't
> have this gpio at all, and nothing bad happens, just userspace
> doesn't know is carrier on or off.

None of this addresses the primary concern which is that because you're
not (as far as I can tell) ensuring that the interrupt won't fire the
driver might crash if the interrupt fires in between the resources it
needs to handle the interrupt being deallocated and the interrupt being
unregistered.  Managed interrupts are relatively tricky to use because
of this issue, with most things like memory it doesn't matter exactly
when it's deallocated but interrupts can potentially trigger actions
themselves.

If you can use it and devm_request_any_context_irq() doesn't exist it'd
be better to add it.

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