netdev - Re: net: fec: rx descriptor ring out of order

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ba3b594f-bfdb-c8d6-ea1e-508040cf0414@gmail.com>
Date:   Thu, 12 Nov 2020 11:42:29 +0100
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Kegl Rohit <keglrohit@...il.com>,
        Fabio Estevam <festevam@...il.com>
Cc:     netdev <netdev@...r.kernel.org>
Subject: Re: net: fec: rx descriptor ring out of order



On 11/12/20 7:52 AM, Kegl Rohit wrote:
> On Wed, Nov 11, 2020 at 11:18 PM Fabio Estevam <festevam@...il.com> wrote:
>>
>> On Wed, Nov 11, 2020 at 11:27 AM Kegl Rohit <keglrohit@...il.com> wrote:
>>>
>>> Hello!
>>>
>>> We are using a imx6q platform.
>>> The fec interface is used to receive a continuous stream of custom /
>>> raw ethernet packets. The packet size is fixed ~132 bytes and they get
>>> sent every 250µs.
>>>
>>> While testing I observed spontaneous packet delays from time to time.
>>> After digging down deeper I think that the fec peripheral does not
>>> update the rx descriptor status correctly.
>>
>> What is the kernel version that you are using?
> 
> Sadly stuck at 3.10.108.
> https://github.com/gregkh/linux/blob/v3.10.108/drivers/net/ethernet/freescale/fec_main.c
> The rx queue status handling did not change much compared to 5.x. Only
> the NAPI handling / clearing IRQs was changed more than once.
> I also backported the newer NAPI handling style / clearing irqs not in
> the irq handler but in napi_poll() => same issue.
> The issue is pretty rare => To reproduce i have to reboot the system
> every 3 min. Sometimes after 1~2min on the first, sometimes on the
> ~10th reboot it will happen.
> 

Is seems some rmb() & wmb() are missing.

Honestly 3.10 is way too old for us spending much time on this issue.

I would try this :

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index fbd0d7df67d8dec64f712602cb0c17e3cb585e2b..99767728f2b501813f9d4a833fa3146caea50ed6 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -344,6 +344,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
         */
        status |= (BD_ENET_TX_READY | BD_ENET_TX_INTR
                        | BD_ENET_TX_LAST | BD_ENET_TX_TC);
+       wmb();
        bdp->cbd_sc = status;
 
        if (fep->bufdesc_ex) {
@@ -810,10 +811,12 @@ fec_enet_rx(struct net_device *ndev, int budget)
         */
        bdp = fep->cur_rx;
 
-       while (!((status = bdp->cbd_sc) & BD_ENET_RX_EMPTY)) {
-
-               if (pkt_received >= budget)
+       while (pkt_received < budget) {
+               status = bdp->cbd_sc;
+               rmb();
+               if (status & BD_ENET_RX_EMPTY)
                        break;
+
                pkt_received++;
 
                /* Since we have allocated space to hold a complete frame,
@@ -918,7 +921,6 @@ rx_processing_done:
 
                /* Mark the buffer empty */
                status |= BD_ENET_RX_EMPTY;
-               bdp->cbd_sc = status;
 
                if (fep->bufdesc_ex) {
                        struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
@@ -927,6 +929,9 @@ rx_processing_done:
                        ebdp->cbd_prot = 0;
                        ebdp->cbd_bdu = 0;
                }
+               /* This needs to be the final write, otherwise NIC could catch garbage values in surrounding fields */
+               wmb();
+               bdp->cbd_sc = status;
 
                /* Update BD pointer to next entry */
                if (status & BD_ENET_RX_WRAP)