Re: [PATCH net 1/2] net: enetc: restore RX ring congestion mode after ring reconfiguration

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

Pre-existing issues:
- [Critical] Lockless race between phylink's mac_link_up and driver reconfiguration corrupts hardware registers and newly introduced software state, leading to DMA use-after-free and loss of flow control.
--

--- Patch [1]: [PATCH net 1/2] net: enetc: restore RX ring congestion mode after ring reconfiguration ---
commit 299f421076cc1f539a88e83a486105279c6c0395
Author: Wei Fang <[email protected]>
Subject: net: enetc: restore RX ring congestion mode after ring reconfiguration

This commit addresses an issue where the RX ring congestion mode is lost
during runtime ring reconfiguration. It introduces a software flag to
track the congestion mode state and restores it when the RX ring is rebuilt,
ensuring flow control continues working.

> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> @@ -633,6 +633,8 @@ static void enetc_pl_mac_link_up(struct phylink_config *config,
>  	}
>  
>  	if (tx_pause) {
> +		set_bit(ENETC_RXBDR_CM, &priv->flags);
> +

[Severity: Critical]
This is a pre-existing issue, but can this lockless read-modify-write on
the ENETC_RBMR register race with driver reconfiguration?

Looking at enetc_pl_mac_link_up() in enetc_pf.c, there is an unsynchronized
read-modify-write loop on ENETC_RBMR just before this flag update:

	for (idx = 0; idx < priv->num_rx_rings; idx++) {
		rbmr = enetc_rxbdr_rd(hw, idx, ENETC_RBMR);
		...
		enetc_rxbdr_wr(hw, idx, ENETC_RBMR, rbmr);
	}

If phylink's mac_link_up executes asynchronously in the phylink_resolve
workqueue, could it run concurrently with a user triggering a driver
reconfiguration under rtnl_lock (like attaching an XDP program)?

If so, enetc_reconfigure() might call enetc_disable_rxbdr(), which clears
the ENETC_RBMR register to disable the ring:

	/* disable EN bit on ring */
	enetc_rxbdr_wr(hw, idx, ENETC_RBMR, 0);

If mac_link_up() reads the ENETC_RBMR register before it is cleared by
reconfiguration, and writes it back after it is cleared, it will write
back the stale ENETC_RBMR_EN (enable) bit.

Since enetc_reconfigure() proceeds to call enetc_free_rxtx_rings() to free
the DMA memory, re-enabling the hardware RX ring at this moment would cause
the hardware to DMA incoming network packets into freed kernel memory.

Additionally, because the new ENETC_RXBDR_CM software flag is set here after
the hardware read-modify-write loop, concurrent readers like
enetc_setup_rxbdr() could observe a stale flag state and permanently lose
flow control.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1