Re: [PATCH net v3 01/14] ice: fix removal of PTP timestamp tracker during reset

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 (2) · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] On the clock owner, ice_ptp_init_owner() ->
  ice_ptp_create_clock() registers the PHC (/dev/ptpN) before the…
- [Medium] Because the E810 tracker is now kept across CORE/GLOBAL/EMP
  reset, the per-slot tx->tstamps[idx].cached_tstamp values survive the…

Pre-existing issues:
- [High] ice_ptp_setup_adapter() sets `pf->adapter->ctrl_pf = pf` for the
  clock-owner PF, and nothing ever clears it: not in the ice_ptp_init()…
- [High] At this commit the clock owner walks pf->adapter->ports.ports
  without adapter->ports.lock in ice_ptp_flush_all_tx_tracker(),…

--

ice: fix removal of PTP timestamp tracker during reset

This patch stops ice_ptp_prepare_for_reset() from releasing the Tx
timestamp tracker, so timestamping can resume after a CORE, GLOBAL or EMP
reset. It also reorders ice_ptp_init() so the kworker and tracker are
created before the port is added to the adapter port list. Finally, it
makes ice_ptp_release() do a full cleanup when PTP is not in the READY
state.

Closes: https://sashiko.dev/#/patchset/20260821-jk-e825c-minimized-fixes-v1-0-9d0731eb4858%40intel.com?part=8
Closes: https://lore.kernel.org/netdev/20260916011213.1632286-1-kuba@kernel.org/

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index eaec36ab6ae3f..e1c66cd0f94ff 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -2939,8 +2939,6 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
>  	if (ice_pf_src_tmr_owned(pf) && hw->mac_type == ICE_MAC_GENERIC_3K_E825)
>  		ice_ptp_prepare_rebuild_sec(pf, false, reset_type);
>  
> -	ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
> -

[Severity: Medium]
The tracker is now kept across CORE/GLOBAL/EMP reset. On E810, does that
leave the per-slot tx->tstamps[idx].cached_tstamp values out of sync with
the hardware?

During rebuild, the call chain is
ice_ptp_rebuild_owner()->ice_ptp_flush_all_tx_tracker()->
ice_ptp_flush_tx_tracker(). E810 reports every slot ready and
hw->reset_ongoing has already been cleared, so every in_use slot is
cleared in hardware:

	if (!hw->reset_ongoing && (tstamp_ready & BIT_ULL(phy_idx)))
		ice_clear_phy_tstamp(hw, tx->block, phy_idx);

ice_clear_phy_tstamp_e810() writes 0 to both halves of the slot. The
flush only clears skb, in_use and stale, so cached_tstamp keeps its old
non-zero value.

E810 has no ready bitmap. It detects a new capture by comparing the
register with cached_tstamp. If the slot is reused and read before the
new capture lands, the register reads 0, which does not match the cache.

In ice_ptp_process_tx_tstamp() the "not yet captured" check is then
skipped:

		if (!drop_ts && !tx->has_ready_bitmap &&
		    raw_tstamp == tx->tstamps[idx].cached_tstamp)
			continue;

		/* Discard any timestamp value without the valid bit set */
		if (!(raw_tstamp & ICE_PTP_TS_VALID))
			drop_ts = true;

so the request is dropped without a timestamp.

ice_ptp_complete_tx_single_tstamp() has no valid bit check:

	if (raw_tstamp == tx->tstamps[idx].cached_tstamp)
		return;

	tx->tstamps[idx].cached_tstamp = raw_tstamp;

Is 0 then taken as a fresh capture, extended through
ice_ptp_extend_40b_ts(), and delivered to the socket as a bogus Tx
timestamp?

Before this patch, and before 7a25fe5cd5fb, the tracker was either
freed or reallocated zeroed, so the cache and the hardware matched.
Should the flush also reset cached_tstamp?

[ ... ]

> @@ -3347,13 +3346,17 @@ void ice_ptp_init(struct ice_pf *pf)
>  		}
>  	}
>  
> -	err = ice_ptp_setup_pf(pf);
> +	err = ice_ptp_init_work(pf, ptp);
>  	if (err)
>  		goto err_exit;
>  
>  	err = ice_ptp_init_port(pf, &ptp->port);
>  	if (err)
> -		goto err_clean_pf;
> +		goto err_release_kworker;
> +
> +	err = ice_ptp_setup_pf(pf);
> +	if (err)
> +		goto err_release_tx_tracker;

[ ... ]

> -err_clean_pf:
> +err_release_tx_tracker:
> +	ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
> +err_release_kworker:
> +	kthread_cancel_delayed_work_sync(&ptp->work);
> +	kthread_destroy_worker(ptp->kworker);
> +	ptp->kworker = NULL;
>  	mutex_destroy(&ptp->port.ps_lock);
> -	ice_ptp_cleanup_pf(pf);
>  err_exit:
>  	/* If we registered a PTP clock, release it */
>  	if (pf->ptp.clock) {

[Severity: High]
On the clock owner, ice_ptp_init_owner()->ice_ptp_create_clock() has
already registered the PHC before ice_ptp_init_work() and
ice_ptp_init_port() run. If ice_ptp_init_port() fails (for example with
-ENOMEM from ice_ptp_alloc_tx_tracker()), this unwind destroys the worker
and sets ptp->kworker to NULL. Only after that does err_exit call
ptp_clock_unregister().

Can a concurrent clock_settime() or clock_adjtime() on /dev/ptpN reach
the driver in that window? ice_ptp_settime64() and ice_ptp_adjtime() call
ice_ptp_reset_cached_phctime() without any state check:

ice_ptp_reset_cached_phctime() {
    ...
	err = ice_ptp_update_cached_phctime(pf);
	if (err) {
		...
		kthread_queue_delayed_work(pf->ptp.kworker, &pf->ptp.work,
					   msecs_to_jiffies(10));
	}
    ...
}

That branch runs whenever ICE_CFG_BUSY is already held, for example by a
second concurrent settime or adjtime.

If this lands after ptp->kworker = NULL, is it a NULL pointer
dereference?

If it lands between kthread_cancel_delayed_work_sync() and
kthread_destroy_worker(), does it re-arm the delayed work timer on a
worker that is then freed? The timer callback would later touch the
freed worker.

ice_ptp_mark_tx_tracker_stale() also runs bitmap_or() on tx->stale
without checking tx->init. Could it race with
ice_ptp_release_tx_tracker() freeing the bitmaps when coming from
err_release_tx_tracker?

The new non-READY branch of ice_ptp_release() unregisters the clock
first. Should this error path use the same order?

The same ordering is still present in two other places. On the success
path the PHC is registered before ice_ptp_init_work() creates the
kworker. The READY branch of ice_ptp_release() still destroys the kworker
before ptp_clock_unregister().

The later patch "ice: use reference counting and SRCU for PTP port
access" adds a tx->init check to ice_ptp_mark_tx_tracker_stale(). It
does not change the kworker vs. unregister ordering.

> @@ -3399,12 +3405,22 @@ void ice_ptp_release(struct ice_pf *pf)
>  		return;
>  
>  	if (pf->ptp.state != ICE_PTP_READY) {
> -		mutex_destroy(&pf->ptp.port.ps_lock);
> -		ice_ptp_cleanup_pf(pf);
>  		if (pf->ptp.clock) {
>  			ptp_clock_unregister(pf->ptp.clock);
>  			pf->ptp.clock = NULL;
>  		}
> +		ice_ptp_cleanup_pf(pf);
> +		ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);

[Severity: High]
This is a pre-existing issue, but at this commit the clock owner walks
pf->adapter->ports.ports without adapter->ports.lock in
ice_ptp_flush_all_tx_tracker(), ice_ptp_restart_all_phy() and
ice_ptp_prepare_rebuild_sec():

	list_for_each_entry(port, &pf->adapter->ports.ports, list_node)
		ice_ptp_flush_tx_tracker(ptp_port_to_pf(port), &port->tx);

A peer PF's ice_ptp_cleanup_pf() removes its node under ports.lock. This
path then frees that peer's Tx tracker, kworker and ps_lock. Nothing
waits for the unlocked walkers, so can the owner still be using the
peer's port memory after it is freed?

The later patch in this series, "ice: use reference counting and SRCU
for PTP port access", seems to fix this. It converts these walkers to
SRCU with kref_get_unless_zero(), and ice_ptp_cleanup_pf() waits for
readers to drain before the tracker and kworker are freed.

> +		if (pf->ptp.kworker) {
> +			kthread_cancel_delayed_work_sync(&pf->ptp.work);
> +			if (pf->hw.mac_type == ICE_MAC_GENERIC)
> +				kthread_cancel_delayed_work_sync(&pf->ptp.port.ov_work);
> +			kthread_destroy_worker(pf->ptp.kworker);
> +			pf->ptp.kworker = NULL;
> +		}
> +		mutex_destroy(&pf->ptp.port.ps_lock);
> +
> +		pf->ptp.state = ICE_PTP_UNINIT;
> +		return;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but adapter->ctrl_pf is never
cleared. ice_ptp_setup_adapter() sets it for the clock owner:

static void ice_ptp_setup_adapter(struct ice_pf *pf)
{
	pf->adapter->ctrl_pf = pf;
}

Neither branch of ice_ptp_release() resets it, and neither does the
ice_ptp_init() error path.

The ice_adapter is shared and refcounted across all PFs of the device.
The owner's ice_pf is freed when that PF unbinds. If only the owner PF is
unbound (for example through sysfs for VFIO passthrough) and the other
PFs stay bound, is adapter->ctrl_pf left pointing at freed memory?

Any user could then reach it by running ethtool -T on a remaining PF:

ice_get_ts_info()
  ice_ptp_clock_index()
    ice_get_ctrl_ptp()        <- &freed_pf->ptp
    ptp_clock_index(ctrl_ptp->clock)

Other users of the pointer are ice_ptp_setup_pf() on re-init and the
ice_txclk.c paths, which take ctrl_pf->dplls.lock and touch
ctrl_pf->ptp.tx_refclks. This is still unchanged at the end of the
series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015191102.1174111-1-anthony.l.nguyen%40intel.com