Re: [PATCH v1] net/idpf: harden PTP frequency adjustment

[Bruce Richardson <bruce.richardson@intel.com>]

On Tue, Apr 07, 2026 at 08:14:45AM -0700, Stephen Hemminger wrote:
> On Thu, 12 Mar 2026 11:57:33 +0000
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > On Thu, Mar 12, 2026 at 02:43:44PM -0400, Soumyadeep Hore wrote:
> > > Normalize ppm to an unsigned magnitude before using it in the
> > > increment value scaling path.
> > > 
> > > This avoids negating INT64_MIN and also prevents subtracting 62
> > > from the reduced log sum unless the sum is still above the
> > > overflow threshold reported by Coverity.
> > > 
> > > Coverity issue: 501832
> > > 
> > > Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
> > > ---
> > >  drivers/net/intel/idpf/idpf_ethdev.c | 32 +++++++++++++++++-----------
> > >  1 file changed, 20 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/net/intel/idpf/idpf_ethdev.c b/drivers/net/intel/idpf/idpf_ethdev.c
> > > index 5e57a45775..1c5bd2ee12 100644
> > > --- a/drivers/net/intel/idpf/idpf_ethdev.c
> > > +++ b/drivers/net/intel/idpf/idpf_ethdev.c
> > > @@ -1007,7 +1007,7 @@ idpf_timesync_adjust_freq(struct rte_eth_dev *dev, int64_t ppm)
> > >  	struct idpf_ptp *ptp = adapter->ptp;
> > >  	int64_t incval, diff = 0;
> > >  	bool negative = false;
> > > -	uint64_t div, rem;
> > > +	uint64_t abs_ppm, div, rem;
> > >  	uint64_t divisor = 1000000ULL << 16;
> > >  	int shift;
> > >  	int ret;
> > > @@ -1016,26 +1016,34 @@ idpf_timesync_adjust_freq(struct rte_eth_dev *dev, int64_t ppm)
> > >  
> > >  	if (ppm < 0) {
> > >  		negative = true;
> > > -		ppm = -ppm;
> > > +		abs_ppm = ppm == INT64_MIN ? (uint64_t)INT64_MAX + 1 :
> > > +			(uint64_t)(-ppm);
> > > +	} else {
> > > +		abs_ppm = (uint64_t)ppm;
> > >  	}
> > >  
> > >  	/* can incval * ppm overflow ? */
> > > -	if (rte_log2_u64(incval) + rte_log2_u64(ppm) > 62) {
> > > -		rem = ppm % divisor;
> > > -		div = ppm / divisor;
> > > +	if (rte_log2_u64(incval) + rte_log2_u64(abs_ppm) > 62) {
> > > +		rem = abs_ppm % divisor;
> > > +		div = abs_ppm / divisor;
> > >  		diff = div * incval;
> > > -		ppm = rem;
> > > +		abs_ppm = rem;
> > > +
> > > +		if (abs_ppm != 0) {
> > > +			uint32_t log_sum;
> > >  
> > > -		shift = rte_log2_u64(incval) + rte_log2_u64(ppm) - 62;
> > > -		if (shift > 0) {
> > > -			/* drop precision */
> > > -			ppm >>= shift;
> > > -			divisor >>= shift;
> > > +			log_sum = rte_log2_u64(incval) + rte_log2_u64(abs_ppm);
> > > +			if (log_sum > 62) {
> > > +				shift = log_sum - 62;
> > > +				/* drop precision */
> > > +				abs_ppm >>= shift;
> > > +				divisor >>= shift;
> > > +			}
> > >  		}
> > >  	}
> > >  
> > >  	if (divisor)
> > > -		diff = diff + incval * ppm / divisor;
> > > +		diff = diff + incval * abs_ppm / divisor;
> > >    
> > 
> > Asking Claude AI about this code and how to simplify it a bit while also
> > avoiding the INT64_MIN issue, it suggests using __int128 type, giving much
> > shorter code:
> > 
> > 	__int128 diff;
> > 	int ret;
> > 
> > 	/*
> > 	 * ppm is scaled ppm: 1 ppm = 2^16 units (matching Linux adjfine).
> > 	 * Use __int128 to avoid overflow without complex split/shift logic,
> > 	 * and to correctly handle ppm == INT64_MIN.
> > 	 */
> > 	incval = ptp->base_incval;
> > 	diff = ((__int128)incval * ppm) / (1000000LL << 16);
> > 	incval += (int64_t)diff;
> > 
> > 	ret = idpf_ptp_adj_dev_clk_fine(adapter, incval);
> > 	if (ret != 0)
> > 		PMD_DRV_LOG(ERR, "PTP failed to set incval, err %d", ret);
> > 	return ret;
> > 
> > Maybe worth considering?
> > 
> > /Bruce
> 
> Claude AI review of this patch spotted some possible math issues.
> 
> 
> 
> Normalize ppm to an unsigned magnitude before using it in the
> increment value scaling path.
> This avoids negating INT64_MIN and also prevents subtracting 62
> from the reduced log sum unless the sum is still above the
> overflow threshold reported by Coverity.
> 
> The simplification is a nice improvement and the __int128 approach
> is correct for realistic hardware values.
> 
> One concern:
> Warning: drivers/net/intel/idpf/idpf_ethdev.c
> The cast (int64_t)diff silently truncates if diff does not fit in
> 64 bits.  The __int128 multiplication itself never overflows (both
> UINT64_MAX × INT64_MAX and UINT64_MAX × INT64_MIN fit within signed
> __int128), but after dividing by 1000000LL << 16 the quotient can
> still exceed INT64_MAX when incval is large and ppm is extreme.
> For realistic PTP hardware (base_incval around 2^32) this is fine,
> but the API accepts the full int64_t range without documented bounds.
> Adding an explicit clamp or assertion before the cast would preserve
> the defensive character of the code being replaced:

Sadly, the CI is also complaining. It seems that __int128 doesn't seem to
be available for 32-bit ARM. :-( Not sure what the best approach here is,
whether to add a more complicated fallback path for that case, or just
disallow the timesync on the unsupported platforms for now. [32-bit x86
doesn't seem to have issues, for example]

/Bruce