# Re: \[PATCH v2\] net: mscc: ocelot: add missing lock protection in ocelot\_port\_xmit()

- From: Paolo Abeni \<pabeni@redhat.com\>
- Date: 2026-02-03 11:41:31 
- List: netdev
- Page: https://ratatoskr.run/netdev/2026/02/3399259
- Thread: https://ratatoskr.run/netdev/2026/01/3395606/t.md

```text
On 1/31/26 5:49 AM, Ziyi Guo wrote:
> ocelot_port_xmit() calls ocelot_can_inject() and ocelot_port_inject_frame()
> without holding ocelot->inj_lock. However, both functions have
> lockdep_assert_held(&ocelot->inj_lock) indicating that callers must hold
> this lock.
> 
> The correct caller felix_port_deferred_xmit() properly acquires the lock
> using ocelot_lock_inj_grp() before calling these functions.
> 
> Add ocelot_lock_inj_grp()/ocelot_unlock_inj_grp() around the injection
> code path in ocelot_port_xmit() to fix the missing lock protection.
> 
> Fixes: c5e12ac3beb0 ("net: mscc: ocelot: serialize access to the injection/extraction groups")
> Signed-off-by: Ziyi Guo <redacted>
> ---
> v2:
>  - Add missing Fixes tag
> 
>  drivers/net/ethernet/mscc/ocelot_net.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
> index 469784d3a1a6..da8579abea1e 100644
> --- a/drivers/net/ethernet/mscc/ocelot_net.c
> +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> @@ -559,15 +559,22 @@ static netdev_tx_t ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
>  	int port = priv->port.index;
>  	u32 rew_op = 0;
>  
> -	if (!static_branch_unlikely(&ocelot_fdma_enabled) &&
> -	    !ocelot_can_inject(ocelot, 0))
> -		return NETDEV_TX_BUSY;
> +	if (!static_branch_unlikely(&ocelot_fdma_enabled)) {
> +		ocelot_lock_inj_grp(ocelot, 0);
> +
> +		if (!ocelot_can_inject(ocelot, 0)) {
> +			ocelot_unlock_inj_grp(ocelot, 0);
> +			return NETDEV_TX_BUSY;
> +		}
> +	}
>  
>  	/* Check if timestamping is needed */
>  	if (ocelot->ptp && (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
>  		struct sk_buff *clone = NULL;
>  
>  		if (ocelot_port_txtstamp_request(ocelot, port, skb, &clone)) {
> +			if (!static_branch_unlikely(&ocelot_fdma_enabled))
> +				ocelot_unlock_inj_grp(ocelot, 0);

I'm under the impression this static_branch_unlikely() usage is racy,
i.e. on CONFIG_PREEMPT kernel execution could enter this branch but not
the paired lock.

What about moving the 'Check timestamp' block in a separate helper and
use a single static_branch_unlikely() branch? something alike the
following, completely untested and unfinished:

	if (!static_branch_unlikely(&ocelot_fdma_enabled)) {
		int ret = NETDEV_TX_OK;

		ocelot_lock_inj_grp(ocelot, 0);

		if (!ocelot_can_inject(ocelot, 0)) {
			ret = NETDEV_TX_BUSY;
			goto unlock;
		}

		if (!ocelot_timestamp_check())
			goto unlock;


		ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);
		consume_skb(skb);
unlock:
		ocelot_unlock_inj_grp(ocelot, 0);
		return ret;
	}

	if (!ocelot_timestamp_check())
		return NETDEV_TX_OK;
	ocelot_fdma_inject_frame(ocelot, port, rew_op, skb, dev);
	return NETDEV_TX_OK;

Well, after scratching the above, I noted it would probably better to
invert the two branches...

/P

```