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: <CAMFT1=Y=FbjyqC6vr6fV74t8AmGVcecKw+YjX6V_iA7XgZtotA@mail.gmail.com>
Date: Wed, 4 Feb 2026 23:49:49 -0600
From: Ziyi Guo <n7l8m4@...orthwestern.edu>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Vladimir Oltean <vladimir.oltean@....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 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.