diff mbox series

[net] r8169: work around RTL8125 UDP hw bug

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

Checks

Context Check Description
netdev/apply fail Patch does not apply to net
netdev/tree_selection success Clearly marked for net

Commit Message

Heiner Kallweit Jan. 26, 2021, 9:02 a.m. UTC
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(-)

Comments

Heiner Kallweit Jan. 26, 2021, 10:45 p.m. UTC | #1
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.
Willem de Bruijn Jan. 27, 2021, 6:07 p.m. UTC | #2
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
>
Heiner Kallweit Jan. 27, 2021, 7:40 p.m. UTC | #3
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;
}
Willem de Bruijn Jan. 27, 2021, 7:54 p.m. UTC | #4
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.
Heiner Kallweit Jan. 27, 2021, 8:32 p.m. UTC | #5
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?
Willem de Bruijn Jan. 27, 2021, 8:35 p.m. UTC | #6
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.
Heiner Kallweit Jan. 27, 2021, 9:34 p.m. UTC | #7
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?
Willem de Bruijn Jan. 27, 2021, 10:48 p.m. UTC | #8
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)
Heiner Kallweit Jan. 28, 2021, 7:31 a.m. UTC | #9
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 mbox series

Patch

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;