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

> > @@ -1005,6 +1028,19 @@ static void netc_free_ports_resources(struct
> netc_switch *priv)
> >  			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?

> 
> [ ... ]
> > @@ -1579,6 +1622,7 @@ static int netc_port_enable(struct dsa_switch *ds,
> int port,
> >  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)
> >  	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
> > index 1384a6f31d1c1..d0423b3c8c33a 100644
> > --- a/drivers/net/dsa/netc/netc_ptp.c
> > +++ b/drivers/net/dsa/netc/netc_ptp.c
> [ ... ]
> > +static void netc_port_program_onestep(struct netc_port *np,
> > +				      struct netc_onestep *onestep,
> > +				      struct sk_buff *skb,
> > +				      u64 tstamp)
> > +{
> [ ... ]
> > +		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.

> 
> [ ... ]
> > +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.