linux-kernel - 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20260208133526.3ed3qzyyj567ylah@skbuf>
Date: Sun, 8 Feb 2026 15:35:26 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Ziyi Guo <n7l8m4@...orthwestern.edu>
Cc: Paolo Abeni <pabeni@...hat.com>,
	Claudiu Manoil <claudiu.manoil@....com>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	UNGLinuxDriver@...rochip.com, 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 Wed, Feb 04, 2026 at 11:49:49PM -0600, Ziyi Guo wrote:
> On Tue, Feb 3, 2026 at 5:41â€¯AM Paolo Abeni <pabeni@...hat.com> wrote:
> >
> > 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...
> 
> 
> Hi Paolo,
> 
> Thank you very much for your review and comments!
> 
> How about we use a new separate helper function like this for previous
> 'Check timestamp' block:
> 
> ```
>   static bool ocelot_xmit_timestamp(struct ocelot *ocelot, int port,
>                                    struct sk_buff *skb, u32 *rew_op)
>   {
>         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)) {
>                         kfree_skb(skb);
>                         return false;
>                 }
> 
>                 if (clone)
>                         OCELOT_SKB_CB(skb)->clone = clone;
> 
>                 *rew_op = ocelot_ptp_rew_op(skb);
>         }
> 
>         return true;
>   }
> ```
> 
> So for the function ocelot_port_xmit()
> 
> it will be:
> 
> ```
>   static netdev_tx_t ocelot_port_xmit(struct sk_buff *skb, struct
> net_device *dev)
>   {
>         struct ocelot_port_private *priv = netdev_priv(dev);
>         struct ocelot_port *ocelot_port = &priv->port;
>         struct ocelot *ocelot = ocelot_port->ocelot;
>         int port = priv->port.index;
>         u32 rew_op = 0;
> 
>         /* FDMA path: uses its own locking, handle separately */
>         if (static_branch_unlikely(&ocelot_fdma_enabled)) {
>                 if (!ocelot_xmit_timestamp(ocelot, port, skb, &rew_op))
>                         return NETDEV_TX_OK;
> 
>                 ocelot_fdma_inject_frame(ocelot, port, rew_op, skb, dev);
>                 return NETDEV_TX_OK;
>         }
> 
>         /* Register injection path: needs inj_lock held throughout */
>         ocelot_lock_inj_grp(ocelot, 0);
> 
>         if (!ocelot_can_inject(ocelot, 0)) {
>                 ocelot_unlock_inj_grp(ocelot, 0);
>                 return NETDEV_TX_BUSY;
>         }
> 
>         if (!ocelot_xmit_timestamp(ocelot, port, skb, &rew_op)) {
>                 ocelot_unlock_inj_grp(ocelot, 0);
>                 return NETDEV_TX_OK;
>         }
> 
>         ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);
> 
>         ocelot_unlock_inj_grp(ocelot, 0);
> 
>         consume_skb(skb);
> 
>         return NETDEV_TX_OK;
>   }
> ```
> 
> Feel free to let me know your thoughts!
> 
> I can send a v3 version patch once we're aligned.

The idea is not bad, but I would move one step further.

Refactor the rew_op handling into an ocelot_xmit_timestamp() function as
a first preparatory patch. The logic will need to be called from two
places and it's good not to duplicate it.

Then create two separate ocelot_port_xmit_fdma() and ocelot_port_xmit_inj(),
as a second preparatory patch.

static netdev_tx_t ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
{
	if (static_branch_unlikely(&ocelot_fdma_enabled))
		return ocelot_port_xmit_fdma(skb, dev);

	return ocelot_port_xmit_inj(skb, dev);
}

Now, as the third patch, add the required locking in ocelot_port_xmit_inj().

It's best for the FDMA vs register injection code paths to be as
separate as possible.