(no subject)

[Fengguang Wu]

Subject: possible double free in edac_mc_alloc()
Reply-To: 
User-Agent: Heirloom mailx 12.5 6/20/10

Hi,

coccinelle warns about:

+ drivers/edac/edac_mc.c:429:9-23: ERROR: reference preceded by free on line 429

and that line does look strange: the 'i' seems like a temporary value
used in previous loops, and it won't change at all in the current
loop. Which means the same mci->csrows[i] get freed once and again.
It might also do double free for the previous kfree(csr) line.

vim +429 drivers/edac/edac_mc.c

   416         if (mci->dimms) {
   417                 for (i = 0; i < tot_dimms; i++)
   418                         kfree(mci->dimms[i]);
   419                 kfree(mci->dimms);
   420         }
   421         if (mci->csrows) {
   422                 for (chn = 0; chn < tot_channels; chn++) {
   423                         csr = mci->csrows[chn];
   424                         if (csr) {
   425                                 for (chn = 0; chn < tot_channels; chn++)
   426						kfree(csr->channels[chn]);
   427					kfree(csr);
   428				}
 > 429				kfree(mci->csrows[i]);
   430			}
   431			kfree(mci->csrows);
   432		}

---
0-DAY kernel build testing backend         Open Source Technology Centre
Fengguang Wu <wfg@linux.intel.com>                     Intel Corporation

Re:

[Mauro Carvalho Chehab]

Hi Fengguang,

Em 09-08-2012 10:54, Fengguang Wu escreveu:
...
> Date: Thu, 9 Aug 2012 21:54:16 +0800
> From: Fengguang Wu <fengguang.wu@intel.com>
> To: Mauro Carvalho Chehab <mchehab@redhat.com>
> Cc: Dave Peterson <dsp@llnl.gov>, kernel-janitors@vger.kernel.org,
>         Doug Thompson <dougthompson@xmission.com>, linux-edac@vger.kernel.org,
>         linux-kernel@vger.kernel.org
> Message-ID: <20120809135416.GA13100@localhost>
> MIME-Version: 1.0
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> User-Agent: Mutt/1.5.21 (2010-09-15)
> X-RedHat-Spam-Score: -5.111  (BAYES_00,MISSING_SUBJECT,RCVD_IN_DNSWL_HI,T_RP_MATCHES_RCVD)
> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24
> X-Scanned-By: MIMEDefang 2.68 on 10.5.110.17
>
> Subject: possible double free in edac_mc_alloc()
> Reply-To:
> User-Agent: Heirloom mailx 12.5 6/20/10

There is an extra space between the email headers and the Subject...
due to that I almost deleted this message, considering it as spam.

> 
> Hi,
> 
> coccinelle warns about:
> 
> + drivers/edac/edac_mc.c:429:9-23: ERROR: reference preceded by free on line 429
> 
> and that line does look strange: the 'i' seems like a temporary value
> used in previous loops, and it won't change at all in the current
> loop. Which means the same mci->csrows[i] get freed once and again.
> It might also do double free for the previous kfree(csr) line.
> 
> vim +429 drivers/edac/edac_mc.c
> 
>     416         if (mci->dimms) {
>     417                 for (i = 0; i < tot_dimms; i++)
>     418                         kfree(mci->dimms[i]);
>     419                 kfree(mci->dimms);
>     420         }
>     421         if (mci->csrows) {
>     422                 for (chn = 0; chn < tot_channels; chn++) {
>     423                         csr = mci->csrows[chn];
>     424                         if (csr) {
>     425                                 for (chn = 0; chn < tot_channels; chn++)
>     426						kfree(csr->channels[chn]);
>     427					kfree(csr);
>     428				}
>   > 429				kfree(mci->csrows[i]);

It should likely be:
	kfree(mci->csrows[csr])
instead. This is likely due to one of the countless rebases I had to do on it,
in order to make everybody happy. I suspect that, in the past, this loop was also
using 'i' as the index variable.

Care to write us a patch fixing it? My HD crashed yesterday... I'm somewhat
busy today recovering from it, and doing some backup/restore stuff.

Thanks!
Mauro

>     430			}
>     431			kfree(mci->csrows);
>     432		}
> 
> ---
> 0-DAY kernel build testing backend         Open Source Technology Centre
> Fengguang Wu <wfg@linux.intel.com>                     Intel Corporation
> 

[PATCH] edac_mc: fix kfree calls in the error path

[Fengguang Wu]

We need to free up memory in this order: 

  free csrows[i]->channels[j]
  free csrows[i]->channels
  free csrows[i]
  free csrows

Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
 drivers/edac/edac_mc.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

--- linux.orig/drivers/edac/edac_mc.c	2012-08-10 17:16:14.444794060 +0800
+++ linux/drivers/edac/edac_mc.c	2012-08-10 17:16:19.048794169 +0800
@@ -419,14 +419,16 @@ error:
 		kfree(mci->dimms);
 	}
 	if (mci->csrows) {
-		for (chn = 0; chn < tot_channels; chn++) {
-			csr = mci->csrows[chn];
+		for (row = 0; row < tot_csrows; row++) {
+			csr = mci->csrows[row];
 			if (csr) {
-				for (chn = 0; chn < tot_channels; chn++)
-					kfree(csr->channels[chn]);
+				if (csr->channels) {
+					for (chn = 0; chn < tot_channels; chn++)
+						kfree(csr->channels[chn]);
+					kfree(csr->channels);
+				}
 				kfree(csr);
 			}
-			kfree(mci->csrows[i]);
 		}
 		kfree(mci->csrows);
 	}

[PATCH 0/3] Fix edac_mc crash in e7xxx_edac error path.

[Shaun Ruffell]

With kernel version 3.6-rc2 on a Dell Poweredge 2600 I experienced a NULL
pointer dereference that did not occur with on 3.5. I believe the error is
related to commit de3910eb79a "edac: change the mem allocation scheme to make
Documentation/kobject.txt happy" [1] and the fact that my system is going
through an error path in the e7xxx_edac driver.

[1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=de3910eb79ac8c0f29a11224661c0ebaaf813039

This is the OOPS:

 [  36.703479] BUG: unable to handle kernel NULL pointer dereference at   (null)
 [  36.703479] IP: [<c045e195>] __wake_up_common+0x1a/0x6a
 [  36.703479] *pde = 7f0c6067 
 [  36.703479] Oops: 0000 [#1] SMP 
 [  36.703479] Modules linked in: parport_pc parport floppy e7xxx_edac(+) ide_cd_mod edac_core intel_rng cdrom microcode(+) dm_snapshot dm_zero dm_mirror dm_region_hash d
 [  36.703479] Pid: 933, comm: modprobe Tainted: G        W    3.6.0-rc2-00111-gc1999ee #12 Dell Computer Corporation PowerEdge 2600             /0F0364
 [  36.703479] EIP: 0060:[<c045e195>] EFLAGS: 00010093 CPU: 3
 [  36.703479] EIP is at __wake_up_common+0x1a/0x6a
 [  36.703479] EAX: f47b0984 EBX: fffffff4 ECX: 00000000 EDX: 00000003
 [  36.703479] ESI: f47b0984 EDI: 00000282 EBP: f3dc7d38 ESP: f3dc7d1c
 [  36.703479]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
 [  36.703479] CR0: 8005003b CR2: 00000000 CR3: 347d4000 CR4: 000007d0
 [  36.703479] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
 [  36.703479] DR6: ffff0ff0 DR7: 00000400
 [  36.703479] Process modprobe (pid: 933, ti=f3dc6000 task=f3db9520 task.ti=f3dc6000)
 [  36.703479] Stack:
 [  36.703479]  00000000 00000000 00000003 c046701a f47b0980 f47b0984 00000282 f3dc7d54
 [  36.703479]  c046703f 00000000 00000000 f47b08b0 f47b08b0 00000000 f3dc7d74 c06961ce
 [  36.703479]  f3dc7d74 f3dc7d80 c05e2837 c094c4cc f47b08b0 f47b08b0 f3dc7d88 c068d56d
 [  36.703479] Call Trace:
 [  36.703479]  [<c046701a>] ? complete_all+0x1a/0x50
 [  36.703479]  [<c046703f>] complete_all+0x3f/0x50
 [  36.703479]  [<c06961ce>] device_pm_remove+0x23/0xa2
 [  36.703479]  [<c05e2837>] ? kobject_put+0x5b/0x5d
 [  36.703479]  [<c068d56d>] device_del+0x34/0x142
 [  36.703479]  [<f8547884>] edac_unregister_sysfs+0x3b/0x5c [edac_core]
 [  36.703479]  [<f8545041>] edac_mc_free+0x29/0x2f [edac_core]
 [  36.703479]  [<f860163f>] e7xxx_probe1+0x268/0x311 [e7xxx_edac]
 [  36.703479]  [<c0603d00>] ? __pci_enable_device_flags+0x8f/0xd3
 [  36.703479]  [<f8601b0b>] e7xxx_init_one+0x56/0x61 [e7xxx_edac]
 [  36.703479]  [<c0604f85>] local_pci_probe+0x13/0x15
 [  36.703479]  [<c0605115>] pci_call_probe+0x1c/0x1e
 [  36.703479]  [<c0605158>] __pci_device_probe+0x41/0x4e
 [  36.703479]  [<c060579c>] pci_device_probe+0x26/0x39
 [  36.703479]  [<c06901ed>] really_probe+0x101/0x2a1
 [  36.703479]  [<c0690680>] ? __driver_attach+0x3d/0x6e
 [  36.703479]  [<c0690680>] ? __driver_attach+0x3d/0x6e
 [  36.703479]  [<c07e0000>] ? quirk_usb_disable_ehci+0xa3/0x141
 [  36.703479]  [<c06903c2>] driver_probe_device+0x35/0x79
 [  36.703479]  [<c06906af>] __driver_attach+0x6c/0x6e
 [  36.703479]  [<c068eb25>] bus_for_each_dev+0x44/0x62
 [  36.703479]  [<c068ff00>] driver_attach+0x1e/0x20
 [  36.703479]  [<c0690643>] ? device_attach+0x98/0x98
 [  36.703479]  [<c068fa12>] bus_add_driver+0xc5/0x1c8
 [  36.703479]  [<c0605beb>] ? store_new_id+0xfa/0xfa
 [  36.703479]  [<c0690bfb>] driver_register+0x52/0xd6
 [  36.703479]  [<f8604000>] ? 0xf8603fff
 [  36.703479]  [<c0605a30>] __pci_register_driver+0x4b/0x73
 [  36.703479]  [<f8604000>] ? 0xf8603fff
 [  36.703479]  [<f8604055>] e7xxx_init+0x55/0x57 [e7xxx_edac]
 [  36.703479]  [<c040120e>] do_one_initcall+0xa3/0xe0
 [  36.703479]  [<c0491023>] sys_init_module+0x70/0x1af
 [  36.703479]  [<c04823a9>] ? trace_hardirqs_on_caller+0x56/0xf9
 [  36.703479]  [<c05ec7e8>] ? trace_hardirqs_on_thunk+0xc/0x10
 [  36.703479]  [<c07f678c>] sysenter_do_call+0x12/0x32
 [  36.703479] Code: 5d c3 55 89 e5 3e 8d 74 26 00 e8 8f ff ff ff 5d c3 55 89 e5 57 56 53 83 ec 10 3e 8d 74 26 00 89 55 ec 89 4d e8 8b 58 28 83 eb 0c <8b> 53 0c 83 c0 28
 [  36.703479] EIP: [<c045e195>] __wake_up_common+0x1a/0x6a SS:ESP 0068:f3dc7d1c
 [  36.703479] CR2: 0000000000000000
 [  36.703479] ---[ end trace 6fcfddc0eef7bbd8 ]---

When I enabled edac debugging I saw the following printed to the kernel log
prior to the above BUG:

  EDAC MC: Ver: 3.0.0
  EDAC DEBUG: edac_mc_sysfs_init: device mc created
  EDAC DEBUG: e7xxx_init_one:
  EDAC DEBUG: e7xxx_probe1: mci
  EDAC DEBUG: edac_mc_alloc: errcount layer 0 size 8
  EDAC DEBUG: edac_mc_alloc: errcount layer 1 size 16
  EDAC DEBUG: edac_mc_alloc: allocating 48 error counters
  EDAC DEBUG: edac_mc_alloc: allocating 1068 bytes for mci data (16 ranks, 16 csrows/channels)
  EDAC DEBUG: e7xxx_probe1: init mci
  EDAC DEBUG: e7xxx_probe1: init pvt
  EDAC e7xxx: error reporting device not found:vendor 8086 device 0x2541 (broken BIOS?)
  EDAC DEBUG: edac_mc_free:
  Floppy drive(s): fd0 is 1.44M
  EDAC DEBUG: edac_unregister_sysfs: Unregistering device (null)

There are probably better ways to accomplish what the following patches are
doing but I thought I would send along what I had if only to motivate any
discussion. I also have resent Fengguang Wu's patch in this series since I found
that it was required as well.

Shaun Ruffell (2):
  edac: Remove invalid kfree in error path of edac_mc_allocate().
  edac: edac_mc_free() cannot assume mem_ctl_info is registered in
    sysfs.

 drivers/edac/edac_mc.c | 60 +++++++++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 25 deletions(-)

-- 
1.7.11.2

[PATCH 1/3] edac_mc: fix kfree calls in the error path

[Shaun Ruffell]

From: Fengguang Wu <fengguang.wu@intel.com>

We need to free up memory in this order:

  free csrows[i]->channels[j]
  free csrows[i]->channels
  free csrows[i]
  free csrows

Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
 drivers/edac/edac_mc.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 616d90b..9037ffa 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -419,14 +419,16 @@ error:
 		kfree(mci->dimms);
 	}
 	if (mci->csrows) {
-		for (chn = 0; chn < tot_channels; chn++) {
-			csr = mci->csrows[chn];
+		for (row = 0; row < tot_csrows; row++) {
+			csr = mci->csrows[row];
 			if (csr) {
-				for (chn = 0; chn < tot_channels; chn++)
-					kfree(csr->channels[chn]);
+				if (csr->channels) {
+					for (chn = 0; chn < tot_channels; chn++)
+						kfree(csr->channels[chn]);
+					kfree(csr->channels);
+				}
 				kfree(csr);
 			}
-			kfree(mci->csrows[i]);
 		}
 		kfree(mci->csrows);
 	}
-- 
1.7.11.2

[PATCH 2/3] edac: edac_mc_free() cannot assume mem_ctl_info is registered in sysfs.

[Shaun Ruffell]

edac_mc_free() may need to deallocate any memory associated with struct
mem_ctl_info directly if the structure was never registered with sysfs in
edac_mc_add_mc(). This moves the error handling code from edac_mc_alloc() into a
dedicated function to be called by edac_mc_free() as well if necessary.

This resolves a NULL pointer dereference from the following code path first
introduced in 3.6-rc1:

  EDAC MC: Ver: 3.0.0
  EDAC DEBUG: edac_mc_sysfs_init: device mc created
  EDAC DEBUG: e7xxx_init_one:
  EDAC DEBUG: e7xxx_probe1: mci
  EDAC DEBUG: edac_mc_alloc: errcount layer 0 size 8
  EDAC DEBUG: edac_mc_alloc: errcount layer 1 size 16
  EDAC DEBUG: edac_mc_alloc: allocating 48 error counters
  EDAC DEBUG: edac_mc_alloc: allocating 1068 bytes for mci data (16 ranks, 16 csrows/channels)
  EDAC DEBUG: e7xxx_probe1: init mci
  EDAC DEBUG: e7xxx_probe1: init pvt
  EDAC e7xxx: error reporting device not found:vendor 8086 device 0x2541 (broken BIOS?)
  EDAC DEBUG: edac_mc_free:
  Floppy drive(s): fd0 is 1.44M
  EDAC DEBUG: edac_unregister_sysfs: Unregistering device (null)

Signed-off-by: Shaun Ruffell <sruffell@digium.com>
---
 drivers/edac/edac_mc.c | 59 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 20 deletions(-)

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 9037ffa..a58facc 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -199,6 +199,36 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
 	return (void *)(((unsigned long)ptr) + align - r);
 }
 
+static void _edac_mc_free(struct mem_ctl_info *mci)
+{
+	int i, chn, row;
+	struct csrow_info *csr;
+	const unsigned int tot_dimms = mci->tot_dimms;
+	const unsigned int tot_channels = mci->num_cschannel;
+	const unsigned int tot_csrows = mci->nr_csrows;
+
+	if (mci->dimms) {
+		for (i = 0; i < tot_dimms; i++)
+			kfree(mci->dimms[i]);
+		kfree(mci->dimms);
+	}
+	if (mci->csrows) {
+		for (row = 0; row < tot_csrows; row++) {
+			csr = mci->csrows[row];
+			if (csr) {
+				if (csr->channels) {
+					for (chn = 0; chn < tot_channels; chn++)
+						kfree(csr->channels[chn]);
+					kfree(csr->channels);
+				}
+				kfree(csr);
+			}
+		}
+		kfree(mci->csrows);
+	}
+	kfree(mci);
+}
+
 /**
  * edac_mc_alloc: Allocate and partially fill a struct mem_ctl_info structure
  * @mc_num:		Memory controller number
@@ -413,26 +443,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
 	return mci;
 
 error:
-	if (mci->dimms) {
-		for (i = 0; i < tot_dimms; i++)
-			kfree(mci->dimms[i]);
-		kfree(mci->dimms);
-	}
-	if (mci->csrows) {
-		for (row = 0; row < tot_csrows; row++) {
-			csr = mci->csrows[row];
-			if (csr) {
-				if (csr->channels) {
-					for (chn = 0; chn < tot_channels; chn++)
-						kfree(csr->channels[chn]);
-					kfree(csr->channels);
-				}
-				kfree(csr);
-			}
-		}
-		kfree(mci->csrows);
-	}
-	kfree(mci);
+	_edac_mc_free(mci);
 
 	return NULL;
 }
@@ -447,6 +458,14 @@ void edac_mc_free(struct mem_ctl_info *mci)
 {
 	edac_dbg(1, "\n");
 
+	/* If we're not yet registered with sysfs free only what was allocated
+	 * in edac_mc_alloc().
+	 */
+	if (!mci->bus.name) {
+		_edac_mc_free(mci);
+		return;
+	}
+
 	/* the mci instance is freed here, when the sysfs object is dropped */
 	edac_unregister_sysfs(mci);
 }
-- 
1.7.11.2

[PATCH 3/3] edac: edac_mc no longer deals with kobjects directly.

[Shaun Ruffell]

There are no more embedded kobjects in struct mem_ctl_info. Remove a header and
a comment that does not reflect the code anymore.

Signed-off-by: Shaun Ruffell <sruffell@digium.com>
---
 drivers/edac/edac_mc.c | 7 -------
 include/linux/edac.h   | 1 -
 2 files changed, 8 deletions(-)

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index a58facc..65c59b1 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -433,13 +433,6 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
 
 	mci->op_state = OP_ALLOC;
 
-	/* at this point, the root kobj is valid, and in order to
-	 * 'free' the object, then the function:
-	 *      edac_mc_unregister_sysfs_main_kobj() must be called
-	 * which will perform kobj unregistration and the actual free
-	 * will occur during the kobject callback operation
-	 */
-
 	return mci;
 
 error:
diff --git a/include/linux/edac.h b/include/linux/edac.h
index bab9f84..aeddb3f 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -14,7 +14,6 @@
 
 #include <linux/atomic.h>
 #include <linux/device.h>
-#include <linux/kobject.h>
 #include <linux/completion.h>
 #include <linux/workqueue.h>
 #include <linux/debugfs.h>
-- 
1.7.11.2

Re: [PATCH 0/3] Fix edac_mc crash in e7xxx_edac error path.

[Shaun Ruffell]

Just a friendly reminder that I'm still seeing this NULL pointer
dereference on boot with 3.6-rc4.

On Sat, Aug 18, 2012 at 11:11:21PM -0500, Shaun Ruffell wrote:
> With kernel version 3.6-rc2 on a Dell Poweredge 2600 I experienced a NULL
> pointer dereference that did not occur with on 3.5. I believe the error is
> related to commit de3910eb79a "edac: change the mem allocation scheme to make
> Documentation/kobject.txt happy" [1] and the fact that my system is going
> through an error path in the e7xxx_edac driver.
> 
> [1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=de3910eb79ac8c0f29a11224661c0ebaaf813039
> 
> This is the OOPS:
> 
>  [  36.703479] BUG: unable to handle kernel NULL pointer dereference at   (null)
>  [  36.703479] IP: [<c045e195>] __wake_up_common+0x1a/0x6a
>  [  36.703479] *pde = 7f0c6067 
>  [  36.703479] Oops: 0000 [#1] SMP 
>  [  36.703479] Modules linked in: parport_pc parport floppy e7xxx_edac(+) ide_cd_mod edac_core intel_rng cdrom microcode(+) dm_snapshot dm_zero dm_mirror dm_region_hash d
>  [  36.703479] Pid: 933, comm: modprobe Tainted: G        W    3.6.0-rc2-00111-gc1999ee #12 Dell Computer Corporation PowerEdge 2600             /0F0364
>  [  36.703479] EIP: 0060:[<c045e195>] EFLAGS: 00010093 CPU: 3
>  [  36.703479] EIP is at __wake_up_common+0x1a/0x6a
>  [  36.703479] EAX: f47b0984 EBX: fffffff4 ECX: 00000000 EDX: 00000003
>  [  36.703479] ESI: f47b0984 EDI: 00000282 EBP: f3dc7d38 ESP: f3dc7d1c
>  [  36.703479]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
>  [  36.703479] CR0: 8005003b CR2: 00000000 CR3: 347d4000 CR4: 000007d0
>  [  36.703479] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
>  [  36.703479] DR6: ffff0ff0 DR7: 00000400
>  [  36.703479] Process modprobe (pid: 933, ti=f3dc6000 task=f3db9520 task.ti=f3dc6000)
>  [  36.703479] Stack:
>  [  36.703479]  00000000 00000000 00000003 c046701a f47b0980 f47b0984 00000282 f3dc7d54
>  [  36.703479]  c046703f 00000000 00000000 f47b08b0 f47b08b0 00000000 f3dc7d74 c06961ce
>  [  36.703479]  f3dc7d74 f3dc7d80 c05e2837 c094c4cc f47b08b0 f47b08b0 f3dc7d88 c068d56d
>  [  36.703479] Call Trace:
>  [  36.703479]  [<c046701a>] ? complete_all+0x1a/0x50
>  [  36.703479]  [<c046703f>] complete_all+0x3f/0x50
>  [  36.703479]  [<c06961ce>] device_pm_remove+0x23/0xa2
>  [  36.703479]  [<c05e2837>] ? kobject_put+0x5b/0x5d
>  [  36.703479]  [<c068d56d>] device_del+0x34/0x142
>  [  36.703479]  [<f8547884>] edac_unregister_sysfs+0x3b/0x5c [edac_core]
>  [  36.703479]  [<f8545041>] edac_mc_free+0x29/0x2f [edac_core]
>  [  36.703479]  [<f860163f>] e7xxx_probe1+0x268/0x311 [e7xxx_edac]
>  [  36.703479]  [<c0603d00>] ? __pci_enable_device_flags+0x8f/0xd3
>  [  36.703479]  [<f8601b0b>] e7xxx_init_one+0x56/0x61 [e7xxx_edac]
>  [  36.703479]  [<c0604f85>] local_pci_probe+0x13/0x15
>  [  36.703479]  [<c0605115>] pci_call_probe+0x1c/0x1e
>  [  36.703479]  [<c0605158>] __pci_device_probe+0x41/0x4e
>  [  36.703479]  [<c060579c>] pci_device_probe+0x26/0x39
>  [  36.703479]  [<c06901ed>] really_probe+0x101/0x2a1
>  [  36.703479]  [<c0690680>] ? __driver_attach+0x3d/0x6e
>  [  36.703479]  [<c0690680>] ? __driver_attach+0x3d/0x6e
>  [  36.703479]  [<c07e0000>] ? quirk_usb_disable_ehci+0xa3/0x141
>  [  36.703479]  [<c06903c2>] driver_probe_device+0x35/0x79
>  [  36.703479]  [<c06906af>] __driver_attach+0x6c/0x6e
>  [  36.703479]  [<c068eb25>] bus_for_each_dev+0x44/0x62
>  [  36.703479]  [<c068ff00>] driver_attach+0x1e/0x20
>  [  36.703479]  [<c0690643>] ? device_attach+0x98/0x98
>  [  36.703479]  [<c068fa12>] bus_add_driver+0xc5/0x1c8
>  [  36.703479]  [<c0605beb>] ? store_new_id+0xfa/0xfa
>  [  36.703479]  [<c0690bfb>] driver_register+0x52/0xd6
>  [  36.703479]  [<f8604000>] ? 0xf8603fff
>  [  36.703479]  [<c0605a30>] __pci_register_driver+0x4b/0x73
>  [  36.703479]  [<f8604000>] ? 0xf8603fff
>  [  36.703479]  [<f8604055>] e7xxx_init+0x55/0x57 [e7xxx_edac]
>  [  36.703479]  [<c040120e>] do_one_initcall+0xa3/0xe0
>  [  36.703479]  [<c0491023>] sys_init_module+0x70/0x1af
>  [  36.703479]  [<c04823a9>] ? trace_hardirqs_on_caller+0x56/0xf9
>  [  36.703479]  [<c05ec7e8>] ? trace_hardirqs_on_thunk+0xc/0x10
>  [  36.703479]  [<c07f678c>] sysenter_do_call+0x12/0x32
>  [  36.703479] Code: 5d c3 55 89 e5 3e 8d 74 26 00 e8 8f ff ff ff 5d c3 55 89 e5 57 56 53 83 ec 10 3e 8d 74 26 00 89 55 ec 89 4d e8 8b 58 28 83 eb 0c <8b> 53 0c 83 c0 28
>  [  36.703479] EIP: [<c045e195>] __wake_up_common+0x1a/0x6a SS:ESP 0068:f3dc7d1c
>  [  36.703479] CR2: 0000000000000000
>  [  36.703479] ---[ end trace 6fcfddc0eef7bbd8 ]---
> 
> When I enabled edac debugging I saw the following printed to the kernel log
> prior to the above BUG:
> 
>   EDAC MC: Ver: 3.0.0
>   EDAC DEBUG: edac_mc_sysfs_init: device mc created
>   EDAC DEBUG: e7xxx_init_one:
>   EDAC DEBUG: e7xxx_probe1: mci
>   EDAC DEBUG: edac_mc_alloc: errcount layer 0 size 8
>   EDAC DEBUG: edac_mc_alloc: errcount layer 1 size 16
>   EDAC DEBUG: edac_mc_alloc: allocating 48 error counters
>   EDAC DEBUG: edac_mc_alloc: allocating 1068 bytes for mci data (16 ranks, 16 csrows/channels)
>   EDAC DEBUG: e7xxx_probe1: init mci
>   EDAC DEBUG: e7xxx_probe1: init pvt
>   EDAC e7xxx: error reporting device not found:vendor 8086 device 0x2541 (broken BIOS?)
>   EDAC DEBUG: edac_mc_free:
>   Floppy drive(s): fd0 is 1.44M
>   EDAC DEBUG: edac_unregister_sysfs: Unregistering device (null)
> 
> There are probably better ways to accomplish what the following patches are
> doing but I thought I would send along what I had if only to motivate any
> discussion. I also have resent Fengguang Wu's patch in this series since I found
> that it was required as well.
> 
> Shaun Ruffell (2):
>   edac: Remove invalid kfree in error path of edac_mc_allocate().
>   edac: edac_mc_free() cannot assume mem_ctl_info is registered in
>     sysfs.
> 
>  drivers/edac/edac_mc.c | 60 +++++++++++++++++++++++++++++---------------------
>  1 file changed, 35 insertions(+), 25 deletions(-)

[PATCH v2 0/3] Fix edac_mc crash in e7xxx_edac error path.

[Shaun Ruffell]

v2:
Use '!device_is_registered(&mci->dev)' instead of 'if (!mci->bus.name)' to
check if mem_ctl_info has been registered with sysfs.

v1:

With kernel version 3.6-rc2 on a Dell Poweredge 2600 I experienced a NULL
pointer dereference that did not occur with on 3.5. I believe the error is
related to commit de3910eb79a "edac: change the mem allocation scheme to make
Documentation/kobject.txt happy" [1] and the fact that my system is going
through an error path in the e7xxx_edac driver.

[1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=de3910eb79ac8c0f29a11224661c0ebaaf813039

This is the OOPS:

 [  36.703479] BUG: unable to handle kernel NULL pointer dereference at   (null)
 [  36.703479] IP: [<c045e195>] __wake_up_common+0x1a/0x6a
 [  36.703479] *pde = 7f0c6067
 [  36.703479] Oops: 0000 [#1] SMP
 [  36.703479] Modules linked in: parport_pc parport floppy e7xxx_edac(+) ide_cd_mod edac_core intel_rng cdrom microcode(+) dm_snapshot dm_zero dm_mirror dm_region_hash d
 [  36.703479] Pid: 933, comm: modprobe Tainted: G        W    3.6.0-rc2-00111-gc1999ee #12 Dell Computer Corporation PowerEdge 2600             /0F0364
 [  36.703479] EIP: 0060:[<c045e195>] EFLAGS: 00010093 CPU: 3
 [  36.703479] EIP is at __wake_up_common+0x1a/0x6a
 [  36.703479] EAX: f47b0984 EBX: fffffff4 ECX: 00000000 EDX: 00000003
 [  36.703479] ESI: f47b0984 EDI: 00000282 EBP: f3dc7d38 ESP: f3dc7d1c
 [  36.703479]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
 [  36.703479] CR0: 8005003b CR2: 00000000 CR3: 347d4000 CR4: 000007d0
 [  36.703479] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
 [  36.703479] DR6: ffff0ff0 DR7: 00000400
 [  36.703479] Process modprobe (pid: 933, ti=f3dc6000 task=f3db9520 task.ti=f3dc6000)
 [  36.703479] Stack:
 [  36.703479]  00000000 00000000 00000003 c046701a f47b0980 f47b0984 00000282 f3dc7d54
 [  36.703479]  c046703f 00000000 00000000 f47b08b0 f47b08b0 00000000 f3dc7d74 c06961ce
 [  36.703479]  f3dc7d74 f3dc7d80 c05e2837 c094c4cc f47b08b0 f47b08b0 f3dc7d88 c068d56d
 [  36.703479] Call Trace:
 [  36.703479]  [<c046701a>] ? complete_all+0x1a/0x50
 [  36.703479]  [<c046703f>] complete_all+0x3f/0x50
 [  36.703479]  [<c06961ce>] device_pm_remove+0x23/0xa2
 [  36.703479]  [<c05e2837>] ? kobject_put+0x5b/0x5d
 [  36.703479]  [<c068d56d>] device_del+0x34/0x142
 [  36.703479]  [<f8547884>] edac_unregister_sysfs+0x3b/0x5c [edac_core]
 [  36.703479]  [<f8545041>] edac_mc_free+0x29/0x2f [edac_core]
 [  36.703479]  [<f860163f>] e7xxx_probe1+0x268/0x311 [e7xxx_edac]
 [  36.703479]  [<c0603d00>] ? __pci_enable_device_flags+0x8f/0xd3
 [  36.703479]  [<f8601b0b>] e7xxx_init_one+0x56/0x61 [e7xxx_edac]
 [  36.703479]  [<c0604f85>] local_pci_probe+0x13/0x15
 ...


Fengguang Wu (1):
  edac_mc: fix kfree calls in the error path

Shaun Ruffell (2):
  edac: edac_mc_free() cannot assume mem_ctl_info is registered in
    sysfs.
  edac: edac_mc no longer deals with kobjects directly.

 drivers/edac/edac_mc.c | 64 ++++++++++++++++++++++++++++++--------------------
 include/linux/edac.h   |  1 -
 2 files changed, 39 insertions(+), 26 deletions(-)

-- 
1.7.11.2

[PATCH v2 1/3] edac_mc: fix kfree calls in the error path

[Shaun Ruffell]

From: Fengguang Wu <fengguang.wu@intel.com>

From: Fengguang Wu <fengguang.wu@intel.com>

We need to free up memory in this order:

  free csrows[i]->channels[j]
  free csrows[i]->channels
  free csrows[i]
  free csrows

Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
 drivers/edac/edac_mc.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 616d90b..9037ffa 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -419,14 +419,16 @@ error:
 		kfree(mci->dimms);
 	}
 	if (mci->csrows) {
-		for (chn = 0; chn < tot_channels; chn++) {
-			csr = mci->csrows[chn];
+		for (row = 0; row < tot_csrows; row++) {
+			csr = mci->csrows[row];
 			if (csr) {
-				for (chn = 0; chn < tot_channels; chn++)
-					kfree(csr->channels[chn]);
+				if (csr->channels) {
+					for (chn = 0; chn < tot_channels; chn++)
+						kfree(csr->channels[chn]);
+					kfree(csr->channels);
+				}
 				kfree(csr);
 			}
-			kfree(mci->csrows[i]);
 		}
 		kfree(mci->csrows);
 	}
-- 
1.7.11.2

[PATCH v2 2/3] edac: edac_mc_free() cannot assume mem_ctl_info is registered in sysfs.

[Shaun Ruffell]

edac_mc_free() may need to deallocate any memory associated with struct
mem_ctl_info directly if the structure was never registered with sysfs in
edac_mc_add_mc(). This moves the error handling code from edac_mc_alloc() into a
dedicated function to be called by edac_mc_free() as well if necessary.

This resolves a NULL pointer dereference from the following code path first
introduced in 3.6-rc1:

  EDAC MC: Ver: 3.0.0
  EDAC DEBUG: edac_mc_sysfs_init: device mc created
  EDAC DEBUG: e7xxx_init_one:
  EDAC DEBUG: e7xxx_probe1: mci
  EDAC DEBUG: edac_mc_alloc: errcount layer 0 size 8
  EDAC DEBUG: edac_mc_alloc: errcount layer 1 size 16
  EDAC DEBUG: edac_mc_alloc: allocating 48 error counters
  EDAC DEBUG: edac_mc_alloc: allocating 1068 bytes for mci data (16 ranks, 16 csrows/channels)
  EDAC DEBUG: e7xxx_probe1: init mci
  EDAC DEBUG: e7xxx_probe1: init pvt
  EDAC e7xxx: error reporting device not found:vendor 8086 device 0x2541 (broken BIOS?)
  EDAC DEBUG: edac_mc_free:
  Floppy drive(s): fd0 is 1.44M
  EDAC DEBUG: edac_unregister_sysfs: Unregistering device (null)

Signed-off-by: Shaun Ruffell <sruffell@digium.com>
---
 drivers/edac/edac_mc.c | 59 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 20 deletions(-)

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 9037ffa..d5dc9da 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -199,6 +199,36 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
 	return (void *)(((unsigned long)ptr) + align - r);
 }
 
+static void _edac_mc_free(struct mem_ctl_info *mci)
+{
+	int i, chn, row;
+	struct csrow_info *csr;
+	const unsigned int tot_dimms = mci->tot_dimms;
+	const unsigned int tot_channels = mci->num_cschannel;
+	const unsigned int tot_csrows = mci->nr_csrows;
+
+	if (mci->dimms) {
+		for (i = 0; i < tot_dimms; i++)
+			kfree(mci->dimms[i]);
+		kfree(mci->dimms);
+	}
+	if (mci->csrows) {
+		for (row = 0; row < tot_csrows; row++) {
+			csr = mci->csrows[row];
+			if (csr) {
+				if (csr->channels) {
+					for (chn = 0; chn < tot_channels; chn++)
+						kfree(csr->channels[chn]);
+					kfree(csr->channels);
+				}
+				kfree(csr);
+			}
+		}
+		kfree(mci->csrows);
+	}
+	kfree(mci);
+}
+
 /**
  * edac_mc_alloc: Allocate and partially fill a struct mem_ctl_info structure
  * @mc_num:		Memory controller number
@@ -413,26 +443,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
 	return mci;
 
 error:
-	if (mci->dimms) {
-		for (i = 0; i < tot_dimms; i++)
-			kfree(mci->dimms[i]);
-		kfree(mci->dimms);
-	}
-	if (mci->csrows) {
-		for (row = 0; row < tot_csrows; row++) {
-			csr = mci->csrows[row];
-			if (csr) {
-				if (csr->channels) {
-					for (chn = 0; chn < tot_channels; chn++)
-						kfree(csr->channels[chn]);
-					kfree(csr->channels);
-				}
-				kfree(csr);
-			}
-		}
-		kfree(mci->csrows);
-	}
-	kfree(mci);
+	_edac_mc_free(mci);
 
 	return NULL;
 }
@@ -447,6 +458,14 @@ void edac_mc_free(struct mem_ctl_info *mci)
 {
 	edac_dbg(1, "\n");
 
+	/* If we're not yet registered with sysfs free only what was allocated
+	 * in edac_mc_alloc().
+	 */
+	if (!device_is_registered(&mci->dev)) {
+		_edac_mc_free(mci);
+		return;
+	}
+
 	/* the mci instance is freed here, when the sysfs object is dropped */
 	edac_unregister_sysfs(mci);
 }
-- 
1.7.11.2

[PATCH v2 3/3] edac: edac_mc no longer deals with kobjects directly.

[Shaun Ruffell]

There are no more embedded kobjects in struct mem_ctl_info. Remove a header and
a comment that does not reflect the code anymore.

Signed-off-by: Shaun Ruffell <sruffell@digium.com>
---
 drivers/edac/edac_mc.c | 7 -------
 include/linux/edac.h   | 1 -
 2 files changed, 8 deletions(-)

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index d5dc9da..02f0d3e 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -433,13 +433,6 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
 
 	mci->op_state = OP_ALLOC;
 
-	/* at this point, the root kobj is valid, and in order to
-	 * 'free' the object, then the function:
-	 *      edac_mc_unregister_sysfs_main_kobj() must be called
-	 * which will perform kobj unregistration and the actual free
-	 * will occur during the kobject callback operation
-	 */
-
 	return mci;
 
 error:
diff --git a/include/linux/edac.h b/include/linux/edac.h
index bab9f84..aeddb3f 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -14,7 +14,6 @@
 
 #include <linux/atomic.h>
 #include <linux/device.h>
-#include <linux/kobject.h>
 #include <linux/completion.h>
 #include <linux/workqueue.h>
 #include <linux/debugfs.h>
-- 
1.7.11.2

Re: [PATCH v2 1/3] edac_mc: fix kfree calls in the error path

[Shaun Ruffell]

On Fri, Sep 14, 2012 at 12:58:16PM -0500, Shaun Ruffell wrote:
> From: Fengguang Wu <fengguang.wu@intel.com>
> 
> From: Fengguang Wu <fengguang.wu@intel.com>
 
Darn it. If you push this through, mind fixing the above?