[PATCH net] stmmac: fix potential division by 0

[PATCH net] stmmac: fix potential division by 0

[Piergiorgio Beruto]

Depending on the HW platform and configuration, the
stmmac_config_sub_second_increment() function may return 0 in the
sec_inc variable. Therefore, the subsequent div_u64 operation can Oops
the kernel because of the divisor being 0.

Signed-off-by: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 23ec0a9e396c..6ed1704b638d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -848,7 +848,7 @@ int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 systime_flags)
 	stmmac_config_sub_second_increment(priv, priv->ptpaddr,
 					   priv->plat->clk_ptp_rate,
 					   xmac, &sec_inc);
-	temp = div_u64(1000000000ULL, sec_inc);
+	temp = div_u64(1000000000ULL, max_t(u32, sec_inc, 1));
 
 	/* Store sub second increment for later use */
 	priv->sub_second_inc = sec_inc;
-- 
2.35.1

Re: [PATCH net] stmmac: fix potential division by 0

[Andrew Lunn]

On Thu, Dec 01, 2022 at 01:37:08AM +0100, Piergiorgio Beruto wrote:
> Depending on the HW platform and configuration, the
> stmmac_config_sub_second_increment() function may return 0 in the
> sec_inc variable. Therefore, the subsequent div_u64 operation can Oops
> the kernel because of the divisor being 0.

I'm wondering why it would return 0? Is the configuration actually
invalid? Is ptp_clock is too small, such that the value of data is
bigger than 255, but when masked with 0xff it gives zero?

I'm wondering if the correct thing to do is return -EINVAL in
stmmac_init_tstamp_counter().

So i would like an explanation of why it is zero.

Thanks
	Andrew

Re: [PATCH net] stmmac: fix potential division by 0

[Piergiorgio Beruto]

On Thu, Dec 01, 2022 at 02:39:03AM +0100, Andrew Lunn wrote:
> On Thu, Dec 01, 2022 at 01:37:08AM +0100, Piergiorgio Beruto wrote:
> > Depending on the HW platform and configuration, the
> > stmmac_config_sub_second_increment() function may return 0 in the
> > sec_inc variable. Therefore, the subsequent div_u64 operation can Oops
> > the kernel because of the divisor being 0.
> 
> I'm wondering why it would return 0? Is the configuration actually
> invalid? Is ptp_clock is too small, such that the value of data is
> bigger than 255, but when masked with 0xff it gives zero?
Ok, I did some more analysis on this. On my reference board, I got two
PHYs connected to two stmmac, one is 1000BASE-T, the other one is
10BASE-T1S.

Fot the 1000BASE-T PHY everything works ok. The ptp_clock is 0ee6b280
which gives data = 8 that is less than FF.

For the 10BASE-T1 PHY the ptp_clock is 001dcd65 which gives data = 400
(too large). Therefore, it is 0 after masking.

The root cause is the MAC using the internal clock as a PTP reference
(default), which should be allowed since the connection to an external
PTP clock is optional from an HW perspective. The internal clock seems
to be derived from the MII clock speed, which is 2.5 MHz at 10 Mb/s.

> 
> I'm wondering if the correct thing to do is return -EINVAL in
> stmmac_init_tstamp_counter().
I've tried that as an alternate fix. The end result is:

/root # ifconfig eth0 up
[   17.535304] socfpga-dwmac ff700000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
[   17.549104] socfpga-dwmac ff700000.ethernet eth0: PHY [stmmac-0:08] driver [NCN26000] (irq=49)
[   17.568801] dwmac1000: Master AXI performs any burst length
[   17.574410] socfpga-dwmac ff700000.ethernet eth0: No Safety Features support found

[   17.595874] socfpga-dwmac ff700000.ethernet eth0: PTP init failed

[   17.605308] socfpga-dwmac ff700000.ethernet eth0: configuring for phy/mii link mode
[   17.613905] socfpga-dwmac ff700000.ethernet eth0: No phy led trigger registered for speed(10)
[   17.624558] socfpga-dwmac ff700000.ethernet eth0: Link is Up - 10Mbps/Half - flow control off

So as you can see the PTP initialization failed, but it soes not seem to
provoke any other unwanted effect.

The real question, in my opinion, is: are we ok just making it fail?
This is certainly good enough for my application, but others may have a
different opinion.

I would suggest to return an error for the time being (as it fixes the
Oops) then see whether a different fix is really needed.

Please, let me know your thoughts.

Kidn Regards,
Piergiorgio

Re: [PATCH net] stmmac: fix potential division by 0

[Andrew Lunn]

On Thu, Dec 01, 2022 at 11:24:42AM +0100, Piergiorgio Beruto wrote:
> On Thu, Dec 01, 2022 at 02:39:03AM +0100, Andrew Lunn wrote:
> > On Thu, Dec 01, 2022 at 01:37:08AM +0100, Piergiorgio Beruto wrote:
> > > Depending on the HW platform and configuration, the
> > > stmmac_config_sub_second_increment() function may return 0 in the
> > > sec_inc variable. Therefore, the subsequent div_u64 operation can Oops
> > > the kernel because of the divisor being 0.
> > 
> > I'm wondering why it would return 0? Is the configuration actually
> > invalid? Is ptp_clock is too small, such that the value of data is
> > bigger than 255, but when masked with 0xff it gives zero?
> Ok, I did some more analysis on this. On my reference board, I got two
> PHYs connected to two stmmac, one is 1000BASE-T, the other one is
> 10BASE-T1S.
> 
> Fot the 1000BASE-T PHY everything works ok. The ptp_clock is 0ee6b280
> which gives data = 8 that is less than FF.
> 
> For the 10BASE-T1 PHY the ptp_clock is 001dcd65 which gives data = 400
> (too large). Therefore, it is 0 after masking.

So both too large, and also unlucky. If it had been 0x3ff you would
not of noticed.

> The root cause is the MAC using the internal clock as a PTP reference
> (default), which should be allowed since the connection to an external
> PTP clock is optional from an HW perspective. The internal clock seems
> to be derived from the MII clock speed, which is 2.5 MHz at 10 Mb/s.

I think we need help from somebody who understands PTP on this device.
The clock is clearly out of range, but how important is that to PTP?
Will PTP work if the value is clamped to 0xff? Or should we be
returning -EINVAL and disabling PTP because it has no chance of
working?

Add to Cc: a few people who have worked on the PTP code. Lets see what
they have to say.

     Andrew

Re: [PATCH net] stmmac: fix potential division by 0

[Piergiorgio Beruto]

On Thu, Dec 01, 2022 at 03:49:37PM +0100, Andrew Lunn wrote:
> On Thu, Dec 01, 2022 at 11:24:42AM +0100, Piergiorgio Beruto wrote:
> > On Thu, Dec 01, 2022 at 02:39:03AM +0100, Andrew Lunn wrote:
> > > On Thu, Dec 01, 2022 at 01:37:08AM +0100, Piergiorgio Beruto wrote:
> > > > Depending on the HW platform and configuration, the
> > > > stmmac_config_sub_second_increment() function may return 0 in the
> > > > sec_inc variable. Therefore, the subsequent div_u64 operation can Oops
> > > > the kernel because of the divisor being 0.
> > > 
> > > I'm wondering why it would return 0? Is the configuration actually
> > > invalid? Is ptp_clock is too small, such that the value of data is
> > > bigger than 255, but when masked with 0xff it gives zero?
> > Ok, I did some more analysis on this. On my reference board, I got two
> > PHYs connected to two stmmac, one is 1000BASE-T, the other one is
> > 10BASE-T1S.
> > 
> > Fot the 1000BASE-T PHY everything works ok. The ptp_clock is 0ee6b280
> > which gives data = 8 that is less than FF.
> > 
> > For the 10BASE-T1 PHY the ptp_clock is 001dcd65 which gives data = 400
> > (too large). Therefore, it is 0 after masking.
> 
> So both too large, and also unlucky. If it had been 0x3ff you would
> not of noticed.
> 
> > The root cause is the MAC using the internal clock as a PTP reference
> > (default), which should be allowed since the connection to an external
> > PTP clock is optional from an HW perspective. The internal clock seems
> > to be derived from the MII clock speed, which is 2.5 MHz at 10 Mb/s.
> 
> I think we need help from somebody who understands PTP on this device.
> The clock is clearly out of range, but how important is that to PTP?
> Will PTP work if the value is clamped to 0xff? Or should we be
> returning -EINVAL and disabling PTP because it has no chance of
> working?
> 
> Add to Cc: a few people who have worked on the PTP code. Lets see what
> they have to say.
> 
>      Andrew

For reference, this is the log of the error.

/root # ifconfig eth0 up
[   95.062067] socfpga-dwmac ff700000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
[   95.076440] socfpga-dwmac ff700000.ethernet eth0: PHY [stmmac-0:08] driver [NCN26000] (irq=49)
[   95.095964] dwmac1000: Master AXI performs any burst length
[   95.101588] socfpga-dwmac ff700000.ethernet eth0: No Safety Features support found
[   95.109428] Division by zero in kernel.
[   95.113447] CPU: 0 PID: 239 Comm: ifconfig Not tainted 6.1.0-rc7-centurion3-1.0.3.0-01574-gb624218205b7-dirty #77
[   95.123686] Hardware name: Altera SOCFPGA
[   95.127695]  unwind_backtrace from show_stack+0x10/0x14
[   95.132938]  show_stack from dump_stack_lvl+0x40/0x4c
[   95.137992]  dump_stack_lvl from Ldiv0+0x8/0x10
[   95.142527]  Ldiv0 from __aeabi_uidivmod+0x8/0x18
[   95.147232]  __aeabi_uidivmod from div_u64_rem+0x1c/0x40
[   95.152552]  div_u64_rem from stmmac_init_tstamp_counter+0xd0/0x164
[   95.158826]  stmmac_init_tstamp_counter from stmmac_hw_setup+0x430/0xf00
[   95.165533]  stmmac_hw_setup from __stmmac_open+0x214/0x2d4
[   95.171117]  __stmmac_open from stmmac_open+0x30/0x44
[   95.176182]  stmmac_open from __dev_open+0x11c/0x134
[   95.181172]  __dev_open from __dev_change_flags+0x168/0x17c
[   95.186750]  __dev_change_flags from dev_change_flags+0x14/0x50
[   95.192662]  dev_change_flags from devinet_ioctl+0x2b4/0x604
[   95.198321]  devinet_ioctl from inet_ioctl+0x1ec/0x214
[   95.203462]  inet_ioctl from sock_ioctl+0x14c/0x3c4
[   95.208354]  sock_ioctl from vfs_ioctl+0x20/0x38
[   95.212984]  vfs_ioctl from sys_ioctl+0x250/0x844
[   95.217691]  sys_ioctl from ret_fast_syscall+0x0/0x4c
[   95.222743] Exception stack(0xd0ee1fa8 to 0xd0ee1ff0)
[   95.227790] 1fa0:                   00574c4f be9aeca4 00000003 00008914 be9aeca4 be9aec50
[   95.235945] 1fc0: 00574c4f be9aeca4 0059f078 00000036 be9aee8c be9aef7a 00000015 00000000
[   95.244096] 1fe0: 005a01f0 be9aec38 004d7484 b6e67d74

   Piergiorgio

Re: [PATCH net] stmmac: fix potential division by 0

[Jakub Kicinski]

On Thu, 1 Dec 2022 15:49:37 +0100 Andrew Lunn wrote:
> > The root cause is the MAC using the internal clock as a PTP reference
> > (default), which should be allowed since the connection to an external
> > PTP clock is optional from an HW perspective. The internal clock seems
> > to be derived from the MII clock speed, which is 2.5 MHz at 10 Mb/s.  
> 
> I think we need help from somebody who understands PTP on this device.
> The clock is clearly out of range, but how important is that to PTP?
> Will PTP work if the value is clamped to 0xff? Or should we be
> returning -EINVAL and disabling PTP because it has no chance of
> working?

Indeed, we need some more info here :( Like does the PTP actually
work with 2.5 MHz clock? The frequency adjustment only cares about 
the addend, what is sub_second_inc thing?

Re: [PATCH net] stmmac: fix potential division by 0

[Andrew Lunn]

On Tue, Dec 06, 2022 at 06:28:23PM -0800, Jakub Kicinski wrote:
> On Thu, 1 Dec 2022 15:49:37 +0100 Andrew Lunn wrote:
> > > The root cause is the MAC using the internal clock as a PTP reference
> > > (default), which should be allowed since the connection to an external
> > > PTP clock is optional from an HW perspective. The internal clock seems
> > > to be derived from the MII clock speed, which is 2.5 MHz at 10 Mb/s.  
> > 
> > I think we need help from somebody who understands PTP on this device.
> > The clock is clearly out of range, but how important is that to PTP?
> > Will PTP work if the value is clamped to 0xff? Or should we be
> > returning -EINVAL and disabling PTP because it has no chance of
> > working?
> 
> Indeed, we need some more info here :( Like does the PTP actually
> work with 2.5 MHz clock? The frequency adjustment only cares about 
> the addend, what is sub_second_inc thing?

Hi Jakub

I Cc: many of the people who worked on PTP with this hardware, and
nobody has replied.

I think we should wait a couple more days, and then add a range check,
and disable PTP for invalid clocks. That might provoke feedback.

    Andrew

Re: [PATCH net] stmmac: fix potential division by 0

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 1638 bytes --]

On Wed Dec 07 2022, Andrew Lunn wrote:
> On Tue, Dec 06, 2022 at 06:28:23PM -0800, Jakub Kicinski wrote:
>> On Thu, 1 Dec 2022 15:49:37 +0100 Andrew Lunn wrote:
>> > > The root cause is the MAC using the internal clock as a PTP reference
>> > > (default), which should be allowed since the connection to an external
>> > > PTP clock is optional from an HW perspective. The internal clock seems
>> > > to be derived from the MII clock speed, which is 2.5 MHz at 10 Mb/s.  
>> > 
>> > I think we need help from somebody who understands PTP on this device.
>> > The clock is clearly out of range, but how important is that to PTP?
>> > Will PTP work if the value is clamped to 0xff? Or should we be
>> > returning -EINVAL and disabling PTP because it has no chance of
>> > working?
>> 
>> Indeed, we need some more info here :( Like does the PTP actually
>> work with 2.5 MHz clock? The frequency adjustment only cares about 
>> the addend, what is sub_second_inc thing?
>
> Hi Jakub
>
> I Cc: many of the people who worked on PTP with this hardware, and
> nobody has replied.
>
> I think we should wait a couple more days, and then add a range check,
> and disable PTP for invalid clocks. That might provoke feedback.

Here's the Altera manual:

 https://www.intel.com/content/www/us/en/docs/programmable/683126/21-2/functional-description-of-the-emac.html

Table 183 shows the minimum PTP frequencies and also states "Therefore,
a higher PTP clock frequency gives better system performance.".

So, I'd say using a clock of 2.5MHz seems possible, but will result in
suboptimal precision.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH net] stmmac: fix potential division by 0

[Andrew Lunn]

On Wed, Dec 07, 2022 at 03:50:13PM +0100, Kurt Kanzenbach wrote:
> On Wed Dec 07 2022, Andrew Lunn wrote:
> > On Tue, Dec 06, 2022 at 06:28:23PM -0800, Jakub Kicinski wrote:
> >> On Thu, 1 Dec 2022 15:49:37 +0100 Andrew Lunn wrote:
> >> > > The root cause is the MAC using the internal clock as a PTP reference
> >> > > (default), which should be allowed since the connection to an external
> >> > > PTP clock is optional from an HW perspective. The internal clock seems
> >> > > to be derived from the MII clock speed, which is 2.5 MHz at 10 Mb/s.  
> >> > 
> >> > I think we need help from somebody who understands PTP on this device.
> >> > The clock is clearly out of range, but how important is that to PTP?
> >> > Will PTP work if the value is clamped to 0xff? Or should we be
> >> > returning -EINVAL and disabling PTP because it has no chance of
> >> > working?
> >> 
> >> Indeed, we need some more info here :( Like does the PTP actually
> >> work with 2.5 MHz clock? The frequency adjustment only cares about 
> >> the addend, what is sub_second_inc thing?
> >
> > Hi Jakub
> >
> > I Cc: many of the people who worked on PTP with this hardware, and
> > nobody has replied.
> >
> > I think we should wait a couple more days, and then add a range check,
> > and disable PTP for invalid clocks. That might provoke feedback.
> 
> Here's the Altera manual:
> 
>  https://www.intel.com/content/www/us/en/docs/programmable/683126/21-2/functional-description-of-the-emac.html
> 
> Table 183 shows the minimum PTP frequencies and also states "Therefore,
> a higher PTP clock frequency gives better system performance.".
> 
> So, I'd say using a clock of 2.5MHz seems possible, but will result in
> suboptimal precision.

Thanks for the info. So i seems like the correct fix is to camp to
0xff, rather than mask with 0xff.

      Andrew

Re: [PATCH net] stmmac: fix potential division by 0

[Piergiorgio Beruto]

> > 
> > Here's the Altera manual:
> > 
> >  https://www.intel.com/content/www/us/en/docs/programmable/683126/21-2/functional-description-of-the-emac.html
> > 
> > Table 183 shows the minimum PTP frequencies and also states "Therefore,
> > a higher PTP clock frequency gives better system performance.".
> > 
> > So, I'd say using a clock of 2.5MHz seems possible, but will result in
> > suboptimal precision.
> 
> Thanks for the info. So i seems like the correct fix is to camp to
> 0xff, rather than mask with 0xff.
Andrew, given your comment, do you wish me to re-post the patch with
this fix? Or wait for more feedback first?

Thanks,
Piergiorgio

Re: [PATCH net] stmmac: fix potential division by 0

[Andrew Lunn]

On Thu, Dec 08, 2022 at 10:27:56AM +0100, Piergiorgio Beruto wrote:
> > > 
> > > Here's the Altera manual:
> > > 
> > >  https://www.intel.com/content/www/us/en/docs/programmable/683126/21-2/functional-description-of-the-emac.html
> > > 
> > > Table 183 shows the minimum PTP frequencies and also states "Therefore,
> > > a higher PTP clock frequency gives better system performance.".
> > > 
> > > So, I'd say using a clock of 2.5MHz seems possible, but will result in
> > > suboptimal precision.
> > 
> > Thanks for the info. So i seems like the correct fix is to camp to
> > 0xff, rather than mask with 0xff.

> Andrew, given your comment, do you wish me to re-post the patch with
> this fix? Or wait for more feedback first?

Please post a patch. Often the only way to get feedback is to break systems :-(

In this case, clamping actually seems like it could fix systems, not
break them.

      Andrew