| Message ID | c7fd197f-8ab8-2297-385e-5d2b1d5911d7@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] r8169: work around RTL8125 UDP hw bug | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to net |
| netdev/tree_selection | success | Clearly marked for net |
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 <xplo.bn@gmail.com> > 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 <hkallweit1@gmail.com> 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 <xplo.bn@gmail.com> > 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. > + 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) > + 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 27.01.2021 19:07, Willem de Bruijn wrote: > On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit <hkallweit1@gmail.com> 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 <xplo.bn@gmail.com> >> 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. > >> + 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) > >> + 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 <hkallweit1@gmail.com> wrote: > > On 27.01.2021 19:07, Willem de Bruijn wrote: > > On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit <hkallweit1@gmail.com> 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 <xplo.bn@gmail.com> > >> 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. > > > >> + 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) > > > >> + 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 <hkallweit1@gmail.com> wrote: >> >> On 27.01.2021 19:07, Willem de Bruijn wrote: >>> On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit <hkallweit1@gmail.com> 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 <xplo.bn@gmail.com> >>>> 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. >>> >>>> + 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) >>> >>>> + 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 <hkallweit1@gmail.com> wrote: > > On 27.01.2021 20:54, Willem de Bruijn wrote: > > On Wed, Jan 27, 2021 at 2:40 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > >> > >> On 27.01.2021 19:07, Willem de Bruijn wrote: > >>> On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit <hkallweit1@gmail.com> 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 <xplo.bn@gmail.com> > >>>> 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 <hkallweit1@gmail.com> wrote: >> >> On 27.01.2021 20:54, Willem de Bruijn wrote: >>> On Wed, Jan 27, 2021 at 2:40 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: >>>> >>>> On 27.01.2021 19:07, Willem de Bruijn wrote: >>>>> On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit <hkallweit1@gmail.com> 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 <xplo.bn@gmail.com> >>>>>> 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 <hkallweit1@gmail.com> wrote: > > On 27.01.2021 21:35, Willem de Bruijn wrote: > > On Wed, Jan 27, 2021 at 3:32 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > >> > >> On 27.01.2021 20:54, Willem de Bruijn wrote: > >>> On Wed, Jan 27, 2021 at 2:40 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > >>>> > >>>> On 27.01.2021 19:07, Willem de Bruijn wrote: > >>>>> On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit <hkallweit1@gmail.com> 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 <xplo.bn@gmail.com> > >>>>>> 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 <hkallweit1@gmail.com> wrote: >> >> On 27.01.2021 21:35, Willem de Bruijn wrote: >>> On Wed, Jan 27, 2021 at 3:32 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: >>>> >>>> On 27.01.2021 20:54, Willem de Bruijn wrote: >>>>> On Wed, Jan 27, 2021 at 2:40 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: >>>>>> >>>>>> On 27.01.2021 19:07, Willem de Bruijn wrote: >>>>>>> On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit <hkallweit1@gmail.com> 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 <xplo.bn@gmail.com> >>>>>>>> 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) > Thanks for the valuable feedback!
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;
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 <xplo.bn@gmail.com> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/realtek/r8169_main.c | 64 ++++++++++++++++++++--- 1 file changed, 58 insertions(+), 6 deletions(-)