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

quoted

+static void netc_port_program_onestep(struct netc_port *np,
+				      struct sk_buff *skb)
+{

[ ... ]

quoted

+		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);

[Severity: Medium]
Does modifying the payload of a cloned SKB here violate the networking core
SKB buffer ownership guidelines?

The original SKB was cloned earlier in netc_port_onestep_work(), making its
data buffer shared (skb_cloned(skb) evaluates to true). Bypassing standard
buffer ownership rules and directly modifying the shared payload triggers an
expensive reallocation and copy (pskb_expand_head) in downstream conduit
drivers that check skb_cloned() before padding or modifying the frame, which
could severely degrade one-step PTP performance.

As stated in netc_port_onestep_work(): "Clone is a ts_req_id token only; its
payload is never read, so sharing the buffer with the mutated original is fine."

The claimed downstream "unshare penalty" also does not occur: the conduit
(ENETC pseudo MAC) transmit path does not check skb_cloned() and does not
call pskb_expand_head() — it only DMA-maps and sends, never rewriting the
payload. So no reallocation is triggered regardless.

And PTP sync packets are very infrequently (for example, ptp4l supports 128
pps), so I do not think this will degrade one-step PTP performance.

[ ... ]

quoted

+static void netc_port_onestep_work(struct work_struct *work) {
+	struct netc_port *np = container_of(work, struct netc_port,
+					    onestep_work);

[ ... ]

quoted

+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);

[Severity: Medium]
Could this clone operation lead to the downstream buffer unsharing penalties
described above?

Since the clone is only used as a timeout token and its payload is never read,
would allocating a dummy lightweight SKB via alloc_skb(0, GFP_ATOMIC)
exclusively to hold the token avoid these performance penalties and
architectural ownership violations?

As explained above, the conduit (ENETC pseudo MAC) transmit path never
rewrites the payload. No reallocation is triggered regardless. So I don't think
this is a problem that needs to be solved. If netdev-ai.bots reports other
issues that need to be addressed, I think we can improve this logic while
we're at it.