[BUG] atl1 causes lockup/reboot on resume from STR

[BUG] atl1 causes lockup/reboot on resume from STR

[Luca Tettamanti]

Hello,
I'm having troubles with atl1 driver (notebook is an Asus F3Sa): during
the resume from S3 the machine either locks up or reboots.
However I was also able to get a (partial) stack trace involving
atl1_resume (the rest of the trace scrolled off screen) and a message
about a recursive fault, followed by a hard lock up.
If I rmmod atl1 before suspending, the machine comes back fine; I don't
known whether this is a regression or not, until recently the radeon
driver didn't survive S3 so I didn't even try suspending to RAM...
I'm currently running -git current (2.6.36-rc3-00396-gbe6200a), but I've
gone back till 2.6.32 without any succcess.
I've no idea on how to debug this, the machine does not have a serial
port (and netconsole obviously does not help)... help :)

Luca

Re: [BUG] atl1 causes lockup/reboot on resume from STR

[Jarek Poplawski]

Luca Tettamanti wrote, On 09/11/2010 03:12 PM:

> Hello,
> I'm having troubles with atl1 driver (notebook is an Asus F3Sa): during
> the resume from S3 the machine either locks up or reboots.
> However I was also able to get a (partial) stack trace involving
> atl1_resume (the rest of the trace scrolled off screen) and a message
> about a recursive fault, followed by a hard lock up.
> If I rmmod atl1 before suspending, the machine comes back fine; I don't
> known whether this is a regression or not, until recently the radeon
> driver didn't survive S3 so I didn't even try suspending to RAM...
> I'm currently running -git current (2.6.36-rc3-00396-gbe6200a), but I've
> gone back till 2.6.32 without any succcess.
> I've no idea on how to debug this, the machine does not have a serial
> port (and netconsole obviously does not help)... help :)

Here is an example of kernel photography with an interesting hint
by Andrew Morton: 
https://bugzilla.kernel.org/show_bug.cgi?id=16626


Jarek P.

Re: [BUG] atl1 causes lockup/reboot on resume from STR

[Luca Tettamanti]

On Sat, Sep 11, 2010 at 10:47 PM, Jarek Poplawski <jarkao2@gmail.com> wrote:
[...]
>> I've no idea on how to debug this, the machine does not have a serial
>> port (and netconsole obviously does not help)... help :)
>
> Here is an example of kernel photography with an interesting hint
> by Andrew Morton:
> https://bugzilla.kernel.org/show_bug.cgi?id=16626

Thanks for the tip, however the stack trace is extremely elusive: the
machine keeps resetting.
Anyway I've managed to track it down the old way (sticking printk
everywhere and commenting out random pieces of code :P)

Luca

Re: [BUG] atl1 causes lockup/reboot on resume from STR

[Luca Tettamanti]

Hello,
I've managed to track down the resume bug in the driver, I'm leaving
the full quote for reference:

On Sat, Sep 11, 2010 at 3:12 PM, Luca Tettamanti <kronos.it@gmail.com> wrote:
> Hello,
> I'm having troubles with atl1 driver (notebook is an Asus F3Sa): during
> the resume from S3 the machine either locks up or reboots.
> However I was also able to get a (partial) stack trace involving
> atl1_resume (the rest of the trace scrolled off screen) and a message
> about a recursive fault, followed by a hard lock up.
> If I rmmod atl1 before suspending, the machine comes back fine; I don't
> known whether this is a regression or not, until recently the radeon
> driver didn't survive S3 so I didn't even try suspending to RAM...
> I'm currently running -git current (2.6.36-rc3-00396-gbe6200a), but I've
> gone back till 2.6.32 without any succcess.
> I've no idea on how to debug this, the machine does not have a serial
> port (and netconsole obviously does not help)... help :)

The bug is in atl1_resume, when it clears the interrupt status:
adapter->cmb.cmb->int_stats = 0

adapter->cmb.cmb is initialized (atl1_setup_ring_resources) only when
the interface is brought up so if the interface is not in use (which
is very common for me, since I'm usually on wifi) on resume the kernel
will try to dereference a NULL pointer.
Worse still, the memory pointed by adapter->cmb.cmb is freed by
atl1_free_ring_resources when the interface is brought down, leaving a
stale pointer which is then used in the resume path.
Btw, the same applies to adapter->smb.smb, though it's not used on resume.

The bug should be solved by moving the assignment under "if
(netif_running(netdev))" (assuming netif_running does what I think it
does :P); it also probably a good idea to reset adapter->cmb.cmb and
adapter->smb.smb when the memory is freed anyway.
My notebook has survived a few suspend&resume cycles with and without
wired networking, so far so good :)
If you agree on the proposed fixes I'll send the patches.

Luca

Re: [BUG] atl1 causes lockup/reboot on resume from STR

[Luca Tettamanti]

Ok, I'll be AFK till next monday so here's the patches.
I think the first one is suitable for -stable.

Luca

[PATCH 1/2] atl1: fix resume

[Luca Tettamanti]

adapter->cmb.cmb is initialized when the device is opened and freed when
it's closed. Accessing it unconditionally during resume results either
in a crash (NULL pointer dereference, when the interface has not been
opened yet) or data corruption (when the interface has been used and
brought down adapter->cmb.cmb points to a deallocated memory area).

Cc: stable@kernel.org
---
 drivers/net/atlx/atl1.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/atlx/atl1.c b/drivers/net/atlx/atl1.c
index 63b9ba0..bbd6e30 100644
--- a/drivers/net/atlx/atl1.c
+++ b/drivers/net/atlx/atl1.c
@@ -2847,10 +2847,11 @@ static int atl1_resume(struct pci_dev *pdev)
 	pci_enable_wake(pdev, PCI_D3cold, 0);
 
 	atl1_reset_hw(&adapter->hw);
-	adapter->cmb.cmb->int_stats = 0;
 
-	if (netif_running(netdev))
+	if (netif_running(netdev)) {
+		adapter->cmb.cmb->int_stats = 0;
 		atl1_up(adapter);
+	}
 	netif_device_attach(netdev);
 
 	return 0;
-- 
1.7.1

Re: [PATCH 1/2] atl1: fix resume

[Chris Snook]

On Thu, Sep 16, 2010 at 3:29 PM, Luca Tettamanti <kronos.it@gmail.com> wrote:
> adapter->cmb.cmb is initialized when the device is opened and freed when
> it's closed. Accessing it unconditionally during resume results either
> in a crash (NULL pointer dereference, when the interface has not been
> opened yet) or data corruption (when the interface has been used and
> brought down adapter->cmb.cmb points to a deallocated memory area).
>
> Cc: stable@kernel.org
> ---
>  drivers/net/atlx/atl1.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/atlx/atl1.c b/drivers/net/atlx/atl1.c
> index 63b9ba0..bbd6e30 100644
> --- a/drivers/net/atlx/atl1.c
> +++ b/drivers/net/atlx/atl1.c
> @@ -2847,10 +2847,11 @@ static int atl1_resume(struct pci_dev *pdev)
>        pci_enable_wake(pdev, PCI_D3cold, 0);
>
>        atl1_reset_hw(&adapter->hw);
> -       adapter->cmb.cmb->int_stats = 0;
>
> -       if (netif_running(netdev))
> +       if (netif_running(netdev)) {
> +               adapter->cmb.cmb->int_stats = 0;
>                atl1_up(adapter);
> +       }
>        netif_device_attach(netdev);
>
>        return 0;
> --
> 1.7.1
>
>

Thanks.  I confirmed that the other atlx drivers (which I have been
reminded I need to finish merging into a single driver) do not have
this bug.

Acked-by: Chris Snook <chris.snook@gmail.com>

Re: [stable] [PATCH 1/2] atl1: fix resume

[Greg KH]

On Thu, Sep 16, 2010 at 09:29:46PM +0200, Luca Tettamanti wrote:
> adapter->cmb.cmb is initialized when the device is opened and freed when
> it's closed. Accessing it unconditionally during resume results either
> in a crash (NULL pointer dereference, when the interface has not been
> opened yet) or data corruption (when the interface has been used and
> brought down adapter->cmb.cmb points to a deallocated memory area).
> 
> Cc: stable@kernel.org
> ---

You do realize you need to sign-off on a patch you create, right?

thanks,

greg k-h

Re: [stable] [PATCH 1/2] atl1: fix resume

[David Miller]

From: Greg KH <greg@kroah.com>
Date: Thu, 16 Sep 2010 14:13:18 -0700

> On Thu, Sep 16, 2010 at 09:29:46PM +0200, Luca Tettamanti wrote:
>> adapter->cmb.cmb is initialized when the device is opened and freed when
>> it's closed. Accessing it unconditionally during resume results either
>> in a crash (NULL pointer dereference, when the interface has not been
>> opened yet) or data corruption (when the interface has been used and
>> brought down adapter->cmb.cmb points to a deallocated memory area).
>> 
>> Cc: stable@kernel.org
>> ---
> 
> You do realize you need to sign-off on a patch you create, right?

Right.

Luca please formally resubmit these patches with a proper signoff
and all of the accumulated ACKs so far.

[PATCH 2/2] atl1: zero out CMB and SBM in atl1_free_ring_resources

[Luca Tettamanti]

They are allocated in atl1_setup_ring_resources, zero out the pointers
in atl1_free_ring_resources (like the other resources).
---
 drivers/net/atlx/atl1.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/net/atlx/atl1.c b/drivers/net/atlx/atl1.c
index bbd6e30..c73be28 100644
--- a/drivers/net/atlx/atl1.c
+++ b/drivers/net/atlx/atl1.c
@@ -1251,6 +1251,12 @@ static void atl1_free_ring_resources(struct atl1_adapter *adapter)
 
 	rrd_ring->desc = NULL;
 	rrd_ring->dma = 0;
+
+	adapter->cmb.dma = 0;
+	adapter->cmb.cmb = NULL;
+
+	adapter->smb.dma = 0;
+	adapter->smb.smb = NULL;
 }
 
 static void atl1_setup_mac_ctrl(struct atl1_adapter *adapter)
-- 
1.7.1

Re: [PATCH 2/2] atl1: zero out CMB and SBM in atl1_free_ring_resources

[Chris Snook]

On Thu, Sep 16, 2010 at 3:29 PM, Luca Tettamanti <kronos.it@gmail.com> wrote:
> They are allocated in atl1_setup_ring_resources, zero out the pointers
> in atl1_free_ring_resources (like the other resources).
> ---
>  drivers/net/atlx/atl1.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/atlx/atl1.c b/drivers/net/atlx/atl1.c
> index bbd6e30..c73be28 100644
> --- a/drivers/net/atlx/atl1.c
> +++ b/drivers/net/atlx/atl1.c
> @@ -1251,6 +1251,12 @@ static void atl1_free_ring_resources(struct atl1_adapter *adapter)
>
>        rrd_ring->desc = NULL;
>        rrd_ring->dma = 0;
> +
> +       adapter->cmb.dma = 0;
> +       adapter->cmb.cmb = NULL;
> +
> +       adapter->smb.dma = 0;
> +       adapter->smb.smb = NULL;
>  }
>
>  static void atl1_setup_mac_ctrl(struct atl1_adapter *adapter)
> --
> 1.7.1
>
>

Acked-by: Chris Snook <chris.snook@gmail.com>

Re: [PATCH 1/2] atl1: fix resume

[Luca Tettamanti]

On Fri, Sep 17, 2010 at 11:56 AM, Luca Tettamanti <kronos.it@gmail.com> wrote:
> adapter->cmb.cmb is initialized when the device is opened and freed when
> it's closed. Accessing it unconditionally during resume results either
> in a crash (NULL pointer dereference, when the interface has not been
> opened yet) or data corruption (when the interface has been used and
> brought down adapter->cmb.cmb points to a deallocated memory area).
>
> Cc: stable@kernel.org
> Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
> Acked-by: Chris Snook <chris.snook@gmail.com>

Ping?

Luca

Re: [PATCH 1/2] atl1: fix resume

[David Miller]

From: Luca Tettamanti <kronos.it@gmail.com>
Date: Wed, 22 Sep 2010 21:34:25 +0200

> On Fri, Sep 17, 2010 at 11:56 AM, Luca Tettamanti <kronos.it@gmail.com> wrote:
>> adapter->cmb.cmb is initialized when the device is opened and freed when
>> it's closed. Accessing it unconditionally during resume results either
>> in a crash (NULL pointer dereference, when the interface has not been
>> opened yet) or data corruption (when the interface has been used and
>> brought down adapter->cmb.cmb points to a deallocated memory area).
>>
>> Cc: stable@kernel.org
>> Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
>> Acked-by: Chris Snook <chris.snook@gmail.com>
> 
> Ping?

For some reason these resubmissions make it to netdev nor patchwork,
could you please try sending these two patches again?

Thanks!

[PATCH 1/2] atl1: fix resume

[Luca Tettamanti]

adapter->cmb.cmb is initialized when the device is opened and freed when
it's closed. Accessing it unconditionally during resume results either
in a crash (NULL pointer dereference, when the interface has not been
opened yet) or data corruption (when the interface has been used and
brought down adapter->cmb.cmb points to a deallocated memory area).

Cc: stable@kernel.org
Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
Acked-by: Chris Snook <chris.snook@gmail.com>
---
This time with mutt :)

 drivers/net/atlx/atl1.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/atlx/atl1.c b/drivers/net/atlx/atl1.c
index 63b9ba0..bbd6e30 100644
--- a/drivers/net/atlx/atl1.c
+++ b/drivers/net/atlx/atl1.c
@@ -2847,10 +2847,11 @@ static int atl1_resume(struct pci_dev *pdev)
 	pci_enable_wake(pdev, PCI_D3cold, 0);
 
 	atl1_reset_hw(&adapter->hw);
-	adapter->cmb.cmb->int_stats = 0;
 
-	if (netif_running(netdev))
+	if (netif_running(netdev)) {
+		adapter->cmb.cmb->int_stats = 0;
 		atl1_up(adapter);
+	}
 	netif_device_attach(netdev);
 
 	return 0;
-- 
1.7.1

Re: [PATCH 1/2] atl1: fix resume

[David Miller]

From: Luca Tettamanti <kronos.it@gmail.com>
Date: Wed, 22 Sep 2010 22:41:58 +0200

> adapter->cmb.cmb is initialized when the device is opened and freed when
> it's closed. Accessing it unconditionally during resume results either
> in a crash (NULL pointer dereference, when the interface has not been
> opened yet) or data corruption (when the interface has been used and
> brought down adapter->cmb.cmb points to a deallocated memory area).
> 
> Cc: stable@kernel.org
> Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
> Acked-by: Chris Snook <chris.snook@gmail.com>

Applied.

[PATCH 2/2] atl1: zero out CMB and SBM in atl1_free_ring_resources

[Luca Tettamanti]

They are allocated in atl1_setup_ring_resources, zero out the pointers
in atl1_free_ring_resources (like the other resources).

Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
Acked-by: Chris Snook <chris.snook@gmail.com>
---
 drivers/net/atlx/atl1.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/net/atlx/atl1.c b/drivers/net/atlx/atl1.c
index bbd6e30..c73be28 100644
--- a/drivers/net/atlx/atl1.c
+++ b/drivers/net/atlx/atl1.c
@@ -1251,6 +1251,12 @@ static void atl1_free_ring_resources(struct atl1_adapter *adapter)
 
 	rrd_ring->desc = NULL;
 	rrd_ring->dma = 0;
+
+	adapter->cmb.dma = 0;
+	adapter->cmb.cmb = NULL;
+
+	adapter->smb.dma = 0;
+	adapter->smb.smb = NULL;
 }
 
 static void atl1_setup_mac_ctrl(struct atl1_adapter *adapter)
-- 
1.7.1

Re: [PATCH 2/2] atl1: zero out CMB and SBM in atl1_free_ring_resources

[David Miller]

From: Luca Tettamanti <kronos.it@gmail.com>
Date: Wed, 22 Sep 2010 22:42:31 +0200

> They are allocated in atl1_setup_ring_resources, zero out the pointers
> in atl1_free_ring_resources (like the other resources).
> 
> Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
> Acked-by: Chris Snook <chris.snook@gmail.com>

Applied.