netdev
[Top] [All Lists]

Re: [PATCH] forcedeth: fix random memory scribbling bug

To: Jeff Garzik <jgarzik@xxxxxxxxx>
Subject: Re: [PATCH] forcedeth: fix random memory scribbling bug
From: Manfred Spraul <manfred@xxxxxxxxxxxxxxxx>
Date: Sat, 24 Dec 2005 17:08:10 +0100
Cc: Linus Torvalds <torvalds@xxxxxxxx>, Ayaz Abdulla <AAbdulla@xxxxxxxxxx>, Linux Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>, Netdev <netdev@xxxxxxxxxxx>
In-reply-to: <43AD64AB.2070306@xxxxxxxxx>
References: <43AD4ADC.8050004@xxxxxxxxxxxxxxxx> <43AD64AB.2070306@xxxxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux i686; fr-FR; rv:1.7.12) Gecko/20050923 Fedora/1.7.12-1.5.1
Jeff Garzik wrote:

Manfred Spraul wrote:

Two critical bugs were found in forcedeth 0.47:
- TSO doesn't work.
- pci_map_single() for the rx buffers is called with size==0. This bug is critical, it causes random memory corruptions on systems with an iommu.

Below is a minimal fix for both bugs, for inclusion into 2.6.15.
TSO will be fixed properly in the next version.
Tested on x86-64.

Signed-Off-By: Manfred Spraul <manfred@xxxxxxxxxxxxxxxx>


1) Why does forcedeth require a non-standard calculation for each pci_map_single() call?

- skb->len is the wrong thing (tm), since it's 0 until skb_put().
- I have not found a field that contains the actual size of the data area of an skb.
- the results must be identical for map and unmap.
- I could recalculate the size of the allocation from np->rx_buf_sz, but I don't like that. Right now it would work, but it's too subtile that changing rx_buf_sz while there are outstanding rx buffers results in a iommu memory leak. Therefore I decided to calculate the mapping size with "skb->end - skb->data": The size of the mapping for an skb is calculated by looking at fields in the skb, no knowledge about driver fields.

2) I have requested multiple times that you avoid MIME...

It's the first time that you complain about Content-Transfer-Encoding: 7bit attachments.

3) Why disable TSO completely? It sounds like it should default to off, then permit enabling via ethtool.

The bugfix is in 0.49 - it's just a bit larger, I would consider it for 2.5.16.

--
   Manfred

<Prev in Thread] Current Thread [Next in Thread>