Re: [PATCH net-next v4 07/11] net: dsa: microchip: extract compute_width

[Bastien Curutchet]

Hi all,

On 9/29/26 2:28 PM, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 1 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] The new check in ksz_ptp_compute_perout_cycle() does not fully
>    stop the divide-by-zero on restart that the commit message says it…
> - [Medium] The new `request->period.sec < 0` check in
>    ksz_ptp_compute_perout_cycle() fixes a crash that userspace can…
>
> --
>
> net: dsa: microchip: extract compute_width
>
> This moves the period and pulse width calculation out of
> ksz_ptp_enable_perout() into a new helper, ksz_ptp_compute_perout_cycle().
> The helper will be reused later for KSZ8463 periodic output support. It
> also adds checks that reject negative or zero periods, so that a restart
> of the periodic output cannot divide by zero.
>
> > diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> > index 79520d345efcf..4b57cf076bb4f 100644
> > --- a/drivers/net/dsa/microchip/ksz_ptp.c
> > +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> > @@ -203,12 +203,50 @@ static int ksz_ptp_get_pin(struct ksz_device *dev,
> >   	return pin;
> >   }
> >   
> > +static int ksz_ptp_compute_perout_cycle(struct ksz_device *dev,
> > +					struct ptp_perout_request const *request,
> > +					u64 max_pulse_width,
> > +					u64 *cycle_width_ns,
> > +					u64 *pulse_width_ns)
> > +{
> > +	struct timespec64 tmp;
> > +
> > +	if (request->period.sec < 0)
> > +		return -EINVAL;
>
> [Severity: Medium]
> Is this check fixing a crash in the existing code? If so, should it go to
> net with a Fixes: tag?

I added these checks to address Sashiko comments from last iteration. I 
don't think they worth a fix in net.

[...]

> [Severity: High]
> Can a zero cycle width still get through here on 32-bit kernels?
>
> ptp_perout_request.period.nsec is a __u32. Neither ptp_perout_request()
> nor the sysfs period_store() checks that it is below NSEC_PER_SEC. On
> 32-bit, struct timespec64.tv_nsec is a signed 32-bit long, so the
> assignment to tmp.tv_nsec can wrap to a negative value.
>
> Take period.sec = 1 and period.nsec = 3294967296 (0xC4653600). Both new
> checks pass. tmp.tv_nsec becomes -1000000000, so timespec64_to_ns()
> returns 0, and the TRIG_CYCLE_WIDTH_M check accepts 0. The pair
> sec = 2, nsec = 2962947296 also gives 0.

In these two cases the nsec field is greater than one second, it seems 
very unlikely to me to receive this kind of request.


Best regards,
Bastien