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;
>               }
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.