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

[Bastien Curutchet <bastien.curutchet@bootlin.com>]

Hi all,

On 9/29/26 2:28 PM, netdev-bot+sashiko@kernel.org 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