Re: [PATCH net-next v9 4/7] idpf: support pacing offload

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

netdev-bot+sashiko@ wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 · Medium: 1 · Low: 2
> 
> - [High] idpf advertises a non-zero netdev->max_pacing_offload_horizon
>   (and therefore programs absolute CLOCK_TAI departure times into the…
> - [Medium] The EDT horizon advertised to the networking core is taken
>   verbatim from firmware and only bounds-checked against U32_MAX, while…
> - [Low] idpf_vport_set_pacing_offload() writes
>   netdev->max_pacing_offload_horizon (idpf_lib.c:832) from the…
> - [Low] In idpf_send_get_edt_caps_msg() the firmware-supplied granularity
>   is a u64 (gran_ns = le64_to_cpu(caps.tstamp_granularity_ns)) but is…
> 
> --
> 
> idpf: support pacing offload
> 
> This adds EDT (earliest departure time) support to idpf: EDT capabilities
> are negotiated over virtchnl, the negotiated horizon is published as
> netdev->max_pacing_offload_horizon, and skb->tstamp is programmed into the
> 23-bit timestamp field of the flow scheduling Tx descriptor.
> 
> A few questions below on the capability gating and on the horizon
> encoding.
> 
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > index 827c795afcb62..cd1b173e3b276 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > @@ -817,6 +817,21 @@ static void idpf_attach_and_open(struct idpf_adapter *adapter)
> >  	}
> >  }
> >  
> > +static void idpf_vport_set_pacing_offload(struct idpf_vport *vport,
> > +					  struct net_device *netdev)
> > +{
> > +	struct idpf_adapter *adapter = vport->adapter;
> > +	u32 max_horizon = 0;
> > +
> > +	if (adapter->edt_caps.time_horizon_ns &&
> > +	    idpf_is_queue_model_split(vport->dflt_qv_rsrc.txq_model) &&
> > +	    !idpf_is_cap_ena(adapter, IDPF_OTHER_CAPS,
> > +			     VIRTCHNL2_CAP_SPLITQ_QSCHED))
> > +		max_horizon = adapter->edt_caps.time_horizon_ns;
> > +
> 
> [Severity: High]
> The commit message says:
> 
>     Negotiate the feature with the device using virtchnl. Support is
>     conditional on
>     ...
>     - PTP to ensure the NIC clock is synced to CLOCK_TAI.

This is the only [High] in the series.

The issue is that PTP support is expected, but this is not an explicit
condition checked before advertising EDT.

Previous versions of the commit message explicitly stated that
preconditions are not always explicitly verified (also: split-queue,
flow-based scheduling, firmware advertises EDT):

  "Do not explicitly check all preconditions. Trust the firmware to
   only advertise EDT when all are met."

The patch now verifies these three other preconditions, so I dropped
that statement from the commit message (on flagging by another bot).

Only now it flags that one condition is still not checked.
If I have to respin, I'll add back a comment.
 
> [Severity: Medium]
> Can the advertised horizon exceed what the descriptor can encode?  The
> hardware field is 23 bits of granularity units:
> 
> 	drivers/net/ethernet/intel/idpf/idpf_lan_txrx.h:
> 		/* [23:23] Horizon Overflow bit, [22:0] timestamp */
> 		u8 ts[3];

In practice this is not a concern with this hardware. The driver
does not check every condition of a potential faulty or malicious
firmware.
 
> [Severity: Low]
> Does this validate a different value than the one it then uses?  gran_ns is
> a u64, but is_power_of_2() takes unsigned long:
> 
> 	include/linux/log2.h:
> 	static __always_inline __attribute__((const))
> 	bool is_power_of_2(unsigned long n)
> 	{
> 		return n - 1 < (n ^ (n - 1));
> 	}
> 
> IDPF has no "depends on 64BIT" in its Kconfig, so on 32-bit builds the
> upper 32 bits are dropped for the check while ilog2(gran_ns) takes the
> __ilog2_u64 path on the full value.  A response of 0x100000001 truncates to
> 1, passes the check, and yields tstamp_granularity_pow2 = 32, which is
> copied into idpf_tx_queue.ts_gran_pow2 and used as the shift in
> idpf_tx_splitq_set_txtime().  The same truncation rejects a legitimate
> granularity of 2^32 on 32-bit.  Would testing the u64 directly, e.g.
> gran_ns & (gran_ns - 1), plus an upper bound, be better here?

Low severity, but good idea if respinning.