RE: [PATCH net] ptp: netc: fix period truncation and potential divide-by-zero in PEROUT
Wei Fang <[email protected]>
| Newsgroups | org.kernel.vger.netdev,dev.linux.lists.imx,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <GV2PR04MB11739C05FD4C2D5A8B477B82D88A32@GV2PR04MB11739.eurprd04.prod.outlook.com> |
NXP Confidential > On Tue, Aug 18, 2026 at 11:33:41AM +0800, [email protected] wrote: > > From: Wei Fang <[email protected]> > > > > The max_period bound in net_timer_enable_perout() was computed as: > > > > max_period = (u64)NETC_TMR_DEFAULT_FIPER + integral_period; > > > > which exceeds U32_MAX when integral_period > 0 (e.g. 0x100000002 for > > the default 333333333 Hz clock). A period_ns that passes this check but > > exceeds U32_MAX is then silently truncated when stored into the u32 > > struct netc_pp::period field. > > > > A truncated value of zero can reach netc_timer_set_perout_alarm(), where > > the local u32 period variable would also be 0, causing a divide-by-zero > > in roundup_u64(delta, period) whenever the stime < min_time branch is > > taken (which always happens for a start time of {0, 0}). > > > > Additionally, netc_timer_enable_periodic_pulse() and > > netc_timer_enable_fiper() both compute: > > > > fiper = pp->period - integral_period; > > > > A zero pp->period results in an unsigned wraparound to 0xFFFFFFFD, > > mis-programming the FIPER hardware register. > > > > Fix all three issues by capping max_period at NETC_TMR_DEFAULT_FIPER > > (0xFFFFFFFF). This ensures that any period_ns passing the range check > > fits in a u32 without truncation, so the stored value is always valid > > and non-zero. The accepted range is reduced by integral_period ns > > (typically only a few nanoseconds), which is negligible in practice. > > > > Fixes: 671e266835b8 ("ptp: netc: add periodic pulse output support") > > Signed-off-by: Wei Fang <[email protected]> > > If so, then I think NETC_TMR_DEFAULT_FIPER can simply be used in > place of max_period. Yes, I also considered this modification, but ultimately chose this lazy approach. Since you brought it up, I will improve it in the next version. Thanks. :) > > Something like this (completely untested!): > > diff --git a/drivers/ptp/ptp_netc.c b/drivers/ptp/ptp_netc.c > index 28889b7b3e15..66d1f0532585 100644 > --- a/drivers/ptp/ptp_netc.c > +++ b/drivers/ptp/ptp_netc.c > @@ -434,22 +434,22 @@ > } > > if (on) { > - u64 period_ns, gclk_period, max_period, min_period; > + u64 period_ns, gclk_period, min_period; > struct timespec64 period, stime; > u32 integral_period; > int alarm_id; > > period.tv_sec = rq->perout.period.sec; > period.tv_nsec = rq->perout.period.nsec; > period_ns = timespec64_to_ns(&period); > > integral_period = netc_timer_get_integral_period(priv); > - max_period = NETC_TMR_DEFAULT_FIPER; > gclk_period = netc_timer_get_gclk_period(priv); > min_period = gclk_period * 4 + integral_period; > - if (period_ns > max_period || period_ns < min_period) { > + if (period_ns > NETC_TMR_DEFAULT_FIPER || > + period_ns < min_period) { > dev_err(dev, "The period range is %llu ~ %lu\n", > - min_period, max_period); > + min_period, NETC_TMR_DEFAULT_FIPER); > err = -EINVAL; > goto unlock_spinlock; > }