Patchwork [net-next-2.6] e1000: don't enable dma receives until after dma address has been setup

login
register
mail settings
Submitter Dean Nelson
Date Sept. 15, 2011, 12:31 a.m.
Message ID <20110915003138.4279.19020.email-sent-by-dnelson@localhost6.localdomain6>
Download mbox | patch
Permalink /patch/114721/
State Awaiting Upstream
Delegated to: David Miller
Headers show

Comments

Dean Nelson - Sept. 15, 2011, 12:31 a.m.
Doing an 'ifconfig ethN down' followed by an 'ifconfig ethN up' on a qemu-kvm
guest system configured with two e1000 NICs can result in an 'unable to handle
kernel paging request at 0000000100000000' or 'bad page map in process ...' or
something similar.

These result from a 4096-byte page being corrupted with the following two-word
pattern (16-bytes) repeated throughout the entire page:

  0x0000000000000000
  0x0000000100000000

There can be other bits set as well. What is a constant is that the 2nd word
has the 32nd bit set. So one could see:

        :
  0x0000000000000000
  0x0000000100000000
  0x0000000000000000
  0x0000000172adc067    <<< bad pte
  0x800000006ec60067
  0x0000000700000040
  0x0000000000000000
  0x0000000100000000
        :

Which came from from a process' page table I dumped out when the marked line
was seen as bad by print_bad_pte().

The repeating pattern represents the e1000's two-word receive descriptor:

struct e1000_rx_desc {
        __le64 buffer_addr;   /* Address of the descriptor's data buffer */
        __le16 length;        /* Length of data DMAed into data buffer */
        __le16 csum;          /* Packet checksum */
        u8 status;            /* Descriptor status */
        u8 errors;            /* Descriptor Errors */
        __le16 special;
};

And the 32nd bit of the 2nd word maps to the 'u8 status' member, and
corresponds to E1000_RXD_STAT_DD which indicates the descriptor is done.

The corruption appears to result from the following...

 . An 'ifconfig ethN down' gets us into e1000_close(), which through a number
   of subfunctions results in:
     1. E1000_RCTL_EN being cleared in RCTL register.  [e1000_down()]
     2. dma_free_coherent() being called.  [e1000_free_rx_resources()]

 . An 'ifconfig ethN up' gets us into e1000_open(), which through a number of
   subfunctions results in:
     1. dma_alloc_coherent() being called.  [e1000_setup_rx_resources()]
     2. E1000_RCTL_EN being set in RCTL register.  [e1000_setup_rctl()]
     3. E1000_RCTL_EN being cleared in RCTL register.  [e1000_configure_rx()]
     4. RDLEN, RDBAH and RDBAL registers being set to reflect the dma page
        allocated in step 1.  [e1000_configure_rx()]
     5. E1000_RCTL_EN being set in RCTL register.  [e1000_configure_rx()]

During the 'ifconfig ethN up' there is a window opened, starting in step 2
where the receives are enabled up until they are disabled in step 3, in which
the address of the receive descriptor dma page known by the NIC is still the
previous one which was freed during the 'ifconfig ethN down'. If this memory
has been reallocated for some other use and the NIC feels so inclined, it will
write to that former dma page with predictably unpleasant results.

I realize that in the guest, we're dealing with an e1000 NIC that is software
emulated by qemu-kvm. The problem doesn't appear to occur on bare-metal. Andy
suspects that this is because in the emulator link-up is essentially instant
and traffic can start flowing immediately. Whereas on bare-metal, link-up
usually seems to take at least a few milliseconds. And this might be enough
to prevent traffic from flowing into the device inside the window where
E1000_RCTL_EN is set.

So perhaps a modification needs to be made to the qemu-kvm e1000 NIC emulator
to delay the link-up. But in defense of the emulator, it seems like a bad idea
to enable dma operations before the address of the memory to be involved has
been made known.

The following patch no longer enables receives in e1000_setup_rctl() but leaves
them however they were. It only enables receives in e1000_configure_rx(), and
only after the dma address has been made known to the hardware.

There are two places where e1000_setup_rctl() gets called. The one in
e1000_configure() is followed immediately by a call to e1000_configure_rx(), so
there's really no change functionally (except for the removal of the problem
window. The other is in __e1000_shutdown() and is not followed by a call to
e1000_configure_rx(), so there is a change functionally. But consider...

 . An 'ifconfig ethN down' (just as described above).

 . A 'suspend' of the system, which (I'm assuming) will find its way into
   e1000_suspend() which calls __e1000_shutdown() resulting in:
     1. E1000_RCTL_EN being set in RCTL register.  [e1000_setup_rctl()]

And again we've re-opened the problem window for some unknown amount of time.

Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Dean Nelson <dnelson@redhat.com>

---
The patch below is Andy's version of a patch I came up with to address this
problem. I liked his version better. Functionally there was no difference
between the two.

Running my version of the patch, the reproducer (see script below) ran for 5
days without issue before I stopped it. Without the patch, former dma pages
were corrupted in a very short timeframe and fairly frequently (relatively
speaking). Note that I'm also running with a debug patch that after step 5 has
completed (mentioned above under an 'ifconfig ethN up'...), the previous dma
page is scanned to see if it had been 'corrupted'. So I found a higher
percentage of occurrences then one would find if one waits for a kernel BUG.

The reproducer for this problem is:
cat > reproducer.sh <<EOF
#!/bin/bash
typeset -i i=0
echo eth1:down
ifconfig eth1 down
sleep 2
while :; do
  i=$i+1
  ifconfig eth0 down& ifconfig eth1 up&
  echo "$i | eth0:down eth1:up"
  wait
  sleep 2
  ifconfig eth0 up& ifconfig eth1 down&
  echo "$i | eth0:up eth1:down"
  wait
  sleep 2
done
EOF

The e1000e looks to have the same issue. I don't know about igb. But I'm not
aware of either having hardware emulation in qemu-kvm. So unless this issue is
reproducible on bare-metal... it's probably not a big deal for them.

 drivers/net/ethernet/intel/e1000/e1000_main.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jesse Brandeburg - Sept. 15, 2011, 5:21 p.m.
On Wed, 14 Sep 2011 17:31:38 -0700
Dean Nelson <dnelson@redhat.com> wrote:

> Doing an 'ifconfig ethN down' followed by an 'ifconfig ethN up' on a
> qemu-kvm guest system configured with two e1000 NICs can result in an
> 'unable to handle kernel paging request at 0000000100000000' or 'bad
> page map in process ...' or something similar.

<snip>

> The corruption appears to result from the following...
> 
>  . An 'ifconfig ethN down' gets us into e1000_close(), which through
> a number of subfunctions results in:
>      1. E1000_RCTL_EN being cleared in RCTL register.  [e1000_down()]
>      2. dma_free_coherent() being called.  [e1000_free_rx_resources()]
> 
>  . An 'ifconfig ethN up' gets us into e1000_open(), which through a
> number of subfunctions results in:
>      1. dma_alloc_coherent() being called.
> [e1000_setup_rx_resources()] 2. E1000_RCTL_EN being set in RCTL
> register.  [e1000_setup_rctl()] 3. E1000_RCTL_EN being cleared in
> RCTL register.  [e1000_configure_rx()] 4. RDLEN, RDBAH and RDBAL
> registers being set to reflect the dma page allocated in step 1.
> [e1000_configure_rx()] 5. E1000_RCTL_EN being set in RCTL register.
> [e1000_configure_rx()]
> 
> During the 'ifconfig ethN up' there is a window opened, starting in
> step 2 where the receives are enabled up until they are disabled in
> step 3, in which the address of the receive descriptor dma page known
> by the NIC is still the previous one which was freed during the
> 'ifconfig ethN down'. If this memory has been reallocated for some
> other use and the NIC feels so inclined, it will write to that former
> dma page with predictably unpleasant results.
> 
> I realize that in the guest, we're dealing with an e1000 NIC that is
> software emulated by qemu-kvm. The problem doesn't appear to occur on
> bare-metal. Andy suspects that this is because in the emulator
> link-up is essentially instant and traffic can start flowing
> immediately. Whereas on bare-metal, link-up usually seems to take at
> least a few milliseconds. And this might be enough to prevent traffic
> from flowing into the device inside the window where E1000_RCTL_EN is
> set.

nice analysis dean, yes, we shouldn't enable rx before we have the
hardware all ready.

You didn't mention however that the hardware is reset in e1000_down,
which will clear the RDBAL/RDBAH in real hardware.

> 
> So perhaps a modification needs to be made to the qemu-kvm e1000 NIC
> emulator to delay the link-up. But in defense of the emulator, it
> seems like a bad idea to enable dma operations before the address of
> the memory to be involved has been made known.

the hardware reset code in kvm should also reset to default many
registers (almost all of them in fact) which may also end up solving
the problem.

> 
> The following patch no longer enables receives in e1000_setup_rctl()
> but leaves them however they were. It only enables receives in
> e1000_configure_rx(), and only after the dma address has been made
> known to the hardware.

I still like your patch better as it is more correct.  We could also
correct the kvm virtual hardware driver.

> There are two places where e1000_setup_rctl() gets called. The one in
> e1000_configure() is followed immediately by a call to
> e1000_configure_rx(), so there's really no change functionally
> (except for the removal of the problem window. The other is in
> __e1000_shutdown() and is not followed by a call to
> e1000_configure_rx(), so there is a change functionally. But
> consider...
> 
>  . An 'ifconfig ethN down' (just as described above).
> 
>  . A 'suspend' of the system, which (I'm assuming) will find its way
> into e1000_suspend() which calls __e1000_shutdown() resulting in:
>      1. E1000_RCTL_EN being set in RCTL register.
> [e1000_setup_rctl()]
> 
> And again we've re-opened the problem window for some unknown amount
> of time.
> 
> Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Dean Nelson <dnelson@redhat.com>
> 
> ---
> The patch below is Andy's version of a patch I came up with to
> address this problem. I liked his version better. Functionally there
> was no difference between the two.
> 
> Running my version of the patch, the reproducer (see script below)
> ran for 5 days without issue before I stopped it. Without the patch,
> former dma pages were corrupted in a very short timeframe and fairly
> frequently (relatively speaking). Note that I'm also running with a
> debug patch that after step 5 has completed (mentioned above under an
> 'ifconfig ethN up'...), the previous dma page is scanned to see if it
> had been 'corrupted'. So I found a higher percentage of occurrences
> then one would find if one waits for a kernel BUG.
> 
> The reproducer for this problem is:
> cat > reproducer.sh <<EOF
> #!/bin/bash
> typeset -i i=0
> echo eth1:down
> ifconfig eth1 down
> sleep 2
> while :; do
>   i=$i+1
>   ifconfig eth0 down& ifconfig eth1 up&
>   echo "$i | eth0:down eth1:up"
>   wait
>   sleep 2
>   ifconfig eth0 up& ifconfig eth1 down&
>   echo "$i | eth0:up eth1:down"
>   wait
>   sleep 2
> done
> EOF
> 
> The e1000e looks to have the same issue. I don't know about igb. But
> I'm not aware of either having hardware emulation in qemu-kvm. So
> unless this issue is reproducible on bare-metal... it's probably not
> a big deal for them.
> 
>  drivers/net/ethernet/intel/e1000/e1000_main.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c
> b/drivers/net/ethernet/intel/e1000/e1000_main.c index
> 4a32c15..cd26a0a 100644 ---
> a/drivers/net/ethernet/intel/e1000/e1000_main.c +++
> b/drivers/net/ethernet/intel/e1000/e1000_main.c @@ -1814,8 +1814,8 @@
> static void e1000_setup_rctl(struct e1000_adapter *adapter) 
>  	rctl &= ~(3 << E1000_RCTL_MO_SHIFT);
>  
> -	rctl |= E1000_RCTL_EN | E1000_RCTL_BAM |
> -		E1000_RCTL_LBM_NO | E1000_RCTL_RDMTS_HALF |
> +	rctl |= E1000_RCTL_BAM | E1000_RCTL_LBM_NO |
> +		E1000_RCTL_RDMTS_HALF |
>  		(hw->mc_filter_type << E1000_RCTL_MO_SHIFT);
>  
>  	if (hw->tbi_compatibility_on == 1)
> @@ -1917,7 +1917,7 @@ static void e1000_configure_rx(struct
> e1000_adapter *adapter) }
>  
>  	/* Enable Receives */
> -	ew32(RCTL, rctl);
> +	ew32(RCTL, rctl | E1000_RCTL_EN);
>  }
>  
>  /**


generally i like the patch.  We should take it in and test it, and I
don't really see any problems with it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dean Nelson - Sept. 15, 2011, 6:22 p.m.
On 09/15/2011 12:21 PM, Jesse Brandeburg wrote:
> On Wed, 14 Sep 2011 17:31:38 -0700
> Dean Nelson<dnelson@redhat.com>  wrote:
>
>> Doing an 'ifconfig ethN down' followed by an 'ifconfig ethN up' on a
>> qemu-kvm guest system configured with two e1000 NICs can result in an
>> 'unable to handle kernel paging request at 0000000100000000' or 'bad
>> page map in process ...' or something similar.
>
> <snip>
>
>> The corruption appears to result from the following...
>>
<snip>
>>
>> I realize that in the guest, we're dealing with an e1000 NIC that is
>> software emulated by qemu-kvm. The problem doesn't appear to occur on
>> bare-metal. Andy suspects that this is because in the emulator
>> link-up is essentially instant and traffic can start flowing
>> immediately. Whereas on bare-metal, link-up usually seems to take at
>> least a few milliseconds. And this might be enough to prevent traffic
>> from flowing into the device inside the window where E1000_RCTL_EN is
>> set.
>
> nice analysis dean, yes, we shouldn't enable rx before we have the
> hardware all ready.

Thank you.


> You didn't mention however that the hardware is reset in e1000_down,
> which will clear the RDBAL/RDBAH in real hardware.

You are correct, I did fail to mention the reset. And the clearing of
RDBAL/RDHAH was definitely not happening in the qemu-kvm emulator.


>> So perhaps a modification needs to be made to the qemu-kvm e1000 NIC
>> emulator to delay the link-up. But in defense of the emulator, it
>> seems like a bad idea to enable dma operations before the address of
>> the memory to be involved has been made known.
>
> the hardware reset code in kvm should also reset to default many
> registers (almost all of them in fact) which may also end up solving
> the problem.

Agreed.


>> The following patch no longer enables receives in e1000_setup_rctl()
>> but leaves them however they were. It only enables receives in
>> e1000_configure_rx(), and only after the dma address has been made
>> known to the hardware.
>
> I still like your patch better as it is more correct.  We could also
> correct the kvm virtual hardware driver.

The hardware emulator should definitely be doing a proper hardware reset.


>> There are two places where e1000_setup_rctl() gets called. The one in
<snip>
>>
>> The e1000e looks to have the same issue. I don't know about igb. But
>> I'm not aware of either having hardware emulation in qemu-kvm. So
>> unless this issue is reproducible on bare-metal... it's probably not
>> a big deal for them.
>>
<snip>
>
> generally i like the patch.  We should take it in and test it, and I
> don't really see any problems with it.

Thanks.

As mentioned above, the e1000e has a similar algorithm, but the
FLAG2_NO_DISABLE_RX complicates it a bit. I have no idea what happens
if receives are enabled while setting RDBAL and RDBAH. Is there any
possibility that the hardware could try to make use of a half-baked
address?

Thanks much for your review of the patch.

Dean

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Kirsher - Sept. 16, 2011, 2:03 a.m.
On Thu, 2011-09-15 at 11:22 -0700, Dean Nelson wrote:
> On 09/15/2011 12:21 PM, Jesse Brandeburg wrote:
> > On Wed, 14 Sep 2011 17:31:38 -0700
> > Dean Nelson<dnelson@redhat.com>  wrote:
> >
> >> Doing an 'ifconfig ethN down' followed by an 'ifconfig ethN up' on a
> >> qemu-kvm guest system configured with two e1000 NICs can result in an
> >> 'unable to handle kernel paging request at 0000000100000000' or 'bad
> >> page map in process ...' or something similar.
> >
> > <snip>
> >
> >> The corruption appears to result from the following...
> >>
> <snip>
> >>
> >> I realize that in the guest, we're dealing with an e1000 NIC that is
> >> software emulated by qemu-kvm. The problem doesn't appear to occur on
> >> bare-metal. Andy suspects that this is because in the emulator
> >> link-up is essentially instant and traffic can start flowing
> >> immediately. Whereas on bare-metal, link-up usually seems to take at
> >> least a few milliseconds. And this might be enough to prevent traffic
> >> from flowing into the device inside the window where E1000_RCTL_EN is
> >> set.
> >
> > nice analysis dean, yes, we shouldn't enable rx before we have the
> > hardware all ready.
> 
> Thank you.
> 
> 
> > You didn't mention however that the hardware is reset in e1000_down,
> > which will clear the RDBAL/RDBAH in real hardware.
> 
> You are correct, I did fail to mention the reset. And the clearing of
> RDBAL/RDHAH was definitely not happening in the qemu-kvm emulator.
> 
> 
> >> So perhaps a modification needs to be made to the qemu-kvm e1000 NIC
> >> emulator to delay the link-up. But in defense of the emulator, it
> >> seems like a bad idea to enable dma operations before the address of
> >> the memory to be involved has been made known.
> >
> > the hardware reset code in kvm should also reset to default many
> > registers (almost all of them in fact) which may also end up solving
> > the problem.
> 
> Agreed.
> 
> 
> >> The following patch no longer enables receives in e1000_setup_rctl()
> >> but leaves them however they were. It only enables receives in
> >> e1000_configure_rx(), and only after the dma address has been made
> >> known to the hardware.
> >
> > I still like your patch better as it is more correct.  We could also
> > correct the kvm virtual hardware driver.
> 
> The hardware emulator should definitely be doing a proper hardware reset.
> 
> 
> >> There are two places where e1000_setup_rctl() gets called. The one in
> <snip>
> >>
> >> The e1000e looks to have the same issue. I don't know about igb. But
> >> I'm not aware of either having hardware emulation in qemu-kvm. So
> >> unless this issue is reproducible on bare-metal... it's probably not
> >> a big deal for them.
> >>
> <snip>
> >
> > generally i like the patch.  We should take it in and test it, and I
> > don't really see any problems with it.
> 
> Thanks.
> 
> As mentioned above, the e1000e has a similar algorithm, but the
> FLAG2_NO_DISABLE_RX complicates it a bit. I have no idea what happens
> if receives are enabled while setting RDBAL and RDBAH. Is there any
> possibility that the hardware could try to make use of a half-baked
> address?
> 
> Thanks much for your review of the patch.
> 
> Dean
> 

Thanks Dean!  I will add your patch to my queue.  I will work with Bruce
to review e1000e and get a patch put together.

Patch

diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 4a32c15..cd26a0a 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -1814,8 +1814,8 @@  static void e1000_setup_rctl(struct e1000_adapter *adapter)
 
 	rctl &= ~(3 << E1000_RCTL_MO_SHIFT);
 
-	rctl |= E1000_RCTL_EN | E1000_RCTL_BAM |
-		E1000_RCTL_LBM_NO | E1000_RCTL_RDMTS_HALF |
+	rctl |= E1000_RCTL_BAM | E1000_RCTL_LBM_NO |
+		E1000_RCTL_RDMTS_HALF |
 		(hw->mc_filter_type << E1000_RCTL_MO_SHIFT);
 
 	if (hw->tbi_compatibility_on == 1)
@@ -1917,7 +1917,7 @@  static void e1000_configure_rx(struct e1000_adapter *adapter)
 	}
 
 	/* Enable Receives */
-	ew32(RCTL, rctl);
+	ew32(RCTL, rctl | E1000_RCTL_EN);
 }
 
 /**