netdev - Re: Realtek 8139 problem on 486.

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: <CAK8P3a3W=njxgG-GzpW=XkcLjuo0-xsbzhZRcAYvRYT_S0uwsw@mail.gmail.com>
Date:   Sun, 13 Jun 2021 23:52:38 +0200
From:   Arnd Bergmann <arnd@...nel.org>
To:     Nikolai Zhubr <zhubr.2@...il.com>
Cc:     netdev <netdev@...r.kernel.org>
Subject: Re: Realtek 8139 problem on 486.

On Sun, Jun 13, 2021 at 4:00 PM Nikolai Zhubr <zhubr.2@...il.com> wrote:
>
> 13.06.2021 1:41, Arnd Bergmann:
> > Or, to keep the change simpler, keep the inner loop in the tx
> > and rx processing, doing all rx events before moving on
> > to processing all tx events, but then looping back to try both
> > again, until either the budget runs out or no further events
> > are pending.
>
> Ok, made a new version: https://pastebin.com/3FUUrg7C
> It is much simpler and is very close to your patch now.
>
> All previous conditional defines are eliminated along with unnecessary
> code fragments, and here is TUNE8139_BIG_LOOP to introduce a top-level
> loop in poll function as you suggested above. But apparently it works
> well both with and without this loop. At least my testing did not show
> any substantial difference in performance. Therefore I think it could be
> completely removed for the sake of simplicity.

Ok, simpler is better in that case.

> One problem though is the kernel now always throws a traceback shortly
> after communication start:
> https://pastebin.com/VhwQ8wsU
> According to system.map it likely points to __local_bh_endble_ip() and
> there is one WARN_ON_ONCE() in it indeed, but I have no idea what it is
> and how to fix it.

There must be some call to spin_unlock_bh() or local_bh_enabled() or similar,
which is not allowed when interrupts are disabled with spin_lock_irqsave().

I don't see where exactly happens, but since nothing interesting is now
executed in hardirq context, I assume you can change tp->lock from
being called with _irq or _irqsave to using the _bh version that just
blocks the poll and start_xmit functions from happening, not the
hardirq.

> Yet another thing is that tp->rx_lock and tp->lock are now used within
> poll function in a way that possibly suggests one of them could be
> eliminated.

Agreed. I didn't want to change too much in my proposal, but I'm
sure these can be merged into a single lock, or possibly even
eliminated entirely.

        Arnd