netdev - Re: rtl8168e-vl dropping tftp ack

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]
Date:	Thu, 25 Apr 2013 12:56:30 +0200
From:	Stefan Bader <stefan.bader@...onical.com>
To:	hayeswang <hayeswang@...ltek.com>
CC:	'Francois Romieu' <romieu@...zoreil.com>, netdev@...r.kernel.org,
	'nic_swsd' <nic_swsd@...ltek.com>
Subject: Re: rtl8168e-vl dropping tftp ack

On 19.04.2013 09:49, hayeswang wrote:

>> I don't get it: arp aside, the normal trace in the capture 
>> file exhibits no
>> sub-60 bytes packet. Could you reformulate ?
>>
> 
> In brief, when the packet < 60, that is skb->len < 60, the hw should pad the
> packet to 60 bytes automatically. However, in my memory, the rtl8168e-vl
> wouldn't do this, and the packet wouldn't be sent. Therefore, the patch would be
> similar with the followings.
> 
> --- r8169.c.org 2013-04-19 22:35:40.785759473 +0800
> +++ r8169.c     2013-04-19 22:38:24.227189535 +0800
> @@ -5760,12 +5760,29 @@ static inline void rtl8169_tso_csum(stru
>         } else if (skb->ip_summed == CHECKSUM_PARTIAL) {
>                 const struct iphdr *ip = ip_hdr(skb);
> 
> +               if (unlikely(skb->len < ETH_ZLEN &&
> +                   (tp->mac_version == RTL_GIGA_MAC_VER_34))) {
> +                       if (skb_padto(skb, ETH_ZLEN))
> +                               return false;
> +                       skb_checksum_help(skb);
> +                       skb_put(skb, ETH_ZLEN - skb->len);
> +                       return true;
> +               }
> +
>                 if (ip->protocol == IPPROTO_TCP)
>                         opts[offset] |= info->checksum.tcp;
>                 else if (ip->protocol == IPPROTO_UDP)
>                         opts[offset] |= info->checksum.udp;
>                 else
>                         WARN_ON_ONCE(1);
> +       } else {
> +               if (unlikely(skb->len < ETH_ZLEN &&
> +                   (tp->mac_version == RTL_GIGA_MAC_VER_34))) {
> +                       if (skb_padto(skb, ETH_ZLEN))
> +                               return false;
> +                       skb_put(skb, ETH_ZLEN - skb->len);
> +                       return true;

Hardware is now back and I ran a couple of tests. First, the crash I ran into
previously was because I accidentally slipped in a skp_checksum_help into the
else case that Hayes proposed. Sorry.

So with the right version and both hunks in rtl8169_tso_csum, I see the problem
go away. I do observe the else case being hit on other occasions (not only while
doing pxe boots) from time to time. So that may happen more often than it causes
visible problems. I never saw the checksum case being hit.

Naively assuming that the checksum path never gets executed (which may be wrong)
I condensed the changes to:

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek
index 4ecbe64..1519a2f 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5790,12 +5790,23 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *sk
        if (unlikely(le32_to_cpu(txd->opts1) & DescOwn))
                goto err_stop_0;

+       /*
+        * 8168E-VL hardware does not automatically pad to minimum
+        * length.
+        */
+       if (unlikely(skb->len < ETH_ZLEN &&
+           (tp->mac_version == RTL_GIGA_MAC_VER_34))) {
+               if (skb_padto(skb, ETH_ZLEN))
+                       goto err_update_stats_0;
+               skb_put(skb, ETH_ZLEN - skb->len);
+       }
+
        len = skb_headlen(skb);
        mapping = dma_map_single(d, skb->data, len, DMA_TO_DEVICE);
        if (unlikely(dma_mapping_error(d, mapping))) {
                if (net_ratelimit())
                        netif_err(tp, drv, dev, "Failed to map TX DMA!\n");
-               goto err_dma_0;
+               goto err_free_skb_1;
        }

        tp->tx_skb[entry].len = len;
@@ -5808,7 +5819,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,

        frags = rtl8169_xmit_frags(tp, skb, opts);
        if (frags < 0)
-               goto err_dma_1;
+               goto err_unmap_dma_1;
        else if (frags)
                opts[0] |= FirstFrag;
        else {
@@ -5854,10 +5865,11 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *sk

        return NETDEV_TX_OK;

-err_dma_1:
+err_unmap_dma_1:
        rtl8169_unmap_tx_skb(d, tp->tx_skb + entry, txd);
-err_dma_0:
+err_free_skb_1:
        dev_kfree_skb(skb);
+err_update_stats_0:
        dev->stats.tx_dropped++;
        return NETDEV_TX_OK;


This variant also seems to work (and does not drop the tftp packet). Whichever
path looks more suitable to you, you could add my tested-by to both.
We should also flag whichever change results from this as stable material as I
could see this happen even in v3.2 (just did not try further back).

Thanks,
Stefan

> +               }
>         }
>  }
> 



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