netdev - Re: [PATCH net] net: usbnet: fix softirq storm on suspend

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, 04 Sep 2012 09:49:14 +0200
From:	Bjørn Mork <bjorn@...k.no>
To:	Ming Lei <ming.lei@...onical.com>
Cc:	netdev@...r.kernel.org, linux-usb@...r.kernel.org,
	Oliver Neukum <oneukum@...e.de>
Subject: Re: [PATCH net] net: usbnet: fix softirq storm on suspend

Ming Lei <ming.lei@...onical.com> writes:
> On Mon, Sep 3, 2012 at 4:26 PM, Bjørn Mork <bjorn@...k.no> wrote:
>
>> Sorry for not noticing this before, but commit 65841fd5
>> makes usbnet autosuspend completely unusable.  The device
>> is suspended fine, but burning one CPU core at full load
>> uses a tiny bit more power making the power saving
>> negative...
>
> I am wondering how you can reproduce the issue.

That's easy:

- Take any usbnet device supporting remote wakeup (and of course with a
  minidriver supporting it as well),
- enable autosuspend,
- ip link set dev ethX/usbX/wwanX up

And watch ksoftirqd/X use 100% of one of your CPU cores.

I'd very much like to hear the details if you are unable to reproduce
the bug using this simple test.

> usbnet_terminate_urbs is called inside usbnet_suspend to
> consume all URBs and SKBs, and rx_alloc_submit can't be
> called during the period because of !netif_device_present().

Huh???? We do support suspending a USB device while the network device
is up and running. That's the whole idea here, isn't it? I.e.
netif_device_present() is *expected* to be true while the USB device is
suspended.

> Once usbnet_terminate_urbs and netif_device_attach() are
> completed, who will schedule tasklet to trigger rx_alloc_submit?

That's a good question.  It sure would be nice if that never happened
without waking the device first.  But there are just too many call sites
for me to follow:

bjorn@...i:/usr/local/src/git/linux$ grep tasklet_schedule drivers/net/usb/usbnet.c
                tasklet_schedule(&dev->bh);
 * but tasklet_schedule() doesn't.  hope the failure is rare.
                        tasklet_schedule (&dev->bh);
        tasklet_schedule(&dev->bh);
                tasklet_schedule(&dev->bh);
        tasklet_schedule (&dev->bh);
                        tasklet_schedule (&dev->bh);
                                tasklet_schedule (&dev->bh);
        tasklet_schedule (&dev->bh);
                                tasklet_schedule (&dev->bh);
                        tasklet_schedule (&dev->bh);

And I do believe the code before your change demonstrated that the
original authors had the same view.  There was an explicit exception for
just this case, and I do assume that was put there for a good
reason. usbnet_bh() will be called while the device is suspended, and we
must avoid making it reschedule itself in this case.

Anyway, the ENOLINK test was there.  You removed it with no explanation
whatsoever. It is *your* call to verify and explain to us why this test
is unnecessary, not mine.

Bjørn
--
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