netdev - Re: [PATCH 04/10] net/macb: Fix a race in macb_start

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACiLriTSddM-5pnpii9c+4dA++OrsKovGLabegdwSRLchuSnAA@mail.gmail.com>
Date:	Thu, 6 Sep 2012 17:49:11 +0200
From:	Havard Skinnemoen <havard@...nnemoen.net>
To:	David Miller <davem@...emloft.net>
Cc:	nicolas.ferre@...el.com, netdev@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, plagnioj@...osoft.com,
	jamie@...ieiles.com, linux-kernel@...r.kernel.org,
	patrice.vilchez@...el.com
Subject: Re: [PATCH 04/10] net/macb: Fix a race in macb_start_xmit()

On Wed, Sep 5, 2012 at 11:30 PM, David Miller <davem@...emloft.net> wrote:
> From: Nicolas Ferre <nicolas.ferre@...el.com>
> Date: Wed, 5 Sep 2012 10:19:11 +0200
>
>> From: Havard Skinnemoen <havard@...nnemoen.net>
>>
>> Fix a race in macb_start_xmit() where we unconditionally set the TSTART bit.
>> If an underrun just happened (we do this with interrupts disabled, so it might
>> not have been handled yet), the controller starts transmitting from the first
>> entry in the ring, which is usually wrong.
>> Restart the controller after error handling.
>>
>> Signed-off-by: Havard Skinnemoen <havard@...nnemoen.net>
>> [nicolas.ferre@...el.com: split patch in topics]
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@...el.com>
>
> Accumulating special case code and checks into the hot path of TX packet
> processing is extremely unwise.
>
> Instead, when you handle the TX error conditions and reset the chip you
> should first ensure that there are no flows of control in the transmit
> function of your driver by using the appropriate locking et al. facilities.

IIRC, the hardware resets the ring pointers when an error happens, and
if we set TSTART right after that happens, the hardware will happily
transmit whatever is sitting in the beginning of the ring. This is
what I was trying to avoid.

The details are a bit hazy as it's been a while since I looked at
this, so it could be that simply letting it happen and using a bigger
hammer during reset processing might work just as well. Just want to
make sure y'all understand that we're talking about a race against
hardware, not against interrupt handlers, threads or anything that can
be solved by locking :)

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