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

> >  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()?

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.
> 
> >
> >  	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/20260728023217.2499420-1-wei.fang
> @oss.nxp.com?part=2