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
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 <[email protected]>
Acked-by: Aristeu Rozanski <[email protected]>
---
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
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
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 .
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 .
>
> .
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