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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] State desynchronization due to TOCTOU race condition updating the newly added software flag.

Pre-existing issues:
- [Critical] Unprotected read-modify-write of hardware registers leads to DMA memory corruption (Use-After-Free).
--

--- Patch [2]: [PATCH net 2/2] net: enetc: restore RX ring congestion mode for ENETC v4 ---
commit c984fc5e7d3c90c2dfd0f53ae9ad32adf715c689
Author: Wei Fang <[email protected]>

net: enetc: restore RX ring congestion mode for ENETC v4

This commit fixes an issue where the RX BD ring congestion mode is cleared
when RX BD rings are rebuilt at runtime without a link status change. It
tracks the desired CM state in the software flag ENETC_RXBDR_CM so it can
be restored during reconfiguration.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> index 437a15bbb47ba..8db5a47fdbae4 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> @@ -748,6 +748,7 @@ static void enetc4_set_rx_pause(struct enetc_pf *pf, bool rx_pause)
>  
>  static void enetc4_set_tx_pause(struct enetc_pf *pf, int num_rxbdr, bool tx_pause)
>  {
> +	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
>  	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);
>  		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()?

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.

>  
>  	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.

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

[ ... ]

> @@ -949,7 +954,10 @@ static void enetc4_pl_mac_link_down(struct phylink_config *config,
>  				    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/[email protected]?part=2