[PATCH v4 net-next 8/8] net: dsa: netc: add PTP one-step timestamping support

[Wei Fang]

> > diff --git a/drivers/net/dsa/netc/netc_ptp.c b/drivers/net/dsa/netc/netc_ptp.c
> > index 2e743443ca03..1bc0f0956206 100644
> > --- a/drivers/net/dsa/netc/netc_ptp.c
> > +++ b/drivers/net/dsa/netc/netc_ptp.c
> > @@ -11,6 +11,15 @@
> >
> >  #define NETC_NUM_TS_REQ_ID		16
> >  #define NETC_TSTAMP_TIMEOUT		(5 * HZ)
> > +#define NETC_MAX_STEP_OFFSET		0x1ff
> > +#define NETC_ONESTEP_QTH		512
> > +/* The 30-bit timestamp of the To_Port subtype 3 tag lets the hardware
> > + * account for a single wrap, so the correction field of a one-step Sync
> > + * frame is only correct if it is sent out within 2^30 ns after the
> > + * software timestamp is read. Past this window the frame is beyond
> > + * repair, and PM_SINGLE_STEP becomes safe to reprogram.
> > + */
> 
> [Severity: Medium]
> This comment argues that the correction value is beyond repair after 2^30 ns,
> but PM_SINGLE_STEP also carries the correction-field offset and the
> "update UDP checksum" bit, which the hardware still applies at SFD time to
> whatever frame is being transmitted.
> 
> If the previously programmed frame is still sitting in the switch or port
> egress path (egress congestion, flow control, TAS gating), the next
> netc_port_set_onestep_control() call can change offset and CH under it.  For
> an L2 PTP frame the correction offset is around 22, for UDP/IPv6 around 74,
> so the MAC would write 8 bytes of correction data at the wrong position
> inside the pending frame and may recompute its UDP checksum.
> 
> The "no ts_req_id available" branch of netc_port_onestep_work() has the same
> shape: it overwrites np->onestep_state with NETC_ONESTEP_SCHEDULED,
> which
> erases the record that a frame is in flight at all.
> 
> Would it be safer to only reprogram the register when the new frame's
> offset/CH match the pending one, or to hold off until the pending frame is
> known to be gone?

As stated in the comment, "Past this window the frame is beyond repair",
that is to say, when the sync frame leaves the port, the correction field
is not correct. When PM_SINGLE_STEP is programmed after a timeout,
there are essentially two possibilities: either the value of PM_SINGLE_STEP
remains unchanged, with no impact; or PM_SINGLE_STEP changes, in this
case the frame will be corrupted and either discarded by the sender or
receiver, which is as expected.

> 
> > +#define NETC_ONESTEP_VALID_WINDOW	0x40000000 /* ns */
> >
> >  static void netc_port_tstamp_timeout_work(struct work_struct *work)
> >  {
> > @@ -68,12 +77,276 @@ static int netc_get_ts_req_id(struct netc_port *np)
> >  	return ts_req_id;
> >  }
> >
> > +static int netc_get_phc_time(struct netc_switch *priv, u64 *ns)
> > +{
> > +	if (unlikely(!priv->tmr_dev))
> > +		return -ENODEV;
> > +
> > +	return netc_timer_get_current_time(priv->tmr_dev, ns);
> > +}
> > +
> > +static void netc_port_set_onestep_control(struct netc_port *np,
> > +					  bool csum_update, int offset)
> > +{
> > +	u32 val;
> > +
> > +	val = PM_SINGLE_STEP_EN | FIELD_PREP(PM_SINGLE_STEP_OFFSET,
> offset);
> > +	if (csum_update)
> > +		val |= PM_SINGLE_STEP_CH;
> > +	netc_mac_port_wr(np, NETC_PM_SINGLE_STEP(0), val);
> 
> [Severity: Low]
> This isn't a bug on today's hardware, but netc_mac_port_wr() returns without
> touching the register when is_netc_pseudo_port(np) is true, and it reports
> nothing back to the caller.
> 
> The one-step machinery is gated only on dsa_port_is_user(), and
> netc_get_ts_info() advertises HWTSTAMP_TX_ONESTEP_SYNC unconditionally,
> so a
> user port with caps.pseudo_link set would get its originTimestamp rewritten
> and a subtype-3 tag attached while the MAC single-step engine was never
> enabled.
> 
> That combination looks impossible right now: imx94_switch_phylink_get_caps()
> only marks port 3 (the CPU port) as internal, and the binding requires
> ethernet-port at 3 to carry the ethernet = <&enetc3> phandle.  Would it still be
> worth refusing one-step on pseudo-link ports explicitly so a future platform
> cannot silently drop the register write?

Currently, for i.MX94 only the CPU port is pseudo MAC, so no need to add
a such check in this stage. It is better to be added when future NETCs support
pseudo MAC on user port.

> 
> > +}
> > +
> > +static void netc_port_program_onestep(struct netc_port *np,
> > +				      struct sk_buff *skb)
> > +{
> > +	u16 correction_offset = NETC_SKB_CB(skb)->correction_offset;
> > +	u16 tstamp_offset = NETC_SKB_CB(skb)->timestamp_offset;
> > +	u64 tstamp = NETC_SKB_CB(skb)->tstamp;
> > +	u8 *hdr = skb_mac_header(skb);
> 
> [ ... ]
> 
> > +	if (NETC_SKB_CB(skb)->is_udp) {
> > +		__be32 old_sec_l, old_ns;
> > +		struct udphdr *uh;
> > +		__be16 old_sec_h;
> > +
> > +		if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > +			csum_update = true;
> > +			goto update_timestamp;
> > +		}
> > +
> > +		if (unlikely(!skb_transport_header_was_set(skb)))
> > +			uh = (struct udphdr *)(hdr + tstamp_offset -
> > +					       sizeof(struct ptp_header) -
> > +					       sizeof(struct udphdr));
> > +		else
> > +			uh = udp_hdr(skb);
> 
> [Severity: High]
> Can udp_hdr(skb) be trusted here?  netc_port_prepare_onestep_sync() derives
> and bounds-checks the PTP offsets itself:
> 
> 	if (unlikely(tstamp_offset + 10 > skb_headlen(skb) ||
> 		     correction_offset > NETC_MAX_STEP_OFFSET))
> 		goto set_ptp_flag_drop;
> 
> but skb->transport_header is never validated against skb_headlen(), and the
> safe computed expression is only used when the transport header was not set.
> The code then reads uh->check at offset 6 and writes 2 bytes there through
> inet_proto_csum_replace2()/inet_proto_csum_replace4().
> 
> Two AF_PACKET paths can leave transport_header at or past the tail while the
> frame still parses as a valid PTPv2 Sync over UDP with ip_summed ==
> CHECKSUM_NONE:
> 
> packet_snd()
>   packet_parse_headers()
>     skb_probe_transport_header()
>       __skb_flow_dissect()
>         key_control->thoff = min_t(u16, nhoff, skb ? skb->len : hlen);
> 
> The dissector runs with skb->protocol taken from the user-supplied
> sll_protocol and nhoff = 0, so a frame whose declared ethertype disagrees
> with its real headers can walk attacker-chosen IPv6 option hdrlen bytes and
> end with thoff clamped to skb->len, i.e. transport_header == tail.
> 
> The other path is PACKET_VNET_HDR:
> 
> virtio_net_hdr_to_skb()
>   skb_partial_csum_set()   /* csum_start = headlen - 2 accepted */
> ...
> skb_checksum_help()        /* flips ip_summed to CHECKSUM_NONE,
>                               transport_header left as-is */
> 
> With the frame length tuned so tailroom is under 8 bytes, wouldn't the
> uh->check read go past skb_tail_pointer() and the csum-replace write land
> inside struct skb_shared_info (flags/meta_len/nr_frags/tx_flags/gso_size)?
> 
> Since the validated expression is already computed just above, could it be
> used unconditionally instead of udp_hdr(skb)?  The same udp_hdr() pattern
> exists in drivers/net/ethernet/freescale/enetc/enetc.c, but this path is new
> here.

The AF_PACKET + flow-dissector path is real (transport_header can be clamped
to tail), but the OOB write can't happen:

All real reads/writes are mac-header relative, not transport_header relative.
ptp_parse_header() + ptp_classify_raw() work on actual content, and
tstamp_offset + 10 > skb_headlen() after skb_linearize_cow() bounds the whole
region. The correct UDP header is at ptp_hdr - sizeof(udphdr), so uh->check is
always in-bounds.

Hitting the udp_hdr(skb) branch as an exploit needs a frame that both passes
the mac-header bounds check and has transport_header at tail with a non-zero
uh->check - mutually inconsistent. So: not a High OOB write.

That said, your suggestion is right. The skb_transport_header_was_set() branch
is redundant and trusts an unvalidated transport_header. I'll drop it and use the
already-validated expression unconditionally.

> 
> > +
> > +		/* For IPv4, a UDP checksum of zero on the wire means "no
> > +		 * checksum". For IPv6, its UDP checksum is mandatory and
> > +		 * never zero.
> > +		 */
> > +		if (!uh->check)
> > +			goto update_timestamp;
> > +
> > +		old_sec_h = __get_unaligned_t(__be16, hdr + tstamp_offset);
> > +		old_sec_l = __get_unaligned_t(__be32, hdr + tstamp_offset + 2);
> > +		old_ns = __get_unaligned_t(__be32, hdr + tstamp_offset + 6);
> > +		inet_proto_csum_replace2(&uh->check, skb, old_sec_h,
> > +					 new_sec_h, false);
> > +		inet_proto_csum_replace4(&uh->check, skb, old_sec_l,
> > +					 new_sec_l, false);
> > +		inet_proto_csum_replace4(&uh->check, skb, old_ns,
> > +					 new_ns, false);
> > +		csum_update = true;
> > +	}
> > +
> > +update_timestamp:
> 
> [ ... ]
> 
> > +static void netc_port_onestep_work(struct work_struct *work)
> > +{
> 
> [ ... ]
> 
> > +skb_dequeue:
> > +	skb = __skb_dequeue(&np->onestep_queue);
> > +	if (!skb)
> > +		goto set_onestep_state_idle;
> > +
> > +	/* Clone is a ts_req_id token only; its payload is never read, so
> > +	 * sharing the buffer with the mutated original is fine.
> > +	 */
> > +	clone = skb_clone(skb, GFP_ATOMIC);
> > +	if (unlikely(!clone)) {
> > +		kfree_skb(skb);
> > +		goto skb_dequeue;
> > +	}
> > +
> > +	spin_lock_bh(&np->tstamp_lock);
> > +	ts_req_id = netc_get_ts_req_id(np);
> > +	if (unlikely(ts_req_id < 0)) {
> > +		spin_unlock_bh(&np->tstamp_lock);
> > +
> > +		/* Re-queuing the frame and immediately rescheduling the work
> > +		 * would busy-loop on system_percpu_wq and burn CPU until an
> > +		 * ID is freed, so drop this frame and move on to the next one
> > +		 * in the queue instead.
> > +		 */
> > +		np->onestep_state = NETC_ONESTEP_SCHEDULED;
> > +		schedule_work(&np->onestep_work);
> > +
> > +		goto onestep_unlock;
> > +	}
> 
> [Severity: Medium]
> The comment says re-queuing plus rescheduling would busy-loop, but the work
> does reschedule itself here unconditionally, and each pass destroys one
> queued one-step Sync frame at onestep_unlock.  With NETC_ONESTEP_QTH at
> 512,
> can this discard the entire backlog in quick succession?

That is intention if there is no available ts_req_id. If we do not discard the skb,
the work will be busy-loop until there is an available ts_req_id. But the issue is
that we do not know how long it will take to an available ts_req_id. So drop the
skb is a better choice.

> 
> The 16-entry ts_req_id pool is shared with the two-step path:
> netc_port_prepare_twostep() takes an ID for every PTP frame that is not a
> one-step Sync (Announce, Follow_Up, Delay_Resp still flow in one-step mode),
> and holds it for up to NETC_TSTAMP_TIMEOUT when a response is lost.  So a
> burst of two-step requests can drive this branch.
> 
> The only trace is the dev_dbg_ratelimited() inside netc_get_ts_req_id().
> Should ID exhaustion apply backpressure (leave the frame queued and arm a
> delayed retry) rather than dropping frames, and should the changelog mention
> that exhaustion means one-step Sync frames are discarded?

I don't think this makes much sense. Under normal circumstances, the frame is
sent out quickly, and if a situation arises where ts_req_id is unavailable, it will
recover quickly. Discarding the sync frame won't cause any problems, and
subsequent sync frames will still maintain PTP synchronization. If a timeout
occurs, PTP synchronization is already affected, so discarding the message is
even less of a problem.

> 
> > +
> > +	/* PHC is unavailable, drop the whole queue */
> > +	if (unlikely(netc_get_phc_time(priv, &tstamp))) {
> > +		spin_unlock_bh(&np->tstamp_lock);
> > +		goto set_onestep_state_idle;
> > +	}
> > +
> > +	NETC_SKB_CB(skb)->tstamp = tstamp;
> > +	NETC_SKB_CB(skb)->ts_req_id = ts_req_id;
> > +	NETC_SKB_CB(skb)->ptp_flag = NETC_PTP_FLAG_ONESTEP;
> > +	NETC_SKB_CB(clone)->ts_req_id = ts_req_id;
> > +	NETC_SKB_CB(clone)->ptp_tx_time = jiffies_64;
> > +	NETC_SKB_CB(clone)->ptp_flag = NETC_PTP_FLAG_ONESTEP;
> > +	np->onestep_tx_time = NETC_SKB_CB(clone)->ptp_tx_time;
> > +	np->onestep_ts_req_id = ts_req_id;
> > +
> > +	__skb_queue_tail(&np->tstamp_queue, clone);
> > +	if (!delayed_work_pending(&np->tstamp_timeout_work))
> > +		schedule_delayed_work(&np->tstamp_timeout_work,
> > +				      NETC_TSTAMP_TIMEOUT);
> > +
> > +	spin_unlock_bh(&np->tstamp_lock);
> > +
> > +	np->onestep_state = NETC_ONESTEP_IN_FLIGHT;
> > +	spin_unlock_bh(&np->onestep_lock);
> > +
> > +	netc_port_program_onestep(np, skb);
> > +	tagger_data = priv->ds->tagger_data;
> > +	tagger_data->onestep_sync_xmit(skb, np->dp->user);
> > +
> > +	return;
> > +
> > +set_onestep_state_idle:
> > +	np->onestep_state = NETC_ONESTEP_IDLE;
> > +purge_onestep_queue:
> > +	__skb_queue_purge(&np->onestep_queue);
> > +onestep_unlock:
> > +	spin_unlock_bh(&np->onestep_lock);
> > +	kfree_skb(skb);
> > +	kfree_skb(clone);
> > +}
> > +
> > +static bool netc_onestep_timeout(struct netc_port *np)
> > +{
> > +	u64 expire_time;
> > +
> > +	/* Use monotonic jiffies_64, as the PHC may be stepped backwards.
> > +	 * Add one tick since the ns-to-jiffies conversion rounds down, so
> > +	 * the software window is never shorter than the hardware window.
> > +	 */
> > +	expire_time = np->onestep_tx_time + 1 +
> > +		      nsecs_to_jiffies64(NETC_ONESTEP_VALID_WINDOW);
> > +	if (np->onestep_state == NETC_ONESTEP_IN_FLIGHT &&
> > +	    time_after64(jiffies_64, expire_time))
> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> > +void netc_port_onestep_sync_enqueue(struct dsa_switch *ds, int port,
> > +				    struct sk_buff *skb)
> > +{
> > +	struct netc_port *np = NETC_PORT(ds, port);
> > +
> > +	spin_lock_bh(&np->onestep_lock);
> > +	if (unlikely(np->onestep_state == NETC_ONESTEP_PORT_INACTIVE)) {
> > +		kfree_skb(skb);
> > +		goto onestep_unlock;
> > +	}
> > +
> > +	if (unlikely(skb_queue_len(&np->onestep_queue) >= NETC_ONESTEP_QTH))
> {
> > +		dev_dbg_ratelimited(np->switch_priv->dev,
> > +				    "The onestep_queue of port %d is full\n",
> > +				    port);
> > +		kfree_skb(skb);
> > +		goto onestep_unlock;
> > +	}
> > +
> > +	__skb_queue_tail(&np->onestep_queue, skb);
> > +	if (likely(np->onestep_state == NETC_ONESTEP_IDLE) ||
> > +	    netc_onestep_timeout(np)) {
> > +		np->onestep_state = NETC_ONESTEP_SCHEDULED;
> > +		schedule_work(&np->onestep_work);
> > +	}
> > +
> > +onestep_unlock:
> > +	spin_unlock_bh(&np->onestep_lock);
> > +}
> 
> [Severity: High]
> Can the port get stuck in NETC_ONESTEP_IN_FLIGHT forever?

Yes, this is a potential issue, I will fix it.

> 
> Once netc_port_onestep_work() sets IN_FLIGHT and hands the frame to
> onestep_sync_xmit(), only three things can resume the drain:
> 
>   1. a matching response in netc_port_txtstamp_handler()
>   2. netc_onestep_timeout(), which is evaluated only from
>      netc_port_onestep_sync_enqueue()
>   3. the ts_req_id exhaustion self-reschedule
> 
> The changelog says:
> 
>     A dropped frame produces no response, so reuse tstamp_timeout_work to
>     reclaim its request ID after 5 seconds
> 
> but netc_port_tstamp_timeout_work() only unlinks and frees the timed-out
> clone:
> 
> 	__skb_unlink(skb, &np->tstamp_queue);
> 	__skb_queue_tail(&free_list, skb);
> 	...
> 	if (!skb_queue_empty(&np->tstamp_queue))
> 		schedule_delayed_work(&np->tstamp_timeout_work,
> 				      NETC_TSTAMP_TIMEOUT);
> 
> It never touches np->onestep_state and never schedules np->onestep_work.
> And once the clone has been reaped, a late response takes the !skb_match
> early return in netc_port_txtstamp_handler(), so it cannot clear IN_FLIGHT
> either.
> 
> Concretely: frame A goes IN_FLIGHT with id X, frame B is enqueued inside the
> window and only appended, A is dropped in the switch so no response arrives,
> 5 seconds later the timeout work frees A's clone and leaves IN_FLIGHT set.
> If the sender then stops, or falls below one frame per window, doesn't B and
> everything behind it stay queued and unfreed until link-down, port disable
> or a hwtstamp reconfiguration?  These are the original TX skbs and still
> hold skb->sk, so the owning socket stays charged.
> 
> The queue-full branch above also returns before the netc_onestep_timeout()
> check, so once 512 frames have accumulated even continued traffic can no
> longer recover the port.
> 
> Would it work to reset onestep_state and schedule onestep_work from the
> timeout path, or to arm a delayed work for the validity deadline, so
> recovery does not depend on new traffic arriving?
> 
> Separately, the changelog says the work is scheduled "when the PTP time has
> advanced beyond NETC_ONESTEP_VALID_WINDOW", while
> netc_onestep_timeout()
> compares monotonic jiffies_64 and only at enqueue time.
> 
> [ ... ]
> 
> > @@ -395,6 +673,66 @@ int netc_port_hwtstamp_get(struct dsa_switch *ds,
> int port,
> >  	return 0;
> >  }
> >
> > +static void netc_port_prepare_onestep_sync(struct netc_port *np,
> > +					   struct sk_buff *skb,
> > +					   u32 ptp_class, bool *twostep)
> > +{
> 
> [ ... ]
> 
> > +	if (unlikely(skb_linearize_cow(skb)))
> > +		goto set_ptp_flag_drop;
> > +
> > +	ptp_hdr = ptp_parse_header(skb, ptp_class);
> > +	if (unlikely(!ptp_hdr))
> > +		goto set_ptp_flag_drop;
> > +
> > +	msg_type = ptp_get_msgtype(ptp_hdr, ptp_class);
> 
> [ ... ]
> 
> > +set_ptp_flag_drop:
> > +	/* Drop instead of falling back to two-step: if it is a Sync,
> > +	 * one-step offload will not be executed, the timestamp in the
> > +	 * frame is inaccurate, which may affect PTP synchronization.
> > +	 */
> > +	NETC_SKB_CB(skb)->ptp_flag = NETC_PTP_FLAG_DROP;
> > +	dev_dbg_ratelimited(priv->dev,
> > +			    "Port %d: PTP frame dropped in error\n",
> > +			    np->dp->index);
> > +}
> 
> [Severity: Medium]
> The skb_linearize_cow() failure is evaluated before the message type is
> known, so a plain ENOMEM destroys any PTP frame on the port, including a
> Delay_Req that this code would otherwise have sent two-step.  Should the
> linearize failure fall back to two-step, or at least be handled after the
> Sync check?

No, first, the frame might be a sync frame, if fall back to two-step, that means
one-step offload will not take effect, so the timestamp of the sync frame is not
correct. If the PTP clocks are already synced, a frame with an incorrect timestamp
may cause the clocks desynchronization. In this case, drop the frame is better
than sending it.

Second, the purpose of skb_linearize_cow() is to ensure that the required
data is in the linear buffer. If a check is performed first, it defeats the purpose
of using skb_linearize_cow(), because it cannot be guaranteed that the
required data is in the linear buffer at this time.

> 
> None of the new drop paths are accounted anywhere.  dsa_user_xmit() has
> already counted the frame as transmitted via dev_sw_netstats_tx_add() before
> the tagger frees it, so from userspace the frames just vanish:
> 
>   netc_port_prepare_onestep_sync() -> NETC_PTP_FLAG_DROP -> kfree_skb()
> in
>     netc_xmit()                               (linearize/parse/offset
> checks)
>   netc_port_onestep_sync_enqueue()            (queue full, port inactive)
>   netc_port_onestep_work()                    (clone failure, no
> ts_req_id,
>                                                PHC read failure
> purges all)
>   netc_port_purge_onestep_queue()             (hwtstamp
> reconfiguration)
> 
> netc has no .get_stats64, so dsa_user_get_stats64() falls back to
> dev_get_tstats64(), which folds dev->stats into rtnl_link_stats64 - could
> these paths bump dev->stats.tx_dropped?
> 
> The changelog only says "Frames that are not one-step Sync still fall back
> to two-step timestamping", which does not tell a reader that enabling
> one-step turns a degraded timestamp into a frame that is never transmitted.
> 
> [ ... ]
> 
> > @@ -449,6 +787,21 @@ void netc_port_txtstamp_handler(struct dsa_switch
> *ds, int port,
> >  		dev_dbg_ratelimited(np->switch_priv->dev,
> >  				    "Port %d ts_req_id %u which seems lost\n",
> >  				    port, ts_req_id);
> > +
> > +		return;
> > +	}
> > +
> > +	if (NETC_SKB_CB(skb_match)->ptp_flag == NETC_PTP_FLAG_ONESTEP) {
> > +		spin_lock_bh(&np->onestep_lock);
> > +		if (likely(np->onestep_state == NETC_ONESTEP_IN_FLIGHT &&
> > +			   np->onestep_ts_req_id == ts_req_id) ||
> > +		    np->onestep_state == NETC_ONESTEP_IDLE) {
> > +			np->onestep_state = NETC_ONESTEP_SCHEDULED;
> > +			schedule_work(&np->onestep_work);
> > +		}
> > +		spin_unlock_bh(&np->onestep_lock);
> > +		consume_skb(skb_match);
> > +
> >  		return;
> >  	}
> 
> [Severity: Medium]
> Can the ts_req_id comparison here match a different frame?  The clone is
> unlinked from np->tstamp_queue under tstamp_lock earlier in this function,
> which immediately makes the 4-bit id reusable, and onestep_lock is only
> taken afterwards:
> 
> CPU0 (work, frame A response overdue past the window)
>   netc_port_onestep_sync_enqueue()
>      netc_onestep_timeout() -> true, schedule_work()
>   netc_port_onestep_work()
>      spin_lock_bh(&np->onestep_lock);
> 
> CPU1 (A's response arrives)
>   netc_port_txtstamp_handler()
>      __skb_unlink(clone_A, &np->tstamp_queue);   /* id X now free */
>      spin_unlock_bh(&np->tstamp_lock);
>      spin_lock_bh(&np->onestep_lock);            /* blocks */
> 
> CPU0 continues
>      netc_get_ts_req_id() -> X again for frame B
>      np->onestep_ts_req_id = X;
>      np->onestep_state = NETC_ONESTEP_IN_FLIGHT;
>      spin_unlock_bh(&np->onestep_lock);
>      onestep_sync_xmit(B)
> 
> CPU1 then sees IN_FLIGHT with onestep_ts_req_id == X and treats A's
> response as B's completion, sets SCHEDULED and re-arms the work, which can
> transmit frame C and reprogram PM_SINGLE_STEP while B is still pending in
> hardware.  Workqueue non-reentrancy serializes the worker, not frame
> completion.
> 
> Would retiring the id and moving the one-step state under a single lock
> order (onestep_lock then tstamp_lock), or matching on a generation counter
> instead of the reusable 4-bit id, close this?
> 

This is a hardware limitation, the response frame only contains ts_req_id
and timestamp. the driver can only use the ts_req_id to match the skb from
the queue. The incorrect matches can occur, but only in the event of a
timeout. In the event of a timeout, PTP synchronization has already been
affected, so incorrect matches will not cause any worse consequences.
Once the link recovers, the timeout disappears, and subsequent PTP
synchronization resumes. Therefore, it's a trade-off.