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

[Stephen Hemminger <stephen@networkplumber.org>]

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: