[patch linux-2.6.11-bk10 1/1] r8169: incoming frame length check

[patch linux-2.6.11-bk10 1/1] r8169: incoming frame length check

[Francois Romieu]

The size of the incoming frame is not correctly checked.

The RxMaxSize register (0xDA) does not work as expected and incoming
frames whose size exceeds the MTU actually end spanning multiple
descriptors. The first Rx descriptor contains the size of the whole
frame (or some garbage in its place). The driver does not expect 
something above the space allocated to the current skb and crashes
loudly when it issues a skb_put.

The fix contains two parts:
- disable hardware Rx size filtering: so far it only proved to be able
  to trigger some new fancy errors;
- drop multi-descriptors frame: as the driver allocates MTU sized Rx
  buffers, it provides an adequate filtering.

As a bonus, wrong descriptors were not returned to the asic after their
processing.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>

diff -puN drivers/net/r8169.c~r8169-570 drivers/net/r8169.c
--- linux-2.6.11/drivers/net/r8169.c~r8169-570	2005-03-13 23:35:37.000000000 +0100
+++ linux-2.6.11-fr/drivers/net/r8169.c	2005-03-14 23:47:56.529014957 +0100
@@ -1585,8 +1585,8 @@ rtl8169_hw_start(struct net_device *dev)
 	RTL_W8(ChipCmd, CmdTxEnb | CmdRxEnb);
 	RTL_W8(EarlyTxThres, EarlyTxThld);
 
-	/* For gigabit rtl8169, MTU + header + CRC + VLAN */
-	RTL_W16(RxMaxSize, tp->rx_buf_sz);
+	/* Low hurts. Let's disable the filtering. */
+	RTL_W16(RxMaxSize, 16383);
 
 	/* Set Rx Config register */
 	i = rtl8169_rx_config |
@@ -2127,6 +2127,11 @@ rtl8169_tx_interrupt(struct net_device *
 	}
 }
 
+static inline int rtl8169_fragmented_frame(u32 status)
+{
+	return (status & (FirstFrag | LastFrag)) != (FirstFrag | LastFrag);
+}
+
 static inline void rtl8169_rx_csum(struct sk_buff *skb, struct RxDesc *desc)
 {
 	u32 opts1 = le32_to_cpu(desc->opts1);
@@ -2177,27 +2182,41 @@ rtl8169_rx_interrupt(struct net_device *
 
 	while (rx_left > 0) {
 		unsigned int entry = cur_rx % NUM_RX_DESC;
+		struct RxDesc *desc = tp->RxDescArray + entry;
 		u32 status;
 
 		rmb();
-		status = le32_to_cpu(tp->RxDescArray[entry].opts1);
+		status = le32_to_cpu(desc->opts1);
 
 		if (status & DescOwn)
 			break;
 		if (status & RxRES) {
-			printk(KERN_INFO "%s: Rx ERROR!!!\n", dev->name);
+			printk(KERN_INFO "%s: Rx ERROR. status = %08x\n",
+			       dev->name, status);
 			tp->stats.rx_errors++;
 			if (status & (RxRWT | RxRUNT))
 				tp->stats.rx_length_errors++;
 			if (status & RxCRC)
 				tp->stats.rx_crc_errors++;
+			rtl8169_mark_to_asic(desc, tp->rx_buf_sz);
 		} else {
-			struct RxDesc *desc = tp->RxDescArray + entry;
 			struct sk_buff *skb = tp->Rx_skbuff[entry];
 			int pkt_size = (status & 0x00001FFF) - 4;
 			void (*pci_action)(struct pci_dev *, dma_addr_t,
 				size_t, int) = pci_dma_sync_single_for_device;
 
+			/*
+			 * The driver does not support incoming fragmented
+			 * frames. They are seen as a symptom of over-mtu
+			 * sized frames.
+			 */
+			if (unlikely(rtl8169_fragmented_frame(status))) {
+				tp->stats.rx_dropped++;
+				tp->stats.rx_length_errors++;
+				rtl8169_mark_to_asic(desc, tp->rx_buf_sz);
+				goto move_on;
+			}
+
 			rtl8169_rx_csum(skb, desc);
 			
 			pci_dma_sync_single_for_cpu(tp->pci_dev,
@@ -2224,7 +2243,7 @@ rtl8169_rx_interrupt(struct net_device *
 			tp->stats.rx_bytes += pkt_size;
 			tp->stats.rx_packets++;
 		}
-		
+move_on:		
 		cur_rx++; 
 		rx_left--;
 	}

_

Re: [patch linux-2.6.11-bk10 1/1] r8169: incoming frame length check

[Jon Mason]

On Monday 14 March 2005 05:31 pm, Francois Romieu wrote:
> The size of the incoming frame is not correctly checked.
>
> The RxMaxSize register (0xDA) does not work as expected and incoming
> frames whose size exceeds the MTU actually end spanning multiple
> descriptors. The first Rx descriptor contains the size of the whole
> frame (or some garbage in its place). The driver does not expect
> something above the space allocated to the current skb and crashes
> loudly when it issues a skb_put.
>
> The fix contains two parts:
> - disable hardware Rx size filtering: so far it only proved to be able
>   to trigger some new fancy errors;
> - drop multi-descriptors frame: as the driver allocates MTU sized Rx
>   buffers, it provides an adequate filtering.
>
> As a bonus, wrong descriptors were not returned to the asic after their
> processing.
>
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>

Applies cleanly and lightly tested on amd64 (kernel version 2.6.11.3).

> diff -puN drivers/net/r8169.c~r8169-570 drivers/net/r8169.c
> --- linux-2.6.11/drivers/net/r8169.c~r8169-570 2005-03-13
> 23:35:37.000000000 +0100 +++ linux-2.6.11-fr/drivers/net/r8169.c 2005-03-14
> 23:47:56.529014957 +0100 @@ -1585,8 +1585,8 @@ rtl8169_hw_start(struct
> net_device *dev)
>   RTL_W8(ChipCmd, CmdTxEnb | CmdRxEnb);
>   RTL_W8(EarlyTxThres, EarlyTxThld);
>
> - /* For gigabit rtl8169, MTU + header + CRC + VLAN */
> - RTL_W16(RxMaxSize, tp->rx_buf_sz);
> + /* Low hurts. Let's disable the filtering. */
> + RTL_W16(RxMaxSize, 16383);

Wouldn't a #define be better than 16k-1?  Perhaps, RxPacketMaxSize could be 
used.

>
>   /* Set Rx Config register */
>   i = rtl8169_rx_config |
> @@ -2127,6 +2127,11 @@ rtl8169_tx_interrupt(struct net_device *
>   }
>  }
>
> +static inline int rtl8169_fragmented_frame(u32 status)
> +{
> + return (status & (FirstFrag | LastFrag)) != (FirstFrag | LastFrag);
> +}
> +
>  static inline void rtl8169_rx_csum(struct sk_buff *skb, struct RxDesc
> *desc) {
>   u32 opts1 = le32_to_cpu(desc->opts1);
> @@ -2177,27 +2182,41 @@ rtl8169_rx_interrupt(struct net_device *
>
>   while (rx_left > 0) {
>    unsigned int entry = cur_rx % NUM_RX_DESC;
> +  struct RxDesc *desc = tp->RxDescArray + entry;
>    u32 status;
>
>    rmb();
> -  status = le32_to_cpu(tp->RxDescArray[entry].opts1);
> +  status = le32_to_cpu(desc->opts1);
>
>    if (status & DescOwn)
>     break;
>    if (status & RxRES) {
> -   printk(KERN_INFO "%s: Rx ERROR!!!\n", dev->name);
> +   printk(KERN_INFO "%s: Rx ERROR. status = %08x\n",
> +          dev->name, status);
>     tp->stats.rx_errors++;
>     if (status & (RxRWT | RxRUNT))
>      tp->stats.rx_length_errors++;
>     if (status & RxCRC)
>      tp->stats.rx_crc_errors++;
> +   rtl8169_mark_to_asic(desc, tp->rx_buf_sz);
>    } else {
> -   struct RxDesc *desc = tp->RxDescArray + entry;
>     struct sk_buff *skb = tp->Rx_skbuff[entry];
>     int pkt_size = (status & 0x00001FFF) - 4;
>     void (*pci_action)(struct pci_dev *, dma_addr_t,
>      size_t, int) = pci_dma_sync_single_for_device;
>
> +   /*
> +    * The driver does not support incoming fragmented
> +    * frames. They are seen as a symptom of over-mtu
> +    * sized frames.
> +    */
> +   if (unlikely(rtl8169_fragmented_frame(status))) {
> +    tp->stats.rx_dropped++;
> +    tp->stats.rx_length_errors++;
> +    rtl8169_mark_to_asic(desc, tp->rx_buf_sz);
> +    goto move_on;
> +   }
> +
>     rtl8169_rx_csum(skb, desc);
>
>     pci_dma_sync_single_for_cpu(tp->pci_dev,
> @@ -2224,7 +2243,7 @@ rtl8169_rx_interrupt(struct net_device *
>     tp->stats.rx_bytes += pkt_size;
>     tp->stats.rx_packets++;
>    }
> -
> +move_on:
>    cur_rx++;
>    rx_left--;
>   }
>
> _

Looks similar to some of the > 8k Jumbo Frames changes.  Should make the diff 
for next patch much cleaner.  Thanks.

-- 
Jon Mason
jdmason@us.ibm.com

Re: [patch linux-2.6.11-bk10 1/1] r8169: incoming frame length check

[Francois Romieu]

Jon Mason <jdmason@us.ibm.com> :
[...]
> > - /* For gigabit rtl8169, MTU + header + CRC + VLAN */
> > - RTL_W16(RxMaxSize, tp->rx_buf_sz);
> > + /* Low hurts. Let's disable the filtering. */
> > + RTL_W16(RxMaxSize, 16383);
> 
> Wouldn't a #define be better than 16k-1?  Perhaps, RxPacketMaxSize could be 
> used.

RxPacketMaxSize does not tell a lot more. It is (imho) still a bit too neutral
as something badly sucks when you try to filter the packet size.

I'll welcome suggestions for something which summarizes "The maximal jumbo
frame size as it is actually needed to disable the filtering".

On a different topic: Mark and I tested the patch as well. Even if it is not
exactly trivial, it could make sense to push it for submission on 2.6.11.x.
Without it one do not want to be on a public Gb link (sic).

Executive summary:
- (patch already included in 2.6.11.x): the Rx ring occasionally announced
  wrong size for the Rx DMA buffers;

- (current patch): the packet length indication in a descriptor after
  Rx DMA can be related to a packet which spans several descriptors. So it
  is inaccurate for skb_put.

--
Ueimor

Re: [patch linux-2.6.11-bk10 1/1] r8169: incoming frame length check

[Jeff Garzik]

applied, let me know when to send upstream.

Re: [patch linux-2.6.11-bk10 1/1] r8169: incoming frame length check

[Francois Romieu]

Jeff Garzik <jgarzik@pobox.com> :
> applied, let me know when to send upstream.

Feel free to push it upstream when Penguin #0 is back.

--
Ueimor