netdev - [PATCH net] net: macb: Fix lost RX packet wakeup race in NAPI receive

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-next>] [day] [month] [year] [list]

Message-Id: <20220228183328.338143-1-robert.hancock@calian.com>
Date:   Mon, 28 Feb 2022 12:33:28 -0600
From:   Robert Hancock <robert.hancock@...ian.com>
To:     netdev@...r.kernel.org
Cc:     nicolas.ferre@...rochip.com, claudiu.beznea@...rochip.com,
        davem@...emloft.net, kuba@...nel.org, soren.brinkmann@...inx.com,
        scott.mcnutt@...iusxm.com,
        Robert Hancock <robert.hancock@...ian.com>,
        stable@...r.kernel.org
Subject: [PATCH net] net: macb: Fix lost RX packet wakeup race in NAPI receive

There is an oddity in the way the RSR register flags propagate to the
ISR register (and the actual interrupt output) on this hardware: it
appears that RSR register bits only result in ISR being asserted if the
interrupt was actually enabled at the time, so enabling interrupts with
RSR bits already set doesn't trigger an interrupt to be raised. There
was already a partial fix for this race in the macb_poll function where
it checked for RSR bits being set and re-triggered NAPI receive.
However, there was a still a race window between checking RSR and
actually enabling interrupts, where a lost wakeup could happen. It's
necessary to check again after enabling interrupts to see if RSR was set
just prior to the interrupt being enabled, and re-trigger receive in that
case.

This issue was noticed in a point-to-point UDP request-response protocol
which periodically saw timeouts or abnormally high response times due to
received packets not being processed in a timely fashion. In many
applications, more packets arriving, including TCP retransmissions, would
cause the original packet to be processed, thus masking the issue.

Also change from using napi_reschedule to napi_schedule, as the only
difference is the presence of a return value which wasn't used here
anyway.

Fixes: 02f7a34f34e3 ("net: macb: Re-enable RX interrupt only when RX is done")
Cc: stable@...r.kernel.org
Co-developed-by: Scott McNutt <scott.mcnutt@...iusxm.com>
Signed-off-by: Scott McNutt <scott.mcnutt@...iusxm.com>
Signed-off-by: Robert Hancock <robert.hancock@...ian.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 26 ++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 98498a76ae16..338660fe1d93 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1573,14 +1573,36 @@ static int macb_poll(struct napi_struct *napi, int budget)
 	if (work_done < budget) {
 		napi_complete_done(napi, work_done);

-		/* Packets received while interrupts were disabled */
+		/* RSR bits only seem to propagate to raise interrupts when
+		 * interrupts are enabled at the time, so if bits are already
+		 * set due to packets received while interrupts were disabled,
+		 * they will not cause another interrupt to be generated when
+		 * interrupts are re-enabled.
+		 * Check for this case here.
+		 */
 		status = macb_readl(bp, RSR);
 		if (status) {
 			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
 				queue_writel(queue, ISR, MACB_BIT(RCOMP));
-			napi_reschedule(napi);
+			napi_schedule(napi);
 		} else {
 			queue_writel(queue, IER, bp->rx_intr_mask);
+
+			/* Packets could have been received in the window
+			 * between the check above and re-enabling interrupts.
+			 * Therefore, a double-check is required to avoid
+			 * losing a wakeup. This can potentially race with
+			 * the interrupt handler doing the same actions if an
+			 * interrupt is raised just after enabling them, but
+			 * this should be harmless.
+			 */
+			status = macb_readl(bp, RSR);
+			if (unlikely(status)) {
+				queue_writel(queue, IDR, bp->rx_intr_mask);
+				if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+					queue_writel(queue, ISR, MACB_BIT(RCOMP));
+				napi_schedule(napi);
+			}
 		}
 	}

-- 
2.31.1