netdev - Re: [PATCH] net: mvneta: fix refilling for Rx DMA buffers

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: <20150716002530.GD10671@kw.sim.vm.gnt>
Date:	Thu, 16 Jul 2015 02:25:30 +0200
From:	Simon Guinot <simon.guinot@...uanux.org>
To:	David Miller <davem@...emloft.net>
Cc:	thomas.petazzoni@...e-electrons.com, jason@...edaemon.net,
	andrew@...n.ch, gregory.clement@...e-electrons.com,
	sebastian.hesselbarth@...il.com, netdev@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, vdonnefort@...il.com,
	yoann@...lo.fr, stable@...r.kernel.org
Subject: Re: [PATCH] net: mvneta: fix refilling for Rx DMA buffers

On Wed, Jul 15, 2015 at 03:52:56PM -0700, David Miller wrote:
> From: Simon Guinot <simon.guinot@...uanux.org>
> Date: Mon, 13 Jul 2015 00:04:57 +0200
> 
> > On some Armada 370-based NAS, I have experimented kernel bugs and
> > crashes when the mvneta Ethernet driver fails to refill Rx DMA buffers
> > due to memory shortage.
> > 
> > With the actual code, if the memory allocation fails while refilling a
> > Rx DMA buffer, then the corresponding descriptor is let with the address
> > of an unmapped DMA buffer already passed to the network stack. Since the
> > driver still increments the non-occupied counter for Rx descriptor (if a
> > next packet is handled successfully), then the Ethernet controller is
> > allowed to reuse the unfilled Rx descriptor...
> > 
> > As a fix, this patch first refills a Rx descriptor before handling the
> > stored data and unmapping the associated Rx DMA buffer. Additionally the
> > occupied and non-occupied counters for the Rx descriptors queues are now
> > both updated with the rx_done value: the number of descriptors ready to
> > be returned to the networking controller. Moreover note that there is no
> > point in using different values for this counters because both the
> > mvneta driver and the Ethernet controller are unable to handle holes in
> > the Rx descriptors queues.
> > 
> > Signed-off-by: Simon Guinot <simon.guinot@...uanux.org>
> > Fixes: c5aff18204da ("net: mvneta: driver for Marvell Armada 370/XP network unit")
> > Cc: <stable@...r.kernel.org> # v3.8+
> > Tested-by: Yoann Sculo <yoann@...lo.fr>

Hi David,

> 
> Failed memory allocations, are normal and if necessary kernel log
> messages will be triggered by the core memory allocator.  So there is
> zero reason to do anything other than bump the drop counter in your
> driver.

Sure.

> 
> If I understand the rest of your logic, if the allocator fails, we
> will reuse the original SKB and place it back into the RX ring?
> Right?

Yes that's what does the patch.

Without this patch, in case of memory allocation error, the original
buffer is both passed to the networking stack in a SKB _and_ let in
the Rx ring. This leads to various kernel oops and crashes.

Here are the problems with the original code:

- Rx descriptor refilling is tried after passing the SKB to the
  networking stack.
- In case of refilling error, the buffer (passed in the SKB) is also
  let in the Rx ring (as an unmapped DMA buffer).
- And (always in case of refilling error), the non-occupied counter of
  the Rx queue (hardware register) is not incremented. The idea was
  maybe to prevent the networking controller to reuse the non-refilled
  descriptor. But since this counter is incremented as soon as a next
  descriptor is handled successfully, it is not correct.

The patch:

- Moves Rx descriptor refilling ahead of passing the SKB to the
  networking stack.
- places the buffer back into the Rx ring in case of refilling error.
- And updates correctly the non-occupied descriptors counter of the Rx
  queue.

Simon

Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)