netdev - Re: [RFC PATCH net-next 00/11] net: remove disable_irq() from ->ndo_poll

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:	Fri, 12 Dec 2014 23:01:28 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Sabrina Dubroca <sd@...asysnail.net>
cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	peterz@...radead.org
Subject: Re: [RFC PATCH net-next 00/11] net: remove disable_irq() from
 ->ndo_poll_controller

On Thu, 11 Dec 2014, Sabrina Dubroca wrote:
> 2014-12-09, 21:44:33 -0500, David Miller wrote:
> > 
> > Adding a new spinlock to every interrupt service routine is
> > simply a non-starter.
> > 
> > You will certainly have to find a way to fix this in a way
> > that doesn't involve adding any new overhead to the normal
> > operational paths of these drivers.
> 
> Okay. Here is another idea.
> 
> Since the issue is with the wait_event() part of synchronize_irq(),
> and it only takes care of threaded handlers, maybe we could try not
> waiting for threaded handlers.
> 
> Introduce disable_irq_nosleep() that returns true if it successfully
> synchronized against all handlers (there was no threaded handler
> running), false if it left some threads running.  And in
> ->ndo_poll_controller, only call the interrupt handler if
> synchronization was successful.
> 
> Both users of the poll controllers retry their action (alloc/xmit an
> skb) several times, with calls to the device's poll controller between
> attempts.  And hopefully, if the first attempt fails, we will still
> manage to get through?

Hopefully is not a good starting point. Is the poll controller
definitely retrying? Otherwise you might end up with the following:

Interrupt line is shared between your network device and a
device which requested a threaded interrupt handler.

  CPU0	       		   	    CPU1
  interrupt()
    your_device_handler()
      return NONE;
    shared_device_handler()
      return WAKE_THREAD;
      --> atomic_inc(threads_active);
				    poll()
				      disable_irq_nosleep()
					sync_hardirq()
					return atomic_read(threads_active);

So if you do not have a reliable retry then you might just go into a
stale state. And this can happen if the interrupt type is edge because
we do not disable the interrupt when we wakeup the thread for obvious
reasons.

Aside of that I think that something like this is a reasonable
approach to the problem.

The only other nitpicks I have are:

    - The name of the function sucks, though my tired braain can't
      come up with something reasonable right now

    - The lack of extensive documentation how this interface is
      supposed to be used and the pitfals of abusage, both in the
      function documentation and the changelog.

      Merlily copying the existing documentation of the other
      interface is not sufficient.

Thanks,

	tglx

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