From: Lorenzo Bianconi <hidden> Date: 2026-09-04 10:33:36
priv->ptp_lock is only initialized in stmmac_ptp_register(), which runs
during __stmmac_open(). However, the lock is also used while the
interface is down and has never been opened: tc_taprio_configure()
invokes the PTP gettime64() callback to compute the EST base time when
offloading a TAPRIO schedule, and stmmac_get_time() takes
priv->ptp_lock. Using an uninitialized rwlock is undefined behaviour.
Move the rwlock_init() to __stmmac_dvr_probe(), together with the other
private locks, so that ptp_lock is always valid regardless of the
interface state.
Fixes: b60189e0392f ("net: stmmac: Integrate EST with TAPRIO scheduler API")
Signed-off-by: Lorenzo Bianconi <redacted>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +
drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c | 1 -
2 files changed, 1 insertion(+), 1 deletion(-)
From: Maxime Chevallier <maxime.chevallier@bootlin.com> Date: 2026-09-04 11:30:49
Hi,
On 9/4/26 12:32, Lorenzo Bianconi wrote:
priv->ptp_lock is only initialized in stmmac_ptp_register(), which runs
during __stmmac_open(). However, the lock is also used while the
interface is down and has never been opened: tc_taprio_configure()
invokes the PTP gettime64() callback to compute the EST base time when
offloading a TAPRIO schedule, and stmmac_get_time() takes
priv->ptp_lock. Using an uninitialized rwlock is undefined behaviour.
Move the rwlock_init() to __stmmac_dvr_probe(), together with the other
private locks, so that ptp_lock is always valid regardless of the
interface state.
Fixes: b60189e0392f ("net: stmmac: Integrate EST with TAPRIO scheduler API")
Signed-off-by: Lorenzo Bianconi <redacted>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Maxime
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 0 (2) · Low: 0
- [High] Incomplete fix: the TAPRIO offload path that this commit
legitimizes for a never-opened interface still performs MMIO on a…
Pre-existing issues:
- [Medium] EST state (priv->est->enable, priv->est->gcl[]) is accessed
with inconsistent locking between the taprio offload path and the PTP…
- [Medium] An EST/taprio schedule programmed while the interface is down
is silently discarded by the MAC/DMA software reset performed at open,…
--
net: stmmac: initialize ptp_lock at probe time
This moves rwlock_init(&priv->ptp_lock) from stmmac_ptp_register() to
__stmmac_dvr_probe() so that the lock is initialized even when the
interface has never been opened, which is the case when tc_taprio_configure()
calls the PTP gettime64() callback to compute the EST base time.
No Link: tags present in the commit header.
A few questions about the paths this touches.
@@ -8025,6 +8025,7 @@ static int __stmmac_dvr_probe(struct device *device,stmmac_napi_add(ndev);mutex_init(&priv->lock);+rwlock_init(&priv->ptp_lock);
[Severity: High]
The commit message describes the down-interface gettime64() call as a
legitimate use ("the lock is also used while the interface is down and has
never been opened"). Is that call safe once the lock is initialized?
__stmmac_dvr_probe() ends with pm_runtime_put(), and
stmmac_runtime_suspend() in stmmac_platform.c gates the CSR clocks:
stmmac_bus_clks_config(priv, false);
clk_disable_unprepare(priv->plat->stmmac_clk);
clk_disable_unprepare(priv->plat->pclk);
So a probed-but-never-opened device is runtime suspended. The taprio
offload path does not take a runtime-PM reference and does not check
netif_running():
stmmac_setup_tc() case TC_SETUP_QDISC_TAPRIO
tc_setup_taprio()
tc_taprio_configure()
priv->ptp_clock_ops.gettime64() -> stmmac_get_time()
stmmac_get_systime(priv->ptpaddr)
stmmac_est_configure() -> writes/polls priv->estaddr
Every other register-touching ndo in this driver wraps the access in
pm_runtime_resume_and_get(), for example stmmac_set_mac_address(),
stmmac_vlan_rx_add_vid(), stmmac_vlan_rx_kill_vid() and __stmmac_open().
Can the MMIO in stmmac_get_systime() and est_configure() read/write a
clock-gated MAC/PTP block here, and on SoCs that actually gate pclk, can
that abort or stall?
There is a second aspect to the same call. The PTP counter is only
programmed from the open path:
__stmmac_open()
stmmac_setup_ptp()
stmmac_init_timestamping()
stmmac_init_tstamp_counter() /* TCR enable, SSINC, init systime */
If the interface was never opened, does gettime64() return a usable time
base, and does stmmac_calc_tas_basetime() then program priv->est->btr[]
from a meaningless value?
Would it make sense to gate TC_SETUP_QDISC_TAPRIO on netif_running() (or
on the PTP counter being started), and/or take a runtime-PM reference in
stmmac_setup_tc(), in addition to moving the rwlock_init()? Reproducer
is "tc qdisc replace dev ethX root taprio ... flags 0x2" on an interface
that has never been brought up; taprio_enable_offload() in
net/sched/sch_taprio.c has no netif_running() gate.
[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but since
the change makes the down-interface configuration sequence easier to
reach it seems worth mentioning. Is an EST schedule programmed while the
interface is down preserved across open?
tc_taprio_configure() commits priv->est->enable, btr[]/ctr[] and
max_sdu[] and pushes them to hardware via stmmac_est_configure(). Then
open does:
__stmmac_open()
stmmac_hw_setup()
/* DMA initialization and SW reset */
stmmac_init_dma_engine(priv);
which asserts DMA SWR and resets the MAC/MTL registers, including
MTL_EST_CONTROL, the GCL RAM and BTR/CTR. stmmac_est_configure() has no
call site in stmmac_main.c (only tc_taprio_configure() and the disable
label in stmmac_tc.c, plus stmmac_adjust_time() in stmmac_ptp.c), and
taprio_dev_notifier() in net/sched/sch_taprio.c only recomputes
picos_per_byte/max_sdu on NETDEV_UP rather than re-issuing the offload.
Does that leave priv->est->enable true with EST disabled in hardware,
while stmmac_xmit(), stmmac_xdp_xmit_back() and the XSK path keep
enforcing priv->est->max_sdu[] and the taprio dump reports an active
offloaded schedule?
[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but while
looking at the locking in this file: is the est_lock coverage of
priv->est consistent between the taprio path and the adjtime path?
In stmmac_adjust_time() the decision is taken with no lock held:
/* If EST is enabled, disabled it before adjust ptp time. */
if (priv->est && priv->est->enable) {
and it is acted upon later, after est_lock has been taken again, where
priv->est->enable is set back to true and stmmac_est_configure() is
called.
Meanwhile tc_taprio_configure() publishes the new size and enable flag
under the lock:
mutex_lock(&priv->est_lock);
priv->est->gcl_size = size;
priv->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
mutex_unlock(&priv->est_lock);
but fills the entries with the lock dropped:
priv->est->gcl[i] = delta_ns | (gates << wid);
tc_taprio_configure() runs under rtnl from ndo_setup_tc while
stmmac_adjust_time() runs from PTP_CLOCK_ADJTIME on /dev/ptpX without
rtnl, so the two can run concurrently.
Can an adjtime landing in that window make stmmac_est_configure()
program the hardware gate list from a partially written gcl[] with the
new gcl_size already visible?
And can a "tc qdisc del" that clears enable and takes the disable: path
be undone by a concurrent adjtime that re-enables EST from its stale
unlocked observation?
From: Lorenzo Bianconi <hidden> Date: 2026-09-09 08:38:26
On Sep 08, netdev-bot+sashiko@kernel.org wrote:
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 0 (2) · Low: 0
- [High] Incomplete fix: the TAPRIO offload path that this commit
legitimizes for a never-opened interface still performs MMIO on a…
Pre-existing issues:
- [Medium] EST state (priv->est->enable, priv->est->gcl[]) is accessed
with inconsistent locking between the taprio offload path and the PTP…
- [Medium] An EST/taprio schedule programmed while the interface is down
is silently discarded by the MAC/DMA software reset performed at open,…
--
net: stmmac: initialize ptp_lock at probe time
This moves rwlock_init(&priv->ptp_lock) from stmmac_ptp_register() to
__stmmac_dvr_probe() so that the lock is initialized even when the
interface has never been opened, which is the case when tc_taprio_configure()
calls the PTP gettime64() callback to compute the EST base time.
No Link: tags present in the commit header.
A few questions about the paths this touches.
@@ -8025,6 +8025,7 @@ static int __stmmac_dvr_probe(struct device *device,stmmac_napi_add(ndev);mutex_init(&priv->lock);+rwlock_init(&priv->ptp_lock);
[Severity: High]
The commit message describes the down-interface gettime64() call as a
legitimate use ("the lock is also used while the interface is down and has
never been opened"). Is that call safe once the lock is initialized?
__stmmac_dvr_probe() ends with pm_runtime_put(), and
stmmac_runtime_suspend() in stmmac_platform.c gates the CSR clocks:
stmmac_bus_clks_config(priv, false);
clk_disable_unprepare(priv->plat->stmmac_clk);
clk_disable_unprepare(priv->plat->pclk);
So a probed-but-never-opened device is runtime suspended. The taprio
offload path does not take a runtime-PM reference and does not check
netif_running():
stmmac_setup_tc() case TC_SETUP_QDISC_TAPRIO
tc_setup_taprio()
tc_taprio_configure()
priv->ptp_clock_ops.gettime64() -> stmmac_get_time()
stmmac_get_systime(priv->ptpaddr)
stmmac_est_configure() -> writes/polls priv->estaddr
Every other register-touching ndo in this driver wraps the access in
pm_runtime_resume_and_get(), for example stmmac_set_mac_address(),
stmmac_vlan_rx_add_vid(), stmmac_vlan_rx_kill_vid() and __stmmac_open().
Can the MMIO in stmmac_get_systime() and est_configure() read/write a
clock-gated MAC/PTP block here, and on SoCs that actually gate pclk, can
that abort or stall?
There is a second aspect to the same call. The PTP counter is only
programmed from the open path:
__stmmac_open()
stmmac_setup_ptp()
stmmac_init_timestamping()
stmmac_init_tstamp_counter() /* TCR enable, SSINC, init systime */
If the interface was never opened, does gettime64() return a usable time
base, and does stmmac_calc_tas_basetime() then program priv->est->btr[]
from a meaningless value?
Would it make sense to gate TC_SETUP_QDISC_TAPRIO on netif_running() (or
on the PTP counter being started), and/or take a runtime-PM reference in
stmmac_setup_tc(), in addition to moving the rwlock_init()? Reproducer
is "tc qdisc replace dev ethX root taprio ... flags 0x2" on an interface
that has never been brought up; taprio_enable_offload() in
net/sched/sch_taprio.c has no netif_running() gate.
[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but since
the change makes the down-interface configuration sequence easier to
reach it seems worth mentioning. Is an EST schedule programmed while the
interface is down preserved across open?
tc_taprio_configure() commits priv->est->enable, btr[]/ctr[] and
max_sdu[] and pushes them to hardware via stmmac_est_configure(). Then
open does:
__stmmac_open()
stmmac_hw_setup()
/* DMA initialization and SW reset */
stmmac_init_dma_engine(priv);
which asserts DMA SWR and resets the MAC/MTL registers, including
MTL_EST_CONTROL, the GCL RAM and BTR/CTR. stmmac_est_configure() has no
call site in stmmac_main.c (only tc_taprio_configure() and the disable
label in stmmac_tc.c, plus stmmac_adjust_time() in stmmac_ptp.c), and
taprio_dev_notifier() in net/sched/sch_taprio.c only recomputes
picos_per_byte/max_sdu on NETDEV_UP rather than re-issuing the offload.
Does that leave priv->est->enable true with EST disabled in hardware,
while stmmac_xmit(), stmmac_xdp_xmit_back() and the XSK path keep
enforcing priv->est->max_sdu[] and the taprio dump reports an active
offloaded schedule?
[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but while
looking at the locking in this file: is the est_lock coverage of
priv->est consistent between the taprio path and the adjtime path?
In stmmac_adjust_time() the decision is taken with no lock held:
/* If EST is enabled, disabled it before adjust ptp time. */
if (priv->est && priv->est->enable) {
and it is acted upon later, after est_lock has been taken again, where
priv->est->enable is set back to true and stmmac_est_configure() is
called.
Meanwhile tc_taprio_configure() publishes the new size and enable flag
under the lock:
mutex_lock(&priv->est_lock);
priv->est->gcl_size = size;
priv->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
mutex_unlock(&priv->est_lock);
but fills the entries with the lock dropped:
priv->est->gcl[i] = delta_ns | (gates << wid);
tc_taprio_configure() runs under rtnl from ndo_setup_tc while
stmmac_adjust_time() runs from PTP_CLOCK_ADJTIME on /dev/ptpX without
rtnl, so the two can run concurrently.
Can an adjtime landing in that window make stmmac_est_configure()
program the hardware gate list from a partially written gcl[] with the
new gcl_size already visible?
And can a "tc qdisc del" that clears enable and takes the disable: path
be undone by a concurrent adjtime that re-enables EST from its stale
unlocked observation?
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski [off-list ref]:
On Fri, 04 Sep 2026 12:32:55 +0200 you wrote:
priv->ptp_lock is only initialized in stmmac_ptp_register(), which runs
during __stmmac_open(). However, the lock is also used while the
interface is down and has never been opened: tc_taprio_configure()
invokes the PTP gettime64() callback to compute the EST base time when
offloading a TAPRIO schedule, and stmmac_get_time() takes
priv->ptp_lock. Using an uninitialized rwlock is undefined behaviour.
Move the rwlock_init() to __stmmac_dvr_probe(), together with the other
private locks, so that ptp_lock is always valid regardless of the
interface state.
[...]