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

quoted

@@ -1005,6 +1028,19 @@ static void netc_free_ports_resources(struct

netc_switch *priv)

quoted

 			continue;

 		netc_port_purge_txtstamp_queue(np);
+
+		/* dsa_tree_teardown() calls dsa_tree_teardown_ports() before
+		 * dsa_tree_teardown_switches(), so netc_port_disable() is
+		 * executed before netc_teardown() and purges onestep->queue,
+		 * so here we only need to drop the port's owner reference.
+		 * In-flight one-step skbs still hold references via the
+		 * destructor; the context (and its work) is freed only after
+		 * the conduit frees the last in-flight skb. By then np may
+		 * be gone, but the work no longer dereferences np because
+		 * onestep->active has been cleared.
+		 */
+		netc_onestep_put(np->onestep);
+		np->onestep = NULL;
 	}
 }

[Severity: High]
Does unloading the netc driver module wait for in-flight one-step Sync SKBs
to complete transmission by the conduit device?

Because the driver drops its reference here and allows the module unload to
complete, the kernel could attempt to execute netc_onestep_skb_destructor()
after the module memory has been unmapped when the conduit eventually
frees
the SKB, leading to a panic.

This is a theoretical issue, I do not think this issue will occur in practical use,
and there won't even be a use case where the switch driver is uninstalled
while running PTP.

I can fix this theoretically existing issue, for example, by calling
try_module_get() in netc_port_onestep_alloc() and module_put() in
netc_onestep_destroy_work().

But what I'm really concerned about is that AI is currently reporting many
theoretical problems that don't actually occur in practice. Fixing these
problems involves introducing a lot of largely useless code, and the effort
expended is far less than the final benefit—is it really worth it?

[ ... ]

quoted

@@ -1579,6 +1622,7 @@ static int netc_port_enable(struct dsa_switch *ds,

int port,

quoted

 static void netc_port_disable(struct dsa_switch *ds, int port)
 {
 	struct netc_port *np = NETC_PORT(ds, port);
+	struct netc_onestep *onestep = np->onestep;

 	/* When .port_disable() is called, .port_enable() may not have been
 	 * called. In this case, both the prepare_count and enable_count of

@@ -1588,6 +1632,13 @@ static void netc_port_disable(struct dsa_switch

*ds, int port)

quoted

 	if (!np->enable)
 		return;

+	if (onestep) {
+		mutex_lock(&onestep->work_lock);
+		onestep->active = false;
+		netc_port_purge_onestep_queue(onestep, true);
+		mutex_unlock(&onestep->work_lock);
+	}
+
 	clk_disable_unprepare(np->ref_clk);
 	np->enable = false;
 }

diff --git a/drivers/net/dsa/netc/netc_ptp.c

b/drivers/net/dsa/netc/netc_ptp.c

quoted

index 1384a6f31d1c1..d0423b3c8c33a 100644

--- a/drivers/net/dsa/netc/netc_ptp.c
+++ b/drivers/net/dsa/netc/netc_ptp.c

[ ... ]

quoted

+static void netc_port_program_onestep(struct netc_port *np,
+				      struct netc_onestep *onestep,
+				      struct sk_buff *skb,
+				      u64 tstamp)
+{

[ ... ]

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

[Severity: Medium]
Could this manual software modification of the UDP checksum evaluate to 0?

A UDP checksum of 0 is invalid for IPv6 and disables checksums for IPv4.
Does this require a check to convert a 0 result into CSUM_MANGLED_0
(0xFFFF)
to prevent non-compliant packets?

This is a false positive, the hardware will modify the correction field and
update the checksum.

[ ... ]

quoted

+void netc_port_purge_onestep_queue(struct netc_onestep *onestep,
+				   bool clear_flight)
+{
+	struct sk_buff_head free_list;
+
+	__skb_queue_head_init(&free_list);
+
+	spin_lock_bh(&onestep->queue_lock);
+	skb_queue_splice_init(&onestep->queue, &free_list);
+	if (clear_flight)
+		onestep->in_flight = false;
+	spin_unlock_bh(&onestep->queue_lock);
+
+	__skb_queue_purge(&free_list);
+}

[Severity: High]
Does forcefully clearing the in_flight flag here during port disablement
allow a new frame to be dequeued if the port is quickly brought back up?

If so, a new frame could be programmed into the shared SINGLE_STEP
hardware
register while the previous frame is still physically in flight in the conduit.
Could this break the 1-to-1 serialization required by the hardware and lead to
corrupted timestamps?

This is a potential issue, I will fix it.