Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 0/4] EDAC/mc: Fixes for mci device removal
@ 2020-02-12 12:03 Robert Richter
  2020-02-12 12:03 ` [PATCH v3 1/4] Revert parts of "EDAC/mc_sysfs: Make debug messages consistent" Robert Richter
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Robert Richter @ 2020-02-12 12:03 UTC (permalink / raw)
  To: Borislav Petkov, Mauro Carvalho Chehab, Tony Luck
  Cc: James Morse, Aristeu Rozanski, Robert Richter, linux-edac, linux-kernel

This series is a split of

 [v2] EDAC/mc: Fix use-after-free and memleaks during device removal
 https://lore.kernel.org/patchwork/patch/1190002/

in smaller patches to better handle stable backports.

Patch #1 helps to ease backports of patch #2 to 5.3 and earlier
kernels, see patch descriptions of both for details.

Patch #2 is the smallest possible fix to address use-after-free and
memleak issues aimed for backports.

Patch #3 is another small patch that is split off for stable. This
fixes code already commited to stable trees.

Patch #4 is the all remaining code containing the rework of the mci
device removal. Resulting code is the same as in v2 except for the
changes outlined for v3:

v3:
 * split patch into smaller pieces to ease backports,
 * removed edac_remove_sysfs_mci_device() in edac_mc_free(), at this
   point the mci device is always unregistered

Robert Richter (4):
  Revert parts of "EDAC/mc_sysfs: Make debug messages consistent"
  EDAC/mc: Fix use-after-free and memleaks during device removal
  EDAC/sysfs: Remove csrow objects on errors
  EDAC/mc: Change mci device removal to use put_device()

 drivers/edac/edac_mc.c       |  20 +++----
 drivers/edac/edac_mc_sysfs.c | 100 +++++++++++++----------------------
 drivers/edac/edac_module.h   |   1 -
 3 files changed, 48 insertions(+), 73 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 1/4] Revert parts of "EDAC/mc_sysfs: Make debug messages consistent"
  2020-02-12 12:03 [PATCH v3 0/4] EDAC/mc: Fixes for mci device removal Robert Richter
@ 2020-02-12 12:03 ` Robert Richter
  2020-02-12 12:03 ` [PATCH v3 2/4] EDAC/mc: Fix use-after-free and memleaks during device removal Robert Richter
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Robert Richter @ 2020-02-12 12:03 UTC (permalink / raw)
  To: Borislav Petkov, Mauro Carvalho Chehab, Tony Luck
  Cc: James Morse, Aristeu Rozanski, Robert Richter, linux-edac, linux-kernel

This reverts parts of commit e701f412030ec3783f1c30c7741492693d6213e3.

Revert a couple of changes to ease stable backports of the next fix on
top of this commit. Due to this revert the next patch will apply
without conflicts on 5.3 based or earlier kernels. As the code
introduced by the revert will be removed, the resulting code of the
fix will be the same as without the revert.

Signed-off-by: Robert Richter <rrichter@marvell.com>
---
 drivers/edac/edac_mc_sysfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 0367554e7437..ba0937140fe4 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -278,7 +278,7 @@ static void csrow_attr_release(struct device *dev)
 {
 	struct csrow_info *csrow = container_of(dev, struct csrow_info, dev);
 
-	edac_dbg(1, "device %s released\n", dev_name(dev));
+	edac_dbg(1, "Releasing csrow device %s\n", dev_name(dev));
 	kfree(csrow);
 }
 
@@ -610,7 +610,7 @@ static void dimm_attr_release(struct device *dev)
 {
 	struct dimm_info *dimm = container_of(dev, struct dimm_info, dev);
 
-	edac_dbg(1, "device %s released\n", dev_name(dev));
+	edac_dbg(1, "Releasing dimm device %s\n", dev_name(dev));
 	kfree(dimm);
 }
 
@@ -895,7 +895,7 @@ static void mci_attr_release(struct device *dev)
 {
 	struct mem_ctl_info *mci = container_of(dev, struct mem_ctl_info, dev);
 
-	edac_dbg(1, "device %s released\n", dev_name(dev));
+	edac_dbg(1, "Releasing csrow device %s\n", dev_name(dev));
 	kfree(mci);
 }
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 2/4] EDAC/mc: Fix use-after-free and memleaks during device removal
  2020-02-12 12:03 [PATCH v3 0/4] EDAC/mc: Fixes for mci device removal Robert Richter
  2020-02-12 12:03 ` [PATCH v3 1/4] Revert parts of "EDAC/mc_sysfs: Make debug messages consistent" Robert Richter
@ 2020-02-12 12:03 ` Robert Richter
  2020-02-12 12:03 ` [PATCH v3 3/4] EDAC/sysfs: Remove csrow objects on errors Robert Richter
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Robert Richter @ 2020-02-12 12:03 UTC (permalink / raw)
  To: Borislav Petkov, Mauro Carvalho Chehab, Tony Luck
  Cc: James Morse, Aristeu Rozanski, Robert Richter, linux-edac,
	linux-kernel, John Garry

A test kernel with the options set below revealed several issues when
removing a mci device:

 DEBUG_TEST_DRIVER_REMOVE
 KASAN
 DEBUG_KMEMLEAK

Issues seen:

1) Use-after-free:

On 27.11.19 17:07:33, John Garry wrote:
> [   22.104498] BUG: KASAN: use-after-free in
> edac_remove_sysfs_mci_device+0x148/0x180

The use-after-free is caused by the mci_for_each_dimm() iterator that
is called in edac_remove_sysfs_mci_device(). The iterator was
introduced with commit c498afaf7df8 ("EDAC: Introduce an
mci_for_each_dimm() iterator"). The iterator loop calls function
device_unregister(&dimm->dev), which removes the sysfs entry of the
device, but also frees the dimm struct in dimm_attr_release(). When
incrementing the loop in mci_for_each_dimm(), the dimm struct is
accessed again, but it is already freed.

The fix is to free all the mci device's subsequent dimm and csrow
objects at a later point in _edac_mc_free() when the mci device is
freed. This keeps the data structures intact and the mci device can be
fully used until its removal. The change allows the save usage of
mci_for_each_dimm() to release dimm devices from sysfs.

2) Memory leaks:

Following memory leaks have been detected:

 # grep edac /sys/kernel/debug/kmemleak | sort | uniq -c
       1     [<000000003c0f58f9>] edac_mc_alloc+0x3bc/0x9d0      # mci->csrows
      16     [<00000000bb932dc0>] edac_mc_alloc+0x49c/0x9d0      # csr->channels
      16     [<00000000e2734dba>] edac_mc_alloc+0x518/0x9d0      # csr->channels[chn]
       1     [<00000000eb040168>] edac_mc_alloc+0x5c8/0x9d0      # mci->dimms
      34     [<00000000ef737c29>] ghes_edac_register+0x1c8/0x3f8 # see edac_mc_alloc()

All leaks are from memory created by edac_mc_alloc().

Note: The test above shows that edac_mc_alloc() was called here from
ghes_edac_register(), thus both functions show up in the stack dump,
but the driver causing the leaks is edac_mc. The comments with the
data structures involved were made manually by analyzing the objdump.

The data structures listed above and created by edac_mc_alloc() are
not properly removed during device removal, which is done in
edac_mc_free(). There are two paths implemented to remove the device
depending on device registration, _edac_mc_free() is called if the
device is not registered and edac_unregister_sysfs() otherwise. The
implemenations differ. For the sysfs case the mci device removal lacks
the removal of subsequent data structures (csrows, channels, dimms).
This causes the memory leaks (see mci_attr_release()).

Fixing this as follows:

Unify code and use the _edac_mc_free() code path to free the mci
struct and subsequent memory allocations. An effect of this is that no
data is freed in edac_mc_sysfs.c (except the "mc" sysfs root node).

The patch has been tested with the above kernel options, no issues
seen any longer.

To backport this patch to 5.4+ kernels, the previous revert patch
needs to be applied too or squashed with this patch to avoid
conflicts.

Reported-by: John Garry <john.garry@huawei.com>
Fixes: c498afaf7df8 ("EDAC: Introduce an mci_for_each_dimm() iterator")
Fixes: faa2ad09c01c ("edac_mc: edac_mc_free() cannot assume mem_ctl_info is registered in sysfs.")
Fixes: 7a623c039075 ("edac: rewrite the sysfs code to use struct device")
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
 drivers/edac/edac_mc.c       | 12 +++---------
 drivers/edac/edac_mc_sysfs.c | 15 +++------------
 2 files changed, 6 insertions(+), 21 deletions(-)

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 7243b88f81d8..69e0d90460e6 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -505,16 +505,10 @@ 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;
-	}
+	if (device_is_registered(&mci->dev))
+		edac_unregister_sysfs(mci);
 
-	/* the mci instance is freed here, when the sysfs object is dropped */
-	edac_unregister_sysfs(mci);
+	_edac_mc_free(mci);
 }
 EXPORT_SYMBOL_GPL(edac_mc_free);
 
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index ba0937140fe4..1c9c6a7b9f66 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -276,10 +276,7 @@ static const struct attribute_group *csrow_attr_groups[] = {
 
 static void csrow_attr_release(struct device *dev)
 {
-	struct csrow_info *csrow = container_of(dev, struct csrow_info, dev);
-
-	edac_dbg(1, "Releasing csrow device %s\n", dev_name(dev));
-	kfree(csrow);
+	/* release device with _edac_mc_free() */
 }
 
 static const struct device_type csrow_attr_type = {
@@ -608,10 +605,7 @@ static const struct attribute_group *dimm_attr_groups[] = {
 
 static void dimm_attr_release(struct device *dev)
 {
-	struct dimm_info *dimm = container_of(dev, struct dimm_info, dev);
-
-	edac_dbg(1, "Releasing dimm device %s\n", dev_name(dev));
-	kfree(dimm);
+	/* release device with _edac_mc_free() */
 }
 
 static const struct device_type dimm_attr_type = {
@@ -893,10 +887,7 @@ static const struct attribute_group *mci_attr_groups[] = {
 
 static void mci_attr_release(struct device *dev)
 {
-	struct mem_ctl_info *mci = container_of(dev, struct mem_ctl_info, dev);
-
-	edac_dbg(1, "Releasing csrow device %s\n", dev_name(dev));
-	kfree(mci);
+	/* release device with _edac_mc_free() */
 }
 
 static const struct device_type mci_attr_type = {
-- 
2.20.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 3/4] EDAC/sysfs: Remove csrow objects on errors
  2020-02-12 12:03 [PATCH v3 0/4] EDAC/mc: Fixes for mci device removal Robert Richter
  2020-02-12 12:03 ` [PATCH v3 1/4] Revert parts of "EDAC/mc_sysfs: Make debug messages consistent" Robert Richter
  2020-02-12 12:03 ` [PATCH v3 2/4] EDAC/mc: Fix use-after-free and memleaks during device removal Robert Richter
@ 2020-02-12 12:03 ` Robert Richter
  2020-02-12 12:03 ` [PATCH v3 4/4] EDAC/mc: Change mci device removal to use put_device() Robert Richter
  2020-02-13 11:05 ` [PATCH v3 0/4] EDAC/mc: Fixes for mci device removal Borislav Petkov
  4 siblings, 0 replies; 9+ messages in thread
From: Robert Richter @ 2020-02-12 12:03 UTC (permalink / raw)
  To: Borislav Petkov, Mauro Carvalho Chehab, Tony Luck
  Cc: James Morse, Aristeu Rozanski, Robert Richter, linux-edac,
	linux-kernel, stable

All created csrow objects must be removed in the error path of
edac_create_csrow_objects(). The objects have been added as devices.
They need to be removed by doing a device_del() *and* put_device()
call to also free their memory. The misssing put_device() leaves a
memory leak. Use device_unregister() instead of device_del() which
properly unregisters the device doing both.

Fixes: 7adc05d2dc3a ("EDAC/sysfs: Drop device references properly")
Cc: <stable@vger.kernel.org>
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
 drivers/edac/edac_mc_sysfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 1c9c6a7b9f66..c70ec0a306d8 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -444,8 +444,7 @@ static int edac_create_csrow_objects(struct mem_ctl_info *mci)
 		csrow = mci->csrows[i];
 		if (!nr_pages_per_csrow(csrow))
 			continue;
-
-		device_del(&mci->csrows[i]->dev);
+		device_unregister(&mci->csrows[i]->dev);
 	}
 
 	return err;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 4/4] EDAC/mc: Change mci device removal to use put_device()
  2020-02-12 12:03 [PATCH v3 0/4] EDAC/mc: Fixes for mci device removal Robert Richter
                   ` (2 preceding siblings ...)
  2020-02-12 12:03 ` [PATCH v3 3/4] EDAC/sysfs: Remove csrow objects on errors Robert Richter
@ 2020-02-12 12:03 ` Robert Richter
  2020-02-13 11:05 ` [PATCH v3 0/4] EDAC/mc: Fixes for mci device removal Borislav Petkov
  4 siblings, 0 replies; 9+ messages in thread
From: Robert Richter @ 2020-02-12 12:03 UTC (permalink / raw)
  To: Borislav Petkov, Mauro Carvalho Chehab, Tony Luck
  Cc: James Morse, Aristeu Rozanski, Robert Richter, linux-edac, linux-kernel

There are dimm and csrow devices linked to the mci device esp. to show
up in sysfs. It must be granted that children devices are removed
before its mci parent. Thus, the release functions must be called in
the correct order and may not miss any child before releasing its
parent. In current implementation this is only granted by the correct
order of release functions.

A much better approach is to use put_device() that releases the device
only after all users are gone. It is the recommended way to release a
device and free its memory. The function uses the device's refcount
and only frees it if there are no users of it anymore such as
children.

So implement a mci_release() function to remove mci devices, use
put_device() to free it and early initialize the mci device right
after its struct was allocated. Change the release function so that it
can be universally used no matter if the device is registered or not.
Since subsequent dimm and csrow sysfs links are implemented as
children devices, their refcounts will keep the parent mci device from
being removed as long as sysfs entries exist and until all users have
been unregistered in edac_remove_sysfs_mci_device().

Remove edac_unregister_sysfs() and merge mci sysfs removal into
edac_remove_sysfs_mci_device(). There is only a single instance now
that removes the sysfs entries. The function can now being used in the
error paths for cleanup.

Also, create device release functions for all involved devices
(dev->release), remove device_type release functions (dev_type->
release) and also use dev->init_name instead of dev_set_name().

Signed-off-by: Robert Richter <rrichter@marvell.com>
Acked-by: Aristeu Rozanski <aris@redhat.com>
---
 drivers/edac/edac_mc.c       | 12 +++--
 drivers/edac/edac_mc_sysfs.c | 90 +++++++++++++++---------------------
 drivers/edac/edac_module.h   |  1 -
 3 files changed, 47 insertions(+), 56 deletions(-)

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 69e0d90460e6..64785e644482 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -278,6 +278,12 @@ void *edac_align_ptr(void **p, unsigned int size, int n_elems)
 
 static void _edac_mc_free(struct mem_ctl_info *mci)
 {
+	put_device(&mci->dev);
+}
+
+static void mci_release(struct device *dev)
+{
+	struct mem_ctl_info *mci = container_of(dev, struct mem_ctl_info, dev);
 	struct csrow_info *csr;
 	int i, chn, row;
 
@@ -371,6 +377,9 @@ struct mem_ctl_info *edac_mc_alloc(unsigned int mc_num,
 	if (mci == NULL)
 		return NULL;
 
+	mci->dev.release = mci_release;
+	device_initialize(&mci->dev);
+
 	/* Adjust pointers so they point within the memory we just allocated
 	 * rather than an imaginary chunk of memory located at address 0.
 	 */
@@ -505,9 +514,6 @@ void edac_mc_free(struct mem_ctl_info *mci)
 {
 	edac_dbg(1, "\n");
 
-	if (device_is_registered(&mci->dev))
-		edac_unregister_sysfs(mci);
-
 	_edac_mc_free(mci);
 }
 EXPORT_SYMBOL_GPL(edac_mc_free);
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index c70ec0a306d8..408bace699dc 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -274,14 +274,8 @@ static const struct attribute_group *csrow_attr_groups[] = {
 	NULL
 };
 
-static void csrow_attr_release(struct device *dev)
-{
-	/* release device with _edac_mc_free() */
-}
-
 static const struct device_type csrow_attr_type = {
 	.groups		= csrow_attr_groups,
-	.release	= csrow_attr_release,
 };
 
 /*
@@ -387,6 +381,14 @@ static const struct attribute_group *csrow_dev_groups[] = {
 	NULL
 };
 
+static void csrow_release(struct device *dev)
+{
+	/*
+	 * Nothing to do. We just unregister sysfs here. The mci
+	 * device owns the data and will also release it.
+	 */
+}
+
 static inline int nr_pages_per_csrow(struct csrow_info *csrow)
 {
 	int chan, nr_pages = 0;
@@ -405,6 +407,7 @@ static int edac_create_csrow_object(struct mem_ctl_info *mci,
 
 	csrow->dev.type = &csrow_attr_type;
 	csrow->dev.groups = csrow_dev_groups;
+	csrow->dev.release = csrow_release;
 	device_initialize(&csrow->dev);
 	csrow->dev.parent = &mci->dev;
 	csrow->mci = mci;
@@ -441,10 +444,8 @@ static int edac_create_csrow_objects(struct mem_ctl_info *mci)
 
 error:
 	for (--i; i >= 0; i--) {
-		csrow = mci->csrows[i];
-		if (!nr_pages_per_csrow(csrow))
-			continue;
-		device_unregister(&mci->csrows[i]->dev);
+		if (device_is_registered(&mci->csrows[i]->dev))
+			device_unregister(&mci->csrows[i]->dev);
 	}
 
 	return err;
@@ -453,15 +454,13 @@ static int edac_create_csrow_objects(struct mem_ctl_info *mci)
 static void edac_delete_csrow_objects(struct mem_ctl_info *mci)
 {
 	int i;
-	struct csrow_info *csrow;
 
-	for (i = mci->nr_csrows - 1; i >= 0; i--) {
-		csrow = mci->csrows[i];
-		if (!nr_pages_per_csrow(csrow))
-			continue;
-		device_unregister(&mci->csrows[i]->dev);
+	for (i = 0; i < mci->nr_csrows; i++) {
+		if (device_is_registered(&mci->csrows[i]->dev))
+			device_unregister(&mci->csrows[i]->dev);
 	}
 }
+
 #endif
 
 /*
@@ -602,16 +601,18 @@ static const struct attribute_group *dimm_attr_groups[] = {
 	NULL
 };
 
-static void dimm_attr_release(struct device *dev)
-{
-	/* release device with _edac_mc_free() */
-}
-
 static const struct device_type dimm_attr_type = {
 	.groups		= dimm_attr_groups,
-	.release	= dimm_attr_release,
 };
 
+static void dimm_release(struct device *dev)
+{
+	/*
+	 * Nothing to do. We just unregister sysfs here. The mci
+	 * device owns the data and will also release it.
+	 */
+}
+
 /* Create a DIMM object under specifed memory controller device */
 static int edac_create_dimm_object(struct mem_ctl_info *mci,
 				   struct dimm_info *dimm)
@@ -620,6 +621,7 @@ static int edac_create_dimm_object(struct mem_ctl_info *mci,
 	dimm->mci = mci;
 
 	dimm->dev.type = &dimm_attr_type;
+	dimm->dev.release = dimm_release;
 	device_initialize(&dimm->dev);
 
 	dimm->dev.parent = &mci->dev;
@@ -884,14 +886,8 @@ static const struct attribute_group *mci_attr_groups[] = {
 	NULL
 };
 
-static void mci_attr_release(struct device *dev)
-{
-	/* release device with _edac_mc_free() */
-}
-
 static const struct device_type mci_attr_type = {
 	.groups		= mci_attr_groups,
-	.release	= mci_attr_release,
 };
 
 /*
@@ -910,8 +906,6 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 
 	/* get the /sys/devices/system/edac subsys reference */
 	mci->dev.type = &mci_attr_type;
-	device_initialize(&mci->dev);
-
 	mci->dev.parent = mci_pdev;
 	mci->dev.groups = groups;
 	dev_set_name(&mci->dev, "mc%d", mci->mc_idx);
@@ -921,7 +915,7 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 	err = device_add(&mci->dev);
 	if (err < 0) {
 		edac_dbg(1, "failure: create device %s\n", dev_name(&mci->dev));
-		put_device(&mci->dev);
+		/* no put_device() here, free mci with _edac_mc_free() */
 		return err;
 	}
 
@@ -937,24 +931,20 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 
 		err = edac_create_dimm_object(mci, dimm);
 		if (err)
-			goto fail_unregister_dimm;
+			goto fail;
 	}
 
 #ifdef CONFIG_EDAC_LEGACY_SYSFS
 	err = edac_create_csrow_objects(mci);
 	if (err < 0)
-		goto fail_unregister_dimm;
+		goto fail;
 #endif
 
 	edac_create_debugfs_nodes(mci);
 	return 0;
 
-fail_unregister_dimm:
-	mci_for_each_dimm(mci, dimm) {
-		if (device_is_registered(&dimm->dev))
-			device_unregister(&dimm->dev);
-	}
-	device_unregister(&mci->dev);
+fail:
+	edac_remove_sysfs_mci_device(mci);
 
 	return err;
 }
@@ -966,6 +956,9 @@ void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci)
 {
 	struct dimm_info *dimm;
 
+	if (!device_is_registered(&mci->dev))
+		return;
+
 	edac_dbg(0, "\n");
 
 #ifdef CONFIG_EDAC_DEBUG
@@ -976,17 +969,14 @@ void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci)
 #endif
 
 	mci_for_each_dimm(mci, dimm) {
-		if (dimm->nr_pages == 0)
+		if (!device_is_registered(&dimm->dev))
 			continue;
 		edac_dbg(1, "unregistering device %s\n", dev_name(&dimm->dev));
 		device_unregister(&dimm->dev);
 	}
-}
 
-void edac_unregister_sysfs(struct mem_ctl_info *mci)
-{
-	edac_dbg(1, "unregistering device %s\n", dev_name(&mci->dev));
-	device_unregister(&mci->dev);
+	/* only remove the device, but keep mci */
+	device_del(&mci->dev);
 }
 
 static void mc_attr_release(struct device *dev)
@@ -1000,9 +990,6 @@ static void mc_attr_release(struct device *dev)
 	kfree(dev);
 }
 
-static const struct device_type mc_attr_type = {
-	.release	= mc_attr_release,
-};
 /*
  * Init/exit code for the module. Basically, creates/removes /sys/class/rc
  */
@@ -1015,11 +1002,10 @@ int __init edac_mc_sysfs_init(void)
 		return -ENOMEM;
 
 	mci_pdev->bus = edac_get_sysfs_subsys();
-	mci_pdev->type = &mc_attr_type;
-	device_initialize(mci_pdev);
-	dev_set_name(mci_pdev, "mc");
+	mci_pdev->release = mc_attr_release;
+	mci_pdev->init_name = "mc";
 
-	err = device_add(mci_pdev);
+	err = device_register(mci_pdev);
 	if (err < 0) {
 		edac_dbg(1, "failure: create device %s\n", dev_name(mci_pdev));
 		put_device(mci_pdev);
diff --git a/drivers/edac/edac_module.h b/drivers/edac/edac_module.h
index 388427d378b1..aa1f91688eb8 100644
--- a/drivers/edac/edac_module.h
+++ b/drivers/edac/edac_module.h
@@ -28,7 +28,6 @@ void edac_mc_sysfs_exit(void);
 extern int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 					const struct attribute_group **groups);
 extern void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci);
-void edac_unregister_sysfs(struct mem_ctl_info *mci);
 extern int edac_get_log_ue(void);
 extern int edac_get_log_ce(void);
 extern int edac_get_panic_on_ue(void);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 0/4] EDAC/mc: Fixes for mci device removal
  2020-02-12 12:03 [PATCH v3 0/4] EDAC/mc: Fixes for mci device removal Robert Richter
                   ` (3 preceding siblings ...)
  2020-02-12 12:03 ` [PATCH v3 4/4] EDAC/mc: Change mci device removal to use put_device() Robert Richter
@ 2020-02-13 11:05 ` Borislav Petkov
  2020-02-13 11:10   ` John Garry
  4 siblings, 1 reply; 9+ messages in thread
From: Borislav Petkov @ 2020-02-13 11:05 UTC (permalink / raw)
  To: Robert Richter, John Garry
  Cc: Mauro Carvalho Chehab, Tony Luck, James Morse, Aristeu Rozanski,
	linux-edac, linux-kernel

On Wed, Feb 12, 2020 at 01:03:36PM +0100, Robert Richter wrote:
> This series is a split of
> 
>  [v2] EDAC/mc: Fix use-after-free and memleaks during device removal
>  https://lore.kernel.org/patchwork/patch/1190002/
> 
> in smaller patches to better handle stable backports.
> 
> Patch #1 helps to ease backports of patch #2 to 5.3 and earlier
> kernels, see patch descriptions of both for details.
> 
> Patch #2 is the smallest possible fix to address use-after-free and
> memleak issues aimed for backports.
> 
> Patch #3 is another small patch that is split off for stable. This
> fixes code already commited to stable trees.
> 
> Patch #4 is the all remaining code containing the rework of the mci
> device removal. Resulting code is the same as in v2 except for the
> changes outlined for v3:
> 
> v3:
>  * split patch into smaller pieces to ease backports,
>  * removed edac_remove_sysfs_mci_device() in edac_mc_free(), at this
>    point the mci device is always unregistered
> 
> Robert Richter (4):
>   Revert parts of "EDAC/mc_sysfs: Make debug messages consistent"
>   EDAC/mc: Fix use-after-free and memleaks during device removal
>   EDAC/sysfs: Remove csrow objects on errors
>   EDAC/mc: Change mci device removal to use put_device()
> 
>  drivers/edac/edac_mc.c       |  20 +++----
>  drivers/edac/edac_mc_sysfs.c | 100 +++++++++++++----------------------
>  drivers/edac/edac_module.h   |   1 -
>  3 files changed, 48 insertions(+), 73 deletions(-)

Thanks, first three (1+2 squashed) pushed here:

https://git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git/log/?h=edac-urgent

@John: if you could run them one more time on your machines, that would
be great!

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 0/4] EDAC/mc: Fixes for mci device removal
  2020-02-13 11:05 ` [PATCH v3 0/4] EDAC/mc: Fixes for mci device removal Borislav Petkov
@ 2020-02-13 11:10   ` John Garry
  2020-02-13 12:08     ` John Garry
  0 siblings, 1 reply; 9+ messages in thread
From: John Garry @ 2020-02-13 11:10 UTC (permalink / raw)
  To: Borislav Petkov, Robert Richter
  Cc: Mauro Carvalho Chehab, Tony Luck, James Morse, Aristeu Rozanski,
	linux-edac, linux-kernel

On 13/02/2020 11:05, Borislav Petkov wrote:
>> Robert Richter (4):
>>    Revert parts of "EDAC/mc_sysfs: Make debug messages consistent"
>>    EDAC/mc: Fix use-after-free and memleaks during device removal
>>    EDAC/sysfs: Remove csrow objects on errors
>>    EDAC/mc: Change mci device removal to use put_device()
>>
>>   drivers/edac/edac_mc.c       |  20 +++----
>>   drivers/edac/edac_mc_sysfs.c | 100 +++++++++++++----------------------
>>   drivers/edac/edac_module.h   |   1 -
>>   3 files changed, 48 insertions(+), 73 deletions(-)
> Thanks, first three (1+2 squashed) pushed here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git/log/?h=edac-urgent
> 
> @John: if you could run them one more time on your machines, that would
> be great!

ok, give me a little while and I'll test that branch

Cheers

> 
> Thx.
> 
> -- Regards/Gruss, Boris. 
> https://people.kernel.org/tglx/notes-about-netiquette .


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 0/4] EDAC/mc: Fixes for mci device removal
  2020-02-13 11:10   ` John Garry
@ 2020-02-13 12:08     ` John Garry
  2020-02-13 12:28       ` Borislav Petkov
  0 siblings, 1 reply; 9+ messages in thread
From: John Garry @ 2020-02-13 12:08 UTC (permalink / raw)
  To: Borislav Petkov, Robert Richter
  Cc: Mauro Carvalho Chehab, Tony Luck, James Morse, Aristeu Rozanski,
	linux-edac, linux-kernel

On 13/02/2020 11:10, John Garry wrote:
> On 13/02/2020 11:05, Borislav Petkov wrote:
>>> Robert Richter (4):
>>>    Revert parts of "EDAC/mc_sysfs: Make debug messages consistent"
>>>    EDAC/mc: Fix use-after-free and memleaks during device removal
>>>    EDAC/sysfs: Remove csrow objects on errors
>>>    EDAC/mc: Change mci device removal to use put_device()
>>>
>>>   drivers/edac/edac_mc.c       |  20 +++----
>>>   drivers/edac/edac_mc_sysfs.c | 100 +++++++++++++----------------------
>>>   drivers/edac/edac_module.h   |   1 -
>>>   3 files changed, 48 insertions(+), 73 deletions(-)
>> Thanks, first three (1+2 squashed) pushed here:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git/log/?h=edac-urgent 
>>
>>
>> @John: if you could run them one more time on your machines, that would
>> be great!
> 
> ok, give me a little while and I'll test that branch
> 

Yeah, it looks ok - I'm just booting and running a kmemleak scan

Cheers

> 
>>
>> Thx.
>>
>> -- Regards/Gruss, Boris. 
>> https://people.kernel.org/tglx/notes-about-netiquette .
> 
> .


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 0/4] EDAC/mc: Fixes for mci device removal
  2020-02-13 12:08     ` John Garry
@ 2020-02-13 12:28       ` Borislav Petkov
  0 siblings, 0 replies; 9+ messages in thread
From: Borislav Petkov @ 2020-02-13 12:28 UTC (permalink / raw)
  To: John Garry
  Cc: Robert Richter, Mauro Carvalho Chehab, Tony Luck, James Morse,
	Aristeu Rozanski, linux-edac, linux-kernel

On Thu, Feb 13, 2020 at 12:08:59PM +0000, John Garry wrote:
> Yeah, it looks ok - I'm just booting and running a kmemleak scan

Thanks, I'll add your Tested-by:

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-12 12:03 [PATCH v3 0/4] EDAC/mc: Fixes for mci device removal Robert Richter
2020-02-12 12:03 ` [PATCH v3 1/4] Revert parts of "EDAC/mc_sysfs: Make debug messages consistent" Robert Richter
2020-02-12 12:03 ` [PATCH v3 2/4] EDAC/mc: Fix use-after-free and memleaks during device removal Robert Richter
2020-02-12 12:03 ` [PATCH v3 3/4] EDAC/sysfs: Remove csrow objects on errors Robert Richter
2020-02-12 12:03 ` [PATCH v3 4/4] EDAC/mc: Change mci device removal to use put_device() Robert Richter
2020-02-13 11:05 ` [PATCH v3 0/4] EDAC/mc: Fixes for mci device removal Borislav Petkov
2020-02-13 11:10   ` John Garry
2020-02-13 12:08     ` John Garry
2020-02-13 12:28       ` Borislav Petkov

Linux-EDAC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-edac/0 linux-edac/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-edac linux-edac/ https://lore.kernel.org/linux-edac \
		linux-edac@vger.kernel.org
	public-inbox-index linux-edac

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-edac


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git