[PATCH net] r8169: work around RTL8125 UDP hw bug

It was reported that on RTL8125 network breaks under heavy UDP load,
e.g. torrent traffic ([0], from comment 27). Realtek confirmed a hw bug
and provided me with a test version of the r8125 driver including a
workaround. Tests confirmed that the workaround fixes the issue.
I modified the original version of the workaround to meet mainline
code style.

[0] https://bugzilla.kernel.org/show_bug.cgi?id=209839

Fixes: f1bce4ad2f1c ("r8169: add support for RTL8125")
Tested-by: xplo <redacted>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 64 ++++++++++++++++++++---
 1 file changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index fb67d8f79..90052033b 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c

@@ -28,6 +28,7 @@
 #include <linux/bitfield.h>
 #include <linux/prefetch.h>
 #include <linux/ipv6.h>
+#include <linux/ptp_classify.h>
 #include <asm/unaligned.h>
 #include <net/ip6_checksum.h>
 

@@ -4007,17 +4008,64 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
 	return -EIO;
 }
 
-static bool rtl_test_hw_pad_bug(struct rtl8169_private *tp)
+static bool rtl_skb_is_udp(struct sk_buff *skb)
 {
+	switch (vlan_get_protocol(skb)) {
+	case htons(ETH_P_IP):
+		return ip_hdr(skb)->protocol == IPPROTO_UDP;
+	case htons(ETH_P_IPV6):
+		return ipv6_hdr(skb)->nexthdr == IPPROTO_UDP;
+	default:
+		return false;
+	}
+}
+
+#define RTL_MIN_PATCH_LEN	47
+#define PTP_GEN_PORT		320
+
+/* see rtl8125_get_patch_pad_len() in r8125 vendor driver */
+static unsigned int rtl8125_quirk_udp_padto(struct rtl8169_private *tp,
+					    struct sk_buff *skb)
+{
+	unsigned int padto = 0, len = skb->len;
+
+	if (rtl_is_8125(tp) && len < 175 && rtl_skb_is_udp(skb) &&
+	    skb_transport_header_was_set(skb)) {
+		unsigned int trans_data_len = skb_tail_pointer(skb) -
+					      skb_transport_header(skb);
+
+		if (trans_data_len > 3 && trans_data_len < RTL_MIN_PATCH_LEN) {
+			u16 dest = ntohs(udp_hdr(skb)->dest);
+
+			if (dest == PTP_EV_PORT || dest == PTP_GEN_PORT)
+				padto = len + RTL_MIN_PATCH_LEN - trans_data_len;
+		}
+
+		if (trans_data_len < UDP_HLEN)
+			padto = max(padto, len + UDP_HLEN - trans_data_len);
+	}
+
+	return padto;
+}
+
+static unsigned int rtl_quirk_packet_padto(struct rtl8169_private *tp,
+					   struct sk_buff *skb)
+{
+	unsigned int padto;
+
+	padto = rtl8125_quirk_udp_padto(tp, skb);
+
 	switch (tp->mac_version) {
 	case RTL_GIGA_MAC_VER_34:
 	case RTL_GIGA_MAC_VER_60:
 	case RTL_GIGA_MAC_VER_61:
 	case RTL_GIGA_MAC_VER_63:
-		return true;
+		padto = max_t(unsigned int, padto, ETH_ZLEN);
 	default:
-		return false;
+		break;
 	}
+
+	return padto;
 }
 
 static void rtl8169_tso_csum_v1(struct sk_buff *skb, u32 *opts)

@@ -4089,9 +4137,10 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
 
 		opts[1] |= transport_offset << TCPHO_SHIFT;
 	} else {
-		if (unlikely(skb->len < ETH_ZLEN && rtl_test_hw_pad_bug(tp)))
-			/* eth_skb_pad would free the skb on error */
-			return !__skb_put_padto(skb, ETH_ZLEN, false);
+		unsigned int padto = rtl_quirk_packet_padto(tp, skb);
+
+		/* skb_padto would free the skb on error */
+		return !__skb_put_padto(skb, padto, false);
 	}
 
 	return true;

@@ -4268,6 +4317,9 @@ static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
 		if (skb->len < ETH_ZLEN)
 			features &= ~NETIF_F_CSUM_MASK;
 
+		if (rtl_quirk_packet_padto(tp, skb))
+			features &= ~NETIF_F_CSUM_MASK;
+
 		if (transport_offset > TCPHO_MAX &&
 		    rtl_chip_supports_csum_v2(tp))
 			features &= ~NETIF_F_CSUM_MASK;

-- 
2.30.0

On 26.01.2021 10:02, Heiner Kallweit wrote:

It was reported that on RTL8125 network breaks under heavy UDP load,
e.g. torrent traffic ([0], from comment 27). Realtek confirmed a hw bug
and provided me with a test version of the r8125 driver including a
workaround. Tests confirmed that the workaround fixes the issue.
I modified the original version of the workaround to meet mainline
code style.

[0] https://bugzilla.kernel.org/show_bug.cgi?id=209839

Fixes: f1bce4ad2f1c ("r8169: add support for RTL8125")
Tested-by: xplo <redacted>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---

Patch is against net-next, not net. I'll rebase and resubmit.

On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit [off-list ref] wrote:

This trusts that an skb with given skb->protocol is well behaved. With
packet sockets/tun/virtio, that may be false.

+       default:
+               return false;
+       }
+}
+
+#define RTL_MIN_PATCH_LEN      47
+#define PTP_GEN_PORT           320

Why the two PTP ports? The report is not PTP specific. Also, what does
patch mean in this context?

+
+/* see rtl8125_get_patch_pad_len() in r8125 vendor driver */
+static unsigned int rtl8125_quirk_udp_padto(struct rtl8169_private *tp,
+                                           struct sk_buff *skb)
+{
+       unsigned int padto = 0, len = skb->len;
+
+       if (rtl_is_8125(tp) && len < 175 && rtl_skb_is_udp(skb) &&
+           skb_transport_header_was_set(skb)) {

What is 175 here?

+               unsigned int trans_data_len = skb_tail_pointer(skb) -
+                                             skb_transport_header(skb);
+
+               if (trans_data_len > 3 && trans_data_len < RTL_MIN_PATCH_LEN) {

And 3 here, instead of sizeof(struct udphdr)

On 27.01.2021 19:07, Willem de Bruijn wrote:

On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit [off-list ref] wrote:

quoted

It was reported that on RTL8125 network breaks under heavy UDP load,
e.g. torrent traffic ([0], from comment 27). Realtek confirmed a hw bug
and provided me with a test version of the r8125 driver including a
workaround. Tests confirmed that the workaround fixes the issue.
I modified the original version of the workaround to meet mainline
code style.

[0] https://bugzilla.kernel.org/show_bug.cgi?id=209839

Fixes: f1bce4ad2f1c ("r8169: add support for RTL8125")
Tested-by: xplo <redacted>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 64 ++++++++++++++++++++---
 1 file changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index fb67d8f79..90052033b 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c

@@ -28,6 +28,7 @@
 #include <linux/bitfield.h>
 #include <linux/prefetch.h>
 #include <linux/ipv6.h>
+#include <linux/ptp_classify.h>
 #include <asm/unaligned.h>
 #include <net/ip6_checksum.h>

@@ -4007,17 +4008,64 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
        return -EIO;
 }

-static bool rtl_test_hw_pad_bug(struct rtl8169_private *tp)
+static bool rtl_skb_is_udp(struct sk_buff *skb)
 {
+       switch (vlan_get_protocol(skb)) {
+       case htons(ETH_P_IP):
+               return ip_hdr(skb)->protocol == IPPROTO_UDP;
+       case htons(ETH_P_IPV6):
+               return ipv6_hdr(skb)->nexthdr == IPPROTO_UDP;

This trusts that an skb with given skb->protocol is well behaved. With
packet sockets/tun/virtio, that may be false.

quoted

+       default:
+               return false;
+       }
+}
+
+#define RTL_MIN_PATCH_LEN      47
+#define PTP_GEN_PORT           320

Why the two PTP ports? The report is not PTP specific. Also, what does
patch mean in this context?

quoted

+
+/* see rtl8125_get_patch_pad_len() in r8125 vendor driver */
+static unsigned int rtl8125_quirk_udp_padto(struct rtl8169_private *tp,
+                                           struct sk_buff *skb)
+{
+       unsigned int padto = 0, len = skb->len;
+
+       if (rtl_is_8125(tp) && len < 175 && rtl_skb_is_udp(skb) &&
+           skb_transport_header_was_set(skb)) {

What is 175 here?

quoted

+               unsigned int trans_data_len = skb_tail_pointer(skb) -
+                                             skb_transport_header(skb);
+
+               if (trans_data_len > 3 && trans_data_len < RTL_MIN_PATCH_LEN) {

And 3 here, instead of sizeof(struct udphdr)

quoted

+                       u16 dest = ntohs(udp_hdr(skb)->dest);
+
+                       if (dest == PTP_EV_PORT || dest == PTP_GEN_PORT)
+                               padto = len + RTL_MIN_PATCH_LEN - trans_data_len;
+               }
+
+               if (trans_data_len < UDP_HLEN)
+                       padto = max(padto, len + UDP_HLEN - trans_data_len);
+       }
+
+       return padto;
+}
+
+static unsigned int rtl_quirk_packet_padto(struct rtl8169_private *tp,
+                                          struct sk_buff *skb)
+{
+       unsigned int padto;
+
+       padto = rtl8125_quirk_udp_padto(tp, skb);
+
        switch (tp->mac_version) {
        case RTL_GIGA_MAC_VER_34:
        case RTL_GIGA_MAC_VER_60:
        case RTL_GIGA_MAC_VER_61:
        case RTL_GIGA_MAC_VER_63:
-               return true;
+               padto = max_t(unsigned int, padto, ETH_ZLEN);
        default:
-               return false;
+               break;
        }
+
+       return padto;
 }

 static void rtl8169_tso_csum_v1(struct sk_buff *skb, u32 *opts)

@@ -4089,9 +4137,10 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,

                opts[1] |= transport_offset << TCPHO_SHIFT;
        } else {
-               if (unlikely(skb->len < ETH_ZLEN && rtl_test_hw_pad_bug(tp)))
-                       /* eth_skb_pad would free the skb on error */
-                       return !__skb_put_padto(skb, ETH_ZLEN, false);
+               unsigned int padto = rtl_quirk_packet_padto(tp, skb);
+
+               /* skb_padto would free the skb on error */
+               return !__skb_put_padto(skb, padto, false);
        }

        return true;

@@ -4268,6 +4317,9 @@ static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
                if (skb->len < ETH_ZLEN)
                        features &= ~NETIF_F_CSUM_MASK;

+               if (rtl_quirk_packet_padto(tp, skb))
+                       features &= ~NETIF_F_CSUM_MASK;
+
                if (transport_offset > TCPHO_MAX &&
                    rtl_chip_supports_csum_v2(tp))
                        features &= ~NETIF_F_CSUM_MASK;
--

2.30.0

The workaround was provided by Realtek, I just modified it to match
mainline code style. For your reference I add the original version below.
I don't know where the magic numbers come from, Realtek releases
neither data sheets nor errata information.


#define MIN_PATCH_LEN (47)
static u32
rtl8125_get_patch_pad_len(struct sk_buff *skb)
{
        u32 pad_len = 0;
        int trans_data_len;
        u32 hdr_len;
        u32 pkt_len = skb->len;
        u8 ip_protocol;
        bool has_trans = skb_transport_header_was_set(skb);

        if (!(has_trans && (pkt_len < 175))) //128 + MIN_PATCH_LEN
                goto no_padding;

        ip_protocol = rtl8125_get_l4_protocol(skb);
        if (!(ip_protocol == IPPROTO_TCP || ip_protocol == IPPROTO_UDP))
                goto no_padding;

        trans_data_len = pkt_len -
                         (skb->transport_header -
                          skb_headroom(skb));
        if (ip_protocol == IPPROTO_UDP) {
                if (trans_data_len > 3 && trans_data_len < MIN_PATCH_LEN) {
                        u16 dest_port = 0;

                        skb_copy_bits(skb, skb->transport_header - skb_headroom(skb) + 2, &dest_port, 2);
                        dest_port = ntohs(dest_port);

                        if (dest_port == 0x13f ||
                            dest_port == 0x140) {
                                pad_len = MIN_PATCH_LEN - trans_data_len;
                                goto out;
                        }
                }
        }

        hdr_len = 0;
        if (ip_protocol == IPPROTO_TCP)
                hdr_len = 20;
        else if (ip_protocol == IPPROTO_UDP)
                hdr_len = 8;
        if (trans_data_len < hdr_len)
                pad_len = hdr_len - trans_data_len;

out:
        if ((pkt_len + pad_len) < ETH_ZLEN)
                pad_len = ETH_ZLEN - pkt_len;

        return pad_len;

no_padding:

        return 0;
}

On Wed, Jan 27, 2021 at 2:40 PM Heiner Kallweit [off-list ref] wrote:

On 27.01.2021 19:07, Willem de Bruijn wrote:

quoted

On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit [off-list ref] wrote:

quoted

It was reported that on RTL8125 network breaks under heavy UDP load,
e.g. torrent traffic ([0], from comment 27). Realtek confirmed a hw bug
and provided me with a test version of the r8125 driver including a
workaround. Tests confirmed that the workaround fixes the issue.
I modified the original version of the workaround to meet mainline
code style.

[0] https://bugzilla.kernel.org/show_bug.cgi?id=209839

Fixes: f1bce4ad2f1c ("r8169: add support for RTL8125")
Tested-by: xplo <redacted>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 64 ++++++++++++++++++++---
 1 file changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index fb67d8f79..90052033b 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c

@@ -28,6 +28,7 @@
 #include <linux/bitfield.h>
 #include <linux/prefetch.h>
 #include <linux/ipv6.h>
+#include <linux/ptp_classify.h>
 #include <asm/unaligned.h>
 #include <net/ip6_checksum.h>

@@ -4007,17 +4008,64 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
        return -EIO;
 }

-static bool rtl_test_hw_pad_bug(struct rtl8169_private *tp)
+static bool rtl_skb_is_udp(struct sk_buff *skb)
 {
+       switch (vlan_get_protocol(skb)) {
+       case htons(ETH_P_IP):
+               return ip_hdr(skb)->protocol == IPPROTO_UDP;
+       case htons(ETH_P_IPV6):
+               return ipv6_hdr(skb)->nexthdr == IPPROTO_UDP;

This trusts that an skb with given skb->protocol is well behaved. With
packet sockets/tun/virtio, that may be false.

quoted

+       default:
+               return false;
+       }
+}
+
+#define RTL_MIN_PATCH_LEN      47
+#define PTP_GEN_PORT           320

Why the two PTP ports? The report is not PTP specific. Also, what does
patch mean in this context?

quoted

+
+/* see rtl8125_get_patch_pad_len() in r8125 vendor driver */
+static unsigned int rtl8125_quirk_udp_padto(struct rtl8169_private *tp,
+                                           struct sk_buff *skb)
+{
+       unsigned int padto = 0, len = skb->len;
+
+       if (rtl_is_8125(tp) && len < 175 && rtl_skb_is_udp(skb) &&
+           skb_transport_header_was_set(skb)) {

What is 175 here?

quoted

+               unsigned int trans_data_len = skb_tail_pointer(skb) -
+                                             skb_transport_header(skb);
+
+               if (trans_data_len > 3 && trans_data_len < RTL_MIN_PATCH_LEN) {

And 3 here, instead of sizeof(struct udphdr)

quoted

+                       u16 dest = ntohs(udp_hdr(skb)->dest);
+
+                       if (dest == PTP_EV_PORT || dest == PTP_GEN_PORT)
+                               padto = len + RTL_MIN_PATCH_LEN - trans_data_len;
+               }
+
+               if (trans_data_len < UDP_HLEN)
+                       padto = max(padto, len + UDP_HLEN - trans_data_len);
+       }
+
+       return padto;
+}
+
+static unsigned int rtl_quirk_packet_padto(struct rtl8169_private *tp,
+                                          struct sk_buff *skb)
+{
+       unsigned int padto;
+
+       padto = rtl8125_quirk_udp_padto(tp, skb);
+
        switch (tp->mac_version) {
        case RTL_GIGA_MAC_VER_34:
        case RTL_GIGA_MAC_VER_60:
        case RTL_GIGA_MAC_VER_61:
        case RTL_GIGA_MAC_VER_63:
-               return true;
+               padto = max_t(unsigned int, padto, ETH_ZLEN);
        default:
-               return false;
+               break;
        }
+
+       return padto;
 }

 static void rtl8169_tso_csum_v1(struct sk_buff *skb, u32 *opts)

@@ -4089,9 +4137,10 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,

                opts[1] |= transport_offset << TCPHO_SHIFT;
        } else {
-               if (unlikely(skb->len < ETH_ZLEN && rtl_test_hw_pad_bug(tp)))
-                       /* eth_skb_pad would free the skb on error */
-                       return !__skb_put_padto(skb, ETH_ZLEN, false);
+               unsigned int padto = rtl_quirk_packet_padto(tp, skb);
+
+               /* skb_padto would free the skb on error */
+               return !__skb_put_padto(skb, padto, false);
        }

        return true;

@@ -4268,6 +4317,9 @@ static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
                if (skb->len < ETH_ZLEN)
                        features &= ~NETIF_F_CSUM_MASK;

+               if (rtl_quirk_packet_padto(tp, skb))
+                       features &= ~NETIF_F_CSUM_MASK;
+
                if (transport_offset > TCPHO_MAX &&
                    rtl_chip_supports_csum_v2(tp))
                        features &= ~NETIF_F_CSUM_MASK;
--

2.30.0

The workaround was provided by Realtek, I just modified it to match
mainline code style. For your reference I add the original version below.
I don't know where the magic numbers come from, Realtek releases
neither data sheets nor errata information.

Okay. I don't know what is customary for this process.

But I would address the possible out of bounds read by trusting ip
header integrity in rtl_skb_is_udp.

On 27.01.2021 20:54, Willem de Bruijn wrote:

On Wed, Jan 27, 2021 at 2:40 PM Heiner Kallweit [off-list ref] wrote:

quoted

On 27.01.2021 19:07, Willem de Bruijn wrote:

quoted

On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit [off-list ref] wrote:

quoted

It was reported that on RTL8125 network breaks under heavy UDP load,
e.g. torrent traffic ([0], from comment 27). Realtek confirmed a hw bug
and provided me with a test version of the r8125 driver including a
workaround. Tests confirmed that the workaround fixes the issue.
I modified the original version of the workaround to meet mainline
code style.

[0] https://bugzilla.kernel.org/show_bug.cgi?id=209839

Fixes: f1bce4ad2f1c ("r8169: add support for RTL8125")
Tested-by: xplo <redacted>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 64 ++++++++++++++++++++---
 1 file changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index fb67d8f79..90052033b 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c

@@ -28,6 +28,7 @@
 #include <linux/bitfield.h>
 #include <linux/prefetch.h>
 #include <linux/ipv6.h>
+#include <linux/ptp_classify.h>
 #include <asm/unaligned.h>
 #include <net/ip6_checksum.h>

@@ -4007,17 +4008,64 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
        return -EIO;
 }

-static bool rtl_test_hw_pad_bug(struct rtl8169_private *tp)
+static bool rtl_skb_is_udp(struct sk_buff *skb)
 {
+       switch (vlan_get_protocol(skb)) {
+       case htons(ETH_P_IP):
+               return ip_hdr(skb)->protocol == IPPROTO_UDP;
+       case htons(ETH_P_IPV6):
+               return ipv6_hdr(skb)->nexthdr == IPPROTO_UDP;

This trusts that an skb with given skb->protocol is well behaved. With
packet sockets/tun/virtio, that may be false.

quoted

+       default:
+               return false;
+       }
+}
+
+#define RTL_MIN_PATCH_LEN      47
+#define PTP_GEN_PORT           320

Why the two PTP ports? The report is not PTP specific. Also, what does
patch mean in this context?

quoted

+
+/* see rtl8125_get_patch_pad_len() in r8125 vendor driver */
+static unsigned int rtl8125_quirk_udp_padto(struct rtl8169_private *tp,
+                                           struct sk_buff *skb)
+{
+       unsigned int padto = 0, len = skb->len;
+
+       if (rtl_is_8125(tp) && len < 175 && rtl_skb_is_udp(skb) &&
+           skb_transport_header_was_set(skb)) {

What is 175 here?

quoted

+               unsigned int trans_data_len = skb_tail_pointer(skb) -
+                                             skb_transport_header(skb);
+
+               if (trans_data_len > 3 && trans_data_len < RTL_MIN_PATCH_LEN) {

And 3 here, instead of sizeof(struct udphdr)

quoted

+                       u16 dest = ntohs(udp_hdr(skb)->dest);
+
+                       if (dest == PTP_EV_PORT || dest == PTP_GEN_PORT)
+                               padto = len + RTL_MIN_PATCH_LEN - trans_data_len;
+               }
+
+               if (trans_data_len < UDP_HLEN)
+                       padto = max(padto, len + UDP_HLEN - trans_data_len);
+       }
+
+       return padto;
+}
+
+static unsigned int rtl_quirk_packet_padto(struct rtl8169_private *tp,
+                                          struct sk_buff *skb)
+{
+       unsigned int padto;
+
+       padto = rtl8125_quirk_udp_padto(tp, skb);
+
        switch (tp->mac_version) {
        case RTL_GIGA_MAC_VER_34:
        case RTL_GIGA_MAC_VER_60:
        case RTL_GIGA_MAC_VER_61:
        case RTL_GIGA_MAC_VER_63:
-               return true;
+               padto = max_t(unsigned int, padto, ETH_ZLEN);
        default:
-               return false;
+               break;
        }
+
+       return padto;
 }

 static void rtl8169_tso_csum_v1(struct sk_buff *skb, u32 *opts)

@@ -4089,9 +4137,10 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,

                opts[1] |= transport_offset << TCPHO_SHIFT;
        } else {
-               if (unlikely(skb->len < ETH_ZLEN && rtl_test_hw_pad_bug(tp)))
-                       /* eth_skb_pad would free the skb on error */
-                       return !__skb_put_padto(skb, ETH_ZLEN, false);
+               unsigned int padto = rtl_quirk_packet_padto(tp, skb);
+
+               /* skb_padto would free the skb on error */
+               return !__skb_put_padto(skb, padto, false);
        }

        return true;

@@ -4268,6 +4317,9 @@ static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
                if (skb->len < ETH_ZLEN)
                        features &= ~NETIF_F_CSUM_MASK;

+               if (rtl_quirk_packet_padto(tp, skb))
+                       features &= ~NETIF_F_CSUM_MASK;
+
                if (transport_offset > TCPHO_MAX &&
                    rtl_chip_supports_csum_v2(tp))
                        features &= ~NETIF_F_CSUM_MASK;
--

2.30.0

The workaround was provided by Realtek, I just modified it to match
mainline code style. For your reference I add the original version below.
I don't know where the magic numbers come from, Realtek releases
neither data sheets nor errata information.

Okay. I don't know what is customary for this process.

But I would address the possible out of bounds read by trusting ip
header integrity in rtl_skb_is_udp.

I don't know tun/virtio et al good enough to judge which header elements
may be trustworthy and which may be not. What should be checked where?

On Wed, Jan 27, 2021 at 3:32 PM Heiner Kallweit [off-list ref] wrote:

On 27.01.2021 20:54, Willem de Bruijn wrote:

quoted

On Wed, Jan 27, 2021 at 2:40 PM Heiner Kallweit [off-list ref] wrote:

quoted

On 27.01.2021 19:07, Willem de Bruijn wrote:

quoted

On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit [off-list ref] wrote:

quoted

It was reported that on RTL8125 network breaks under heavy UDP load,
e.g. torrent traffic ([0], from comment 27). Realtek confirmed a hw bug
and provided me with a test version of the r8125 driver including a
workaround. Tests confirmed that the workaround fixes the issue.
I modified the original version of the workaround to meet mainline
code style.

[0] https://bugzilla.kernel.org/show_bug.cgi?id=209839

Fixes: f1bce4ad2f1c ("r8169: add support for RTL8125")
Tested-by: xplo <redacted>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 64 ++++++++++++++++++++---
 1 file changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index fb67d8f79..90052033b 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c

@@ -28,6 +28,7 @@
 #include <linux/bitfield.h>
 #include <linux/prefetch.h>
 #include <linux/ipv6.h>
+#include <linux/ptp_classify.h>
 #include <asm/unaligned.h>
 #include <net/ip6_checksum.h>

@@ -4007,17 +4008,64 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
        return -EIO;
 }

-static bool rtl_test_hw_pad_bug(struct rtl8169_private *tp)
+static bool rtl_skb_is_udp(struct sk_buff *skb)
 {
+       switch (vlan_get_protocol(skb)) {
+       case htons(ETH_P_IP):
+               return ip_hdr(skb)->protocol == IPPROTO_UDP;
+       case htons(ETH_P_IPV6):
+               return ipv6_hdr(skb)->nexthdr == IPPROTO_UDP;

The workaround was provided by Realtek, I just modified it to match
mainline code style. For your reference I add the original version below.
I don't know where the magic numbers come from, Realtek releases
neither data sheets nor errata information.

Okay. I don't know what is customary for this process.

But I would address the possible out of bounds read by trusting ip
header integrity in rtl_skb_is_udp.

I don't know tun/virtio et al good enough to judge which header elements
may be trustworthy and which may be not. What should be checked where?

It requires treating the transmit path similar to the receive path:
assume malicious or otherwise faulty packets. So do not trust that a
protocol of ETH_P_IPV6 implies a packet with 40B of space to hold a
full ipv6 header. That is the extent of it, really.

On 27.01.2021 21:35, Willem de Bruijn wrote:

On Wed, Jan 27, 2021 at 3:32 PM Heiner Kallweit [off-list ref] wrote:

quoted

On 27.01.2021 20:54, Willem de Bruijn wrote:

quoted

On Wed, Jan 27, 2021 at 2:40 PM Heiner Kallweit [off-list ref] wrote:

quoted

On 27.01.2021 19:07, Willem de Bruijn wrote:

quoted

On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit [off-list ref] wrote:

quoted

It was reported that on RTL8125 network breaks under heavy UDP load,
e.g. torrent traffic ([0], from comment 27). Realtek confirmed a hw bug
and provided me with a test version of the r8125 driver including a
workaround. Tests confirmed that the workaround fixes the issue.
I modified the original version of the workaround to meet mainline
code style.

[0] https://bugzilla.kernel.org/show_bug.cgi?id=209839

Fixes: f1bce4ad2f1c ("r8169: add support for RTL8125")
Tested-by: xplo <redacted>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 64 ++++++++++++++++++++---
 1 file changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index fb67d8f79..90052033b 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c

@@ -28,6 +28,7 @@
 #include <linux/bitfield.h>
 #include <linux/prefetch.h>
 #include <linux/ipv6.h>
+#include <linux/ptp_classify.h>
 #include <asm/unaligned.h>
 #include <net/ip6_checksum.h>

@@ -4007,17 +4008,64 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
        return -EIO;
 }

-static bool rtl_test_hw_pad_bug(struct rtl8169_private *tp)
+static bool rtl_skb_is_udp(struct sk_buff *skb)
 {
+       switch (vlan_get_protocol(skb)) {
+       case htons(ETH_P_IP):
+               return ip_hdr(skb)->protocol == IPPROTO_UDP;
+       case htons(ETH_P_IPV6):
+               return ipv6_hdr(skb)->nexthdr == IPPROTO_UDP;

The workaround was provided by Realtek, I just modified it to match
mainline code style. For your reference I add the original version below.
I don't know where the magic numbers come from, Realtek releases
neither data sheets nor errata information.

Okay. I don't know what is customary for this process.

But I would address the possible out of bounds read by trusting ip
header integrity in rtl_skb_is_udp.

I don't know tun/virtio et al good enough to judge which header elements
may be trustworthy and which may be not. What should be checked where?

It requires treating the transmit path similar to the receive path:
assume malicious or otherwise faulty packets. So do not trust that a
protocol of ETH_P_IPV6 implies a packet with 40B of space to hold a
full ipv6 header. That is the extent of it, really.

OK, so what can I do? Check for
skb_tail_pointer(skb) - skb_network_header(skb) >= sizeof(struct ipv6hdr) ?

On a side note: Why is IP6_HLEN defined in ptp_classify.h and not in any
IPv6 header file? Does no IPv6 code need such a constant?

On Wed, Jan 27, 2021 at 4:34 PM Heiner Kallweit [off-list ref] wrote:

On 27.01.2021 21:35, Willem de Bruijn wrote:

quoted

On Wed, Jan 27, 2021 at 3:32 PM Heiner Kallweit [off-list ref] wrote:

quoted

On 27.01.2021 20:54, Willem de Bruijn wrote:

quoted

On Wed, Jan 27, 2021 at 2:40 PM Heiner Kallweit [off-list ref] wrote:

quoted

On 27.01.2021 19:07, Willem de Bruijn wrote:

quoted

On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit [off-list ref] wrote:

quoted

It was reported that on RTL8125 network breaks under heavy UDP load,
e.g. torrent traffic ([0], from comment 27). Realtek confirmed a hw bug
and provided me with a test version of the r8125 driver including a
workaround. Tests confirmed that the workaround fixes the issue.
I modified the original version of the workaround to meet mainline
code style.

[0] https://bugzilla.kernel.org/show_bug.cgi?id=209839

Fixes: f1bce4ad2f1c ("r8169: add support for RTL8125")
Tested-by: xplo <redacted>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 64 ++++++++++++++++++++---
 1 file changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index fb67d8f79..90052033b 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c

@@ -28,6 +28,7 @@
 #include <linux/bitfield.h>
 #include <linux/prefetch.h>
 #include <linux/ipv6.h>
+#include <linux/ptp_classify.h>
 #include <asm/unaligned.h>
 #include <net/ip6_checksum.h>

@@ -4007,17 +4008,64 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
        return -EIO;
 }

-static bool rtl_test_hw_pad_bug(struct rtl8169_private *tp)
+static bool rtl_skb_is_udp(struct sk_buff *skb)
 {
+       switch (vlan_get_protocol(skb)) {
+       case htons(ETH_P_IP):
+               return ip_hdr(skb)->protocol == IPPROTO_UDP;
+       case htons(ETH_P_IPV6):
+               return ipv6_hdr(skb)->nexthdr == IPPROTO_UDP;

The workaround was provided by Realtek, I just modified it to match
mainline code style. For your reference I add the original version below.
I don't know where the magic numbers come from, Realtek releases
neither data sheets nor errata information.

Okay. I don't know what is customary for this process.

But I would address the possible out of bounds read by trusting ip
header integrity in rtl_skb_is_udp.

I don't know tun/virtio et al good enough to judge which header elements
may be trustworthy and which may be not. What should be checked where?

It requires treating the transmit path similar to the receive path:
assume malicious or otherwise faulty packets. So do not trust that a
protocol of ETH_P_IPV6 implies a packet with 40B of space to hold a
full ipv6 header. That is the extent of it, really.

OK, so what can I do? Check for
skb_tail_pointer(skb) - skb_network_header(skb) >= sizeof(struct ipv6hdr) ?

It is quite rare for device drivers to access protocol header fields
(and a grep points to lots of receive side operations), so I don't
have a good driver example.

But qdisc_pkt_len_init in net/core/dev.c shows a good approach for
this robust access in the transmit path: using skb_header_pointer.

On a side note: Why is IP6_HLEN defined in ptp_classify.h and not in any
IPv6 header file? Does no IPv6 code need such a constant?

It is customary to use sizeof(struct ipv6hdr)

On 27.01.2021 23:48, Willem de Bruijn wrote:

On Wed, Jan 27, 2021 at 4:34 PM Heiner Kallweit [off-list ref] wrote:

quoted

On 27.01.2021 21:35, Willem de Bruijn wrote:

quoted

On Wed, Jan 27, 2021 at 3:32 PM Heiner Kallweit [off-list ref] wrote:

quoted

On 27.01.2021 20:54, Willem de Bruijn wrote:

quoted

On Wed, Jan 27, 2021 at 2:40 PM Heiner Kallweit [off-list ref] wrote:

quoted

On 27.01.2021 19:07, Willem de Bruijn wrote:

quoted

On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit [off-list ref] wrote:

quoted

It was reported that on RTL8125 network breaks under heavy UDP load,
e.g. torrent traffic ([0], from comment 27). Realtek confirmed a hw bug
and provided me with a test version of the r8125 driver including a
workaround. Tests confirmed that the workaround fixes the issue.
I modified the original version of the workaround to meet mainline
code style.

[0] https://bugzilla.kernel.org/show_bug.cgi?id=209839

Fixes: f1bce4ad2f1c ("r8169: add support for RTL8125")
Tested-by: xplo <redacted>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 64 ++++++++++++++++++++---
 1 file changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index fb67d8f79..90052033b 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c

@@ -28,6 +28,7 @@
 #include <linux/bitfield.h>
 #include <linux/prefetch.h>
 #include <linux/ipv6.h>
+#include <linux/ptp_classify.h>
 #include <asm/unaligned.h>
 #include <net/ip6_checksum.h>

@@ -4007,17 +4008,64 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
        return -EIO;
 }

-static bool rtl_test_hw_pad_bug(struct rtl8169_private *tp)
+static bool rtl_skb_is_udp(struct sk_buff *skb)
 {
+       switch (vlan_get_protocol(skb)) {
+       case htons(ETH_P_IP):
+               return ip_hdr(skb)->protocol == IPPROTO_UDP;
+       case htons(ETH_P_IPV6):
+               return ipv6_hdr(skb)->nexthdr == IPPROTO_UDP;

The workaround was provided by Realtek, I just modified it to match
mainline code style. For your reference I add the original version below.
I don't know where the magic numbers come from, Realtek releases
neither data sheets nor errata information.

Okay. I don't know what is customary for this process.

But I would address the possible out of bounds read by trusting ip
header integrity in rtl_skb_is_udp.

I don't know tun/virtio et al good enough to judge which header elements
may be trustworthy and which may be not. What should be checked where?

It requires treating the transmit path similar to the receive path:
assume malicious or otherwise faulty packets. So do not trust that a
protocol of ETH_P_IPV6 implies a packet with 40B of space to hold a
full ipv6 header. That is the extent of it, really.

OK, so what can I do? Check for
skb_tail_pointer(skb) - skb_network_header(skb) >= sizeof(struct ipv6hdr) ?

It is quite rare for device drivers to access protocol header fields
(and a grep points to lots of receive side operations), so I don't
have a good driver example.

But qdisc_pkt_len_init in net/core/dev.c shows a good approach for
this robust access in the transmit path: using skb_header_pointer.

quoted

On a side note: Why is IP6_HLEN defined in ptp_classify.h and not in any
IPv6 header file? Does no IPv6 code need such a constant?

It is customary to use sizeof(struct ipv6hdr)

Thanks for the valuable feedback!