e1000e: can TIMINCA register be zero?

e1000e: can TIMINCA register be zero?

[Denys Vlasenko]

Hello,

I have a user report of division by zero in e1000e_cyclecounter_read+0xd9/0x100
at modprobe:

 [<ffffffff810b3c24>] timecounter_init+0x24/0x40
 [<ffffffffa048db34>] e1000e_config_hwtstamp+0x1c4/0x2e0 [e1000e]
 [<ffffffffa048ee55>] e1000e_reset+0x1c5/0x7a0 [e1000e]
 [<ffffffffa0496228>] e1000_probe+0xa2f/0xc7e [e1000e]
 [<ffffffff812befc7>] local_pci_probe+0x17/0x20
 [<ffffffff812c01b1>] pci_device_probe+0x101/0x120
 [<ffffffff81380c22>] ? driver_sysfs_add+0x62/0x90
 [<ffffffff81380eca>] driver_probe_device+0xaa/0x3a0
 [<ffffffff8138126b>] __driver_attach+0xab/0xb0
 [<ffffffff813811c0>] ? __driver_attach+0x0/0xb0
 [<ffffffff813800b4>] bus_for_each_dev+0x64/0x90
 [<ffffffff81380b5e>] driver_attach+0x1e/0x20
 [<ffffffff8137f8c8>] bus_add_driver+0x1e8/0x2b0
 [<ffffffff8138147f>] driver_register+0x5f/0xe0
 [<ffffffff812c0416>] __pci_register_driver+0x56/0xd0
 [<ffffffffa04ad000>] ? e1000_init_module+0x0/0x43 [e1000e]
 [<ffffffffa04ad041>] e1000_init_module+0x41/0x43 [e1000e]
 [<ffffffff810020d0>] do_one_initcall+0xc0/0x280
 [<ffffffff810c85d1>] sys_init_module+0xe1/0x250
 [<ffffffff8100b0d2>] system_call_fastpath+0x16/0x1b

User says it happens on hotplug.

On code inspection, this is clearly a case of
er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK == 0:

                /* errata for 82574/82583 possible bad bits read from SYSTIMH/L
                 * check to see that the time is incrementing at a reasonable
                 * rate and is a multiple of incvalue
                 */
==>             incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
                for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
                        /* latch SYSTIMH on read of SYSTIML */
                        systim_next = (cycle_t)er32(SYSTIML);
                        systim_next |= (cycle_t)er32(SYSTIMH) << 32;

                        time_delta = systim_next - systim;
                        temp = time_delta;
====>                   rem = do_div(temp, incvalue);

                        systim = systim_next;

                        if ((time_delta < E1000_82574_SYSTIM_EPSILON) &&
                            (rem == 0))
                                break;
                }

Knowing nothing about e1000e, I can easily slap on a quick fix here:

		rem = incvalue ? do_div(temp, incvalue) : (time_delta != 0);


However, I would like to alert you guys that this was seen.


Would zero counter increment in er32(TIMINCA) cause problems elsewhere?
In 1000e_config_hwtstamp(), it is initialized before timecounter_init():

        /* Get and set the System Time Register SYSTIM base frequency */
        ret_val = e1000e_get_base_timinca(adapter, &regval);
        if (ret_val)
                return ret_val;
==>     ew32(TIMINCA, regval);

        /* reset the ns time counter */
==>     timecounter_init(&adapter->tc, &adapter->cc,
                         ktime_to_ns(ktime_get_real()));

By code inspection, e1000e_get_base_timinca() either returns -EINVAL
and we don't do timecounter_init() and the division/0 location
is not reached, or e1000e_get_base_timinca(&regval) sets
nonzero regval. Then we set TIMINCA to this nonzero value.

Isn't it fishy that then timecounter_init() -> e1000e_cyclecounter_read()
-> er32(TIMINCA) sees zero there?

Re: e1000e: can TIMINCA register be zero?

[Richard Cochran]

On Tue, Apr 19, 2016 at 12:44:08PM +0200, Denys Vlasenko wrote:
> User says it happens on hotplug.

This sounds familiar.

   http://lists.openwall.net/netdev/2016/02/07/90

It also only ever happends in a VM, right?

> In 1000e_config_hwtstamp(), it is initialized before timecounter_init():

...

> By code inspection, e1000e_get_base_timinca() either returns -EINVAL
> and we don't do timecounter_init() and the division/0 location
> is not reached, or e1000e_get_base_timinca(&regval) sets
> nonzero regval. Then we set TIMINCA to this nonzero value.

Right, the register is always set to a non-zero value.
 
> Isn't it fishy that then timecounter_init() -> e1000e_cyclecounter_read()
> -> er32(TIMINCA) sees zero there?

Sure is.

Thanks,
Richard

Re: e1000e: can TIMINCA register be zero?

[Denys Vlasenko]

On 04/19/2016 03:07 PM, Richard Cochran wrote:
> On Tue, Apr 19, 2016 at 12:44:08PM +0200, Denys Vlasenko wrote:
>> User says it happens on hotplug.
> 
> This sounds familiar.
> 
>    http://lists.openwall.net/netdev/2016/02/07/90
> 
> It also only ever happends in a VM, right?

Yes, my user's case is also seen under VMware.