* RE: [PATCH iwl-net v2] ice: Skip PTP timestamp processing during release
2026-10-06 10:45 ` sashiko-bot
@ 2026-10-06 13:04 ` Temerkhanov, Sergey
0 siblings, 0 replies; 3+ messages in thread
From: Temerkhanov, Sergey @ 2026-10-06 13:04 UTC (permalink / raw)
To: sashiko-reviews@lists.linux.dev; +Cc: intel-wired-lan@lists.osuosl.org
> -----Original Message-----
> From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
> Sent: Tuesday, October 6, 2026 12:45 PM
> To: Temerkhanov, Sergey <sergey.temerkhanov@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org
> Subject: Re: [PATCH iwl-net v2] ice: Skip PTP timestamp processing during
> release
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [High] net: ice: SKB memory leak in PTP Tx tracker when state is not ready
> --
>
> commit 20fdb60a0f81d0a78832f24386472d8613b50aea
> Author: Sergey Temerkhanov <sergey.temerkhanov@intel.com>
>
> ice: Skip PTP timestamp processing during release
>
> This commit adds early returns to the PTP timestamp processing paths when
> the PTP state is not ICE_PTP_READY. It is intended to prevent running callbacks
> on unready hardware and hitting uninitialized structures.
>
> > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
> > b/drivers/net/ethernet/intel/ice/ice_ptp.c
> > index b2de76065f46d..c0c12e28e08fa 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> > @@ -2815,6 +2815,9 @@ s8 ice_ptp_request_ts(struct ice_ptp_tx *tx,
> > struct sk_buff *skb)
> >
> > void ice_ptp_process_ts(struct ice_pf *pf) {
> > + if (pf->ptp.state != ICE_PTP_READY)
> > + return;
> > +
>
> [Severity: High]
> Does this early return bypass the 2-second SKB timeout cleanup logic in
> ice_ptp_process_tx_tstamp()?
>
> If PTP initialization or rebuild fails, setting pf->ptp.state to ICE_PTP_ERROR (or
> any non-READY state), but the normal NIC rebuild continues and succeeds
> (link_up becomes true, tx->init remains true), userspace can still send packets
> with SO_TIMESTAMPING.
>
> In this scenario, ice_ptp_request_ts() accepts the packets and queues the SKB
> in tx->tstamps[idx].skb because ice_ptp_is_tx_tracker_up() ignores
> pf->ptp.state.
>
> Since ice_ptp_process_ts() now returns immediately, the timeout cleanup logic
> inside ice_ptp_process_tx_tstamp() is bypassed. Will this permanently leak the
> queued SKBs until the tracker fills up and drops new requests?
The ICE_PTP_ERROR scenario is real, but it is a producer-side gap that
predates this commit, not something the early returns introduce. Addressing it
where it belongs.
On "the early return bypasses the 2-second timeout cleanup":
1. In ICE_PTP_ERROR that cleanup was already not running.
ice_ptp_periodic_work() starts with
if (pf->ptp.state != ICE_PTP_READY)
return;
which is upstream and predates this patch (present in v6.17). That worker is
the only caller of ice_ptp_maybe_trigger_tx_interrupt(), i.e. the only
software mechanism that kicks PFINT_OICR_TSYN_TX_M when the PHY has stopped
interrupting. Once the state is ICE_PTP_ERROR the worker stops rescheduling
itself, so nothing drives ice_ptp_process_tx_tstamp() anyway. The early
return is not what stops the GC.
2. Even if it ran, it could not produce usable timestamps.
ice_ptp_update_cached_phctime() lives in that same worker, so cached_phc_time
goes stale. ice_ptp_extend_40b_ts() discards anything once cached_phc_jiffies
is older than 2 s, and discards unconditionally when cached_phc_time is 0.
And ice_ptp_process_tx_tstamp() returns early when
ice_get_phy_tx_tstamp_ready() fails - the likely outcome on a PF whose PTP
rebuild just failed - and otherwise only clears in_use for indices the PHY
marks ready. So the pre-patch path was "touch a PHY we just failed to bring
up, then drop everything", which is exactly what the Fixes: tag is about.
3. It is not an unbounded leak.
Retention is capped at tx->len slots (<= 64 skb refs); beyond that
ice_ptp_request_ts() returns -1 and ice_tstamp() counts tx_hwtstamp_skipped,
which is the existing back-pressure. The references are released by
ice_ptp_flush_all_tx_tracker() on the next successful rebuild and by
ice_ptp_release_tx_tracker() at unload.
4. The E830 hunk is the same answer, and removing it would reintroduce a
different problem: ice_ptp_ts_irq() re-arms PFINT_OICR whenever
ice_ptp_tx_tstamps_pending(pf, true) is true. Without the state check the
hard IRQ re-arms itself while nothing consumes the timestamps.
Where the report is right: ice_ptp_is_tx_tracker_up() does not look at
pf->ptp.state, so after a failed rebuild (link_up restored by ice_rebuild(),
tx->init still set, calibrating still clear) new requests are accepted into a
tracker that nothing will drain. Pre-existing, but worth closing. The fix is on
the producer side rather than keeping a consumer running against unready HW:
@@ ice_ptp_is_tx_tracker_up(struct ice_ptp_tx *tx)
lockdep_assert_held(&tx->lock);
+ /* Nothing drains the tracker unless PTP is READY, so do not let new
+ * requests accumulate in it.
+ */
+ if (ptp_port_to_pf(ptp_port)->ptp.state != ICE_PTP_READY)
+ return false;
+
return tx->init && !tx->calibrating && READ_ONCE(ptp_port->link_up);
@@ ice_ptp_rebuild(struct ice_pf *pf, enum ice_reset_req reset_type)
err:
ptp->state = ICE_PTP_ERROR;
+ /* Timestamp processing stops in this state, so return the requests
+ * that were outstanding when the reset started.
+ */
+ ice_ptp_flush_tx_tracker(pf, &ptp->port.tx);
dev_err(ice_pf_to_dev(pf), "PTP reset failed %d\n", err);
This also makes ICE_PTP_RESETTING and the ice_ptp_release() window consistent:
requests are refused instead of being queued into a tracker that is about to be
torn down. Will send as a separate patch, since it fixes a different (older)
issue than this one.
>
> > switch (pf->ptp.tx_interrupt_mode) {
> > case ICE_PTP_TX_INTERRUPT_NONE:
> > /* This device has the clock owner handle timestamps for it */
>
> [ ... ]
>
> > @@ -2944,6 +2947,9 @@ irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf)
> > return IRQ_WAKE_THREAD;
> > case ICE_MAC_E830:
> > /* E830 can read timestamps in the top half using rd32() */
> > + if (pf->ptp.state != ICE_PTP_READY)
> > + return IRQ_HANDLED;
> > +
>
> [Severity: High]
> Similarly for the E830 IRQ handler, does this early return bypass the cleanup
> logic in ice_ptp_process_ts() and cause the same SKB memory leak when the
> PTP state is not ready?
>
> > ice_ptp_process_ts(pf);
> >
> > if (ice_ptp_tx_tstamps_pending(pf, true)) {
> > /* Process outstanding Tx timestamps. If there
>
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/20261006095435.419128-1-
> sergey.temerkhanov@intel.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread