netdev - Re: [PATCH] netpoll: Don't call driver methods from interrupt context.

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: <87ppm2fe8q.fsf@xmission.com>
Date:	Tue, 04 Mar 2014 02:29:57 -0800
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Cong Wang <xiyou.wangcong@...il.com>
Cc:	David Miller <davem@...emloft.net>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	Matt Mackall <mpm@...enic.com>,
	Satyam Sharma <satyam.sharma@...il.com>
Subject: Re: [PATCH] netpoll: Don't call driver methods from interrupt context.

Cong Wang <xiyou.wangcong@...il.com> writes:

> On Mon, Mar 3, 2014 at 12:40 PM, Eric W. Biederman
> <ebiederm@...ssion.com> wrote:
>>  net/core/netpoll.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
>> index a664f7829a6d..a1877621bf31 100644
>> --- a/net/core/netpoll.c
>> +++ b/net/core/netpoll.c
>> @@ -330,7 +330,7 @@ repeat:
>>                 skb = skb_dequeue(&skb_pool);
>>
>>         if (!skb) {
>> -               if (++count < 10) {
>> +               if (++count < 10 && !in_irq()) {
>>                         netpoll_poll_dev(np->dev);
>
> This looks like a workaround.

It is not a workaround.  It is a neutering of the netpoll code (when
run in irq context) to just allocate memory for the message we are going
to send and queueing that message for delivery in a safer context.

> Here ou are trying to avoid calling netpoll_poll_dev()
> in IRQ context, but it has a side effect for netpoll_send_udp()
> which could possibly return early after find_skb().

It isn't a side effect, it is an unfortuante fact of life that sometimes
you can not allocate memory in irq context.

> Also, netpoll_poll_dev() does more than just calling driver
> poll method, I am not sure if it is safe to skip it either.

netpoll_poll_dev is absolutely safe to skip at this location because
it is not called at this location 99% of the time.

Also note my patch is about much much more than not calling
the driver's napi poll method from interrupt context.  It is about
not calling any driver method from interrupt context.  This includes
ndo_poll_controller, and ndo_start_xmit.

The only work netpoll_poll_dev does that does not call into driver
methods is zap_completion_queues and find_skb has already called
zap_completion_queues.  Which means even a more fine grained aproach
could not find code in netpoll_poll_dev that is desirable to call in
interrupt context.

I expect you were referring to netpoll_neigh_reply and there are issues
with that code.

Semantically netpoll_neigh_reply is a path into the driver methods for
sending packets and as such is not appropriate to call from interrupt
context.

Practically speaking netpoll_neigh_reply is dead code because there
is not a single netpoll user in the kernel that sets rx_skb_hook.

Functionally netpoll_neigh_reply scares me to read.  It has the
potential to infinitely recurse and unless I am missing some deep magic
it leaks every packet that makes it on to the neigh_tx queue.

The code path that can has the potential to infinitely recurse is:
find_skb
   netpoll_poll_dev
      service_neigh_queue
         netpoll_neigh_reply
             find_skb
                ...

It is my personal recommendation that all support for receiving packets
in netpoll be removed.  It has been a decade and we still have yet to
see a user of that code merged into the tree, and the code is extremely
fishy if not totally horrifically broken.

> netpoll code needs to rewrite.

I don't have a clue what you mean there.  My guess is you are saying
netpoll need a rewrite.  After having read through the netpoll code I
would argue that my two line change is the rewrite the netpoll code
needs.  (Well other than dead code removal).

What is desired to do in interrupt context is to allocate a buffer, put
our data in it, and queue that buffer to be handled later.  That is
exactly what happens with my changes when the code is run in interrupt
context.

Better than that I have tested my changes, and the code works.

In this instance I got lucky that everything netpoll needed to do to
handle being called from interrupt context was already present in the
code and I just needed to tell the netpoll code to use those other paths
in interrupt context.

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