netdev - Re: [PATCH RFC] r8169: straighten out overlength frame detection

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: <20091229012046.GD7037@localhost.localdomain>
Date:	Mon, 28 Dec 2009 20:20:46 -0500
From:	Neil Horman <nhorman@...driver.com>
To:	David Dillow <dave@...dillows.org>
Cc:	François romieu <romieu@...eil.com>,
	netdev@...r.kernel.org, davem@...emloft.net,
	eric.dumazet@...il.com, nhorman@...hat.com
Subject: Re: [PATCH RFC] r8169: straighten out overlength frame detection

On Mon, Dec 28, 2009 at 07:24:51PM -0500, David Dillow wrote:
> On Mon, 2009-12-28 at 22:31 +0100, François romieu wrote:
> > (I'm back)
> > 
> > The Mon, Dec 28, 2009 at 02:50:53PM -0500, Neil Horman wrote :
> > [...]
> > I doubt that we will be able to allocate that much memory reliably for long.
> > 
> > I'd rather go for static buffers + copy (+ src mac address of our new friend).
> 
> The driver doesn't support fragmented receives, but it seems like the HW
> does -- is this known to work on the different revisions? Or
> alternatively, is it known to be broken on any of them? It seems like it
> would be preferable to implement this and use RxMaxSize to tell the NIC
> how big the allocated buffer fragments are.
> 
> Or I'm misreading the capabilities of the NIC?
> 
> In any event, I'm probably not going to have time to write/test it on
> the HW I have anytime soon, so perhaps the static buffers/copy is the
> safest option for now, albeit non-optimal.
> 
Agreed, very non-optimal, but the only safe solution, if the rx size register
can't be trusted.  

What about this solution?  Its the same as before, but it takes Francois'
thoughts into account.  It ups the default copybreak value to 16383 so that all
the recevied frames should succede in try_rx_copy.  Again, completely untested,
but it compiles.  If theres some way to identify hardware subclasses that are
affected by this issues, you can restrict the affect by chaning the RxMaxSize
and copybreak in the probe routine.

Regards
Neil

Signed-off-by: Neil Horman <nhorman@...driver.com>


 r8169.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)


diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 60f96c4..c4e331c 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -63,6 +63,7 @@ static const int multicast_filter_limit = 32;
 #define RX_DMA_BURST	6	/* Maximum PCI burst, '6' is 1024 */
 #define TX_DMA_BURST	6	/* Maximum PCI burst, '6' is 1024 */
 #define EarlyTxThld	0x3F	/* 0x3F means NO early transmit */
+#define RxPacketMaxSize	0x3FE8	/* 16K - 1 - ETH_HLEN - VLAN - CRC... */
 #define SafeMtu		0x1c20	/* ... actually life sucks beyond ~7k */
 #define InterFrameGap	0x03	/* 3 means InterFrameGap = the shortest one */
 
@@ -186,7 +187,7 @@ static struct pci_device_id rtl8169_pci_tbl[] = {
 
 MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl);
 
-static int rx_copybreak = 200;
+static int rx_copybreak = 16383;
 static int use_dac;
 static struct {
 	u32 msg_enable;
@@ -3383,7 +3384,7 @@ static u16 rtl_rw_cpluscmd(void __iomem *ioaddr)
 static void rtl_set_rx_max_size(void __iomem *ioaddr, unsigned int rx_buf_sz)
 {
 	/* Low hurts. Let's disable the filtering. */
-	RTL_W16(RxMaxSize, rx_buf_sz + 1);
+	RTL_W16(RxMaxSize, 16383);
 }
 
 static void rtl8169_set_magic_reg(void __iomem *ioaddr, unsigned mac_version)
@@ -3430,7 +3431,7 @@ static void rtl_hw_start_8169(struct net_device *dev)
 
 	RTL_W8(EarlyTxThres, EarlyTxThld);
 
-	rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz);
+	rtl_set_rx_max_size(ioaddr, 16383);
 
 	if ((tp->mac_version == RTL_GIGA_MAC_VER_01) ||
 	    (tp->mac_version == RTL_GIGA_MAC_VER_02) ||
@@ -3691,7 +3692,7 @@ static void rtl_hw_start_8168(struct net_device *dev)
 
 	RTL_W8(EarlyTxThres, EarlyTxThld);
 
-	rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz);
+	rtl_set_rx_max_size(ioaddr, 16383);
 
 	tp->cp_cmd |= RTL_R16(CPlusCmd) | PktCntrDisable | INTT_1;
 
@@ -3871,7 +3872,7 @@ static void rtl_hw_start_8101(struct net_device *dev)
 
 	RTL_W8(EarlyTxThres, EarlyTxThld);
 
-	rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz);
+	rtl_set_rx_max_size(ioaddr, 16383);
 
 	tp->cp_cmd |= rtl_rw_cpluscmd(ioaddr) | PCIMulRW;
 
@@ -3972,7 +3973,7 @@ static struct sk_buff *rtl8169_alloc_rx_skb(struct pci_dev *pdev,
 
 	pad = align ? align : NET_IP_ALIGN;
 
-	skb = netdev_alloc_skb(dev, rx_buf_sz + pad);
+	skb = netdev_alloc_skb(dev, 16383 + pad);
 	if (!skb)
 		goto err_out;
 
--
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