RE: [PATCH net 2/2] net: enetc: restore RX ring congestion mode for ENETC v4

quoted

 static void enetc4_set_tx_pause(struct enetc_pf *pf, int num_rxbdr, bool

tx_pause)

quoted

 {
+	struct enetc_ndev_priv *priv = netdev_priv(pf->si->ndev);
 	u32 pause_off_thresh = 0, pause_on_thresh = 0;
 	u32 init_quanta = 0, refresh_quanta = 0;
 	struct enetc_hw *hw = &pf->si->hw;

@@ -764,6 +765,8 @@ static void enetc4_set_tx_pause(struct enetc_pf *pf,

int num_rxbdr, bool tx_paus

quoted

 	int i;

 	for (i = 0; i < num_rxbdr; i++) {
 		old_rbmr = enetc_rxbdr_rd(hw, i, ENETC_RBMR);
 		rbmr = u32_replace_bits(old_rbmr, tx_pause ? 1 : 0,

ENETC_RBMR_CM);

quoted

 		if (rbmr == old_rbmr)
 			continue;

 		enetc_rxbdr_wr(hw, i, ENETC_RBMR, rbmr);
 	}

[Severity: Critical]
This is a pre-existing issue, but could this unprotected read-modify-write
lead to memory corruption if it races with enetc_disable_rxbdr()?

Yes, this is a potential issue, l can add a spinlock to fix this corner issue,
but I'm not sure if it is worth doing this, because the benefits are minimal.

If phylink_resolve() executes enetc4_set_tx_pause() concurrently with
enetc_reconfigure(), this loop can read ENETC_RBMR before the ring is
torn down.

If it writes the modified value back after enetc_disable_rxbdr()
clears the enable bit, it would inadvertently re-enable the RX ring in
hardware:

drivers/net/ethernet/freescale/enetc/enetc.c:enetc_disable_rxbdr() {
    ...
    /* disable EN bit on ring */
    enetc_rxbdr_wr(hw, idx, ENETC_RBMR, 0);
    ...
}

Because the software frees the associated DMA buffers during teardown, the
hardware would then DMA incoming packets directly into freed memory.

quoted

 	if (tx_pause) {
+		set_bit(ENETC_RXBDR_CM, &priv->flags);
+

[Severity: High]
Can this result in the software state and hardware register becoming
desynchronized?

Since ENETC_RXBDR_CM is set after the hardware registers are updated in the
loop above, there appears to be a race window. If enetc_setup_rxbdr() is
called by a concurrent enetc_reconfigure() during this window, it would
see the unset flag:

drivers/net/ethernet/freescale/enetc/enetc.c:enetc_setup_rxbdr() {
    ...
    if (test_bit(ENETC_RXBDR_CM, &priv->flags))
        rbmr |= ENETC_RBMR_CM;
    ...
}

This would overwrite the hardware register without the CM bit. Then this
thread sets the software flag, leaving flow control disabled in hardware
when the software state expects it to be enabled.

quoted

 		/* When the port first enters congestion, send a PAUSE request
 		 * with the maximum number of quanta. When the port exits

[ ... ]

quoted

@@ -949,7 +954,10 @@ static void enetc4_pl_mac_link_down(struct

phylink_config *config,

quoted

 				    phy_interface_t interface)
 {
 	struct enetc_pf *pf = phylink_to_enetc_pf(config);
+	struct enetc_ndev_priv *priv;

+	priv = netdev_priv(pf->si->ndev);
+	clear_bit(ENETC_RXBDR_CM, &priv->flags);
 	enetc4_mac_rx_graceful_stop(pf);
 	enetc4_mac_tx_graceful_stop(pf);
 }

--
Sashiko AI
review · https://sashiko.dev/#/patchset/20260728023217.2499420-1-wei.fang
@oss.nxp.com?part=2