netdev - Re: [PATCH v2] net: mscc: ocelot: add missing lock protection in ocelot_port

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <cc23a37e-3b12-4dd5-89a9-ed8977601379@redhat.com>
Date: Tue, 3 Feb 2026 12:41:21 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Ziyi Guo <n7l8m4@...orthwestern.edu>,
 Vladimir Oltean <vladimir.oltean@....com>,
 Claudiu Manoil <claudiu.manoil@....com>,
 Alexandre Belloni <alexandre.belloni@...tlin.com>,
 UNGLinuxDriver@...rochip.com
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] net: mscc: ocelot: add missing lock protection in
 ocelot_port_xmit()

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 <n7l8m4@...orthwestern.edu>
> ---
> 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