* EDAC: Fix memory leak in creating CSROW object @ 2019-04-18 2:27 Pan Bian 2019-04-18 17:25 ` Borislav Petkov 0 siblings, 1 reply; 11+ messages in thread From: Pan Bian @ 2019-04-18 2:27 UTC (permalink / raw) To: Borislav Petkov, Mauro Carvalho Chehab, James Morse Cc: linux-edac, linux-kernel, Pan Bian In the function that creates a CSROW object, the object is not released when failing to add the device to device hierarchy. This may result in a memory leak bug. Signed-off-by: Pan Bian <bianpan2016@163.com> --- drivers/edac/edac_mc_sysfs.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c index 4641746..2dafb08 100644 --- a/drivers/edac/edac_mc_sysfs.c +++ b/drivers/edac/edac_mc_sysfs.c @@ -404,6 +404,7 @@ static inline int nr_pages_per_csrow(struct csrow_info *csrow) static int edac_create_csrow_object(struct mem_ctl_info *mci, struct csrow_info *csrow, int index) { + int err; csrow->dev.type = &csrow_attr_type; csrow->dev.groups = csrow_dev_groups; device_initialize(&csrow->dev); @@ -415,7 +416,10 @@ static int edac_create_csrow_object(struct mem_ctl_info *mci, edac_dbg(0, "creating (virtual) csrow node %s\n", dev_name(&csrow->dev)); - return device_add(&csrow->dev); + err = device_add(&csrow->dev); + if (err) + put_device(&csrow->dev); + return err; } /* Create a CSROW object under specifed edac_mc_device */ -- 2.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: EDAC: Fix memory leak in creating CSROW object 2019-04-18 2:27 EDAC: Fix memory leak in creating CSROW object Pan Bian @ 2019-04-18 17:25 ` Borislav Petkov [not found] ` <20190419003536.GA57795@bianpan2016@163.com> 0 siblings, 1 reply; 11+ messages in thread From: Borislav Petkov @ 2019-04-18 17:25 UTC (permalink / raw) To: Pan Bian; +Cc: Mauro Carvalho Chehab, James Morse, linux-edac, linux-kernel On Thu, Apr 18, 2019 at 10:27:18AM +0800, Pan Bian wrote: > In the function that creates a CSROW object, the object is not released > when failing to add the device to device hierarchy. Are you sure about this? > This may result in a memory leak bug. "May"? I see a loop which unwinds by putting the already created devices. Do you? -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <20190419003536.GA57795@bianpan2016@163.com>]
* Re: EDAC: Fix memory leak in creating CSROW object [not found] ` <20190419003536.GA57795@bianpan2016@163.com> @ 2019-04-19 0:45 ` Borislav Petkov 2019-04-27 21:49 ` Greg KH 0 siblings, 1 reply; 11+ messages in thread From: Borislav Petkov @ 2019-04-19 0:45 UTC (permalink / raw) To: PanBian, Greg KH Cc: Mauro Carvalho Chehab, James Morse, linux-edac, linux-kernel On Fri, Apr 19, 2019 at 08:35:36AM +0800, PanBian wrote: > Yes, I see that. Because the loop start with (--i), there is no put > operation for the device that fails to create. So, I think we cannot > rule out the possibility of memory leak. Ok, so this is not something you trigger - you're basically staring at the code? Well, there's something else questionable in that code which I asked Greg about today but we didn't finish that conversation, let me CC him. So AFAIU, devices for which device_add() has returned success, should be removed with their counterpart device_del(). edac_create_csrow_objects(), however, does put_device() on those in the "unwinding" loop. And for the case where device_add() fails, you should do put_device() to it. I.e., what you're saying. So I think we need to figure what needs to be done when before fixing this properly. Greg? -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: EDAC: Fix memory leak in creating CSROW object 2019-04-19 0:45 ` Borislav Petkov @ 2019-04-27 21:49 ` Greg KH 2019-05-08 10:57 ` Borislav Petkov ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Greg KH @ 2019-04-27 21:49 UTC (permalink / raw) To: Borislav Petkov Cc: PanBian, Mauro Carvalho Chehab, James Morse, linux-edac, linux-kernel On Fri, Apr 19, 2019 at 02:45:16AM +0200, Borislav Petkov wrote: > On Fri, Apr 19, 2019 at 08:35:36AM +0800, PanBian wrote: > > Yes, I see that. Because the loop start with (--i), there is no put > > operation for the device that fails to create. So, I think we cannot > > rule out the possibility of memory leak. > > Ok, so this is not something you trigger - you're basically staring at > the code? > > Well, there's something else questionable in that code which I asked > Greg about today but we didn't finish that conversation, let me CC him. > > So AFAIU, devices for which device_add() has returned success, > should be removed with their counterpart device_del(). > edac_create_csrow_objects(), however, does put_device() on those in the > "unwinding" loop. > > And for the case where device_add() fails, you should do put_device() to > it. I.e., what you're saying. > > So I think we need to figure what needs to be done when before fixing > this properly. > > Greg? How about this patch, I think it fixes up everything you need to do here, right? diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c index 464174685589..0fb2d1de6d0e 100644 --- a/drivers/edac/edac_mc_sysfs.c +++ b/drivers/edac/edac_mc_sysfs.c @@ -404,6 +404,8 @@ static inline int nr_pages_per_csrow(struct csrow_info *csrow) static int edac_create_csrow_object(struct mem_ctl_info *mci, struct csrow_info *csrow, int index) { + int retval; + csrow->dev.type = &csrow_attr_type; csrow->dev.groups = csrow_dev_groups; device_initialize(&csrow->dev); @@ -415,7 +417,10 @@ static int edac_create_csrow_object(struct mem_ctl_info *mci, edac_dbg(0, "creating (virtual) csrow node %s\n", dev_name(&csrow->dev)); - return device_add(&csrow->dev); + retval = device_add(&csrow->dev); + if (retval) + put_device(&csrow->dev); + return retval; } /* Create a CSROW object under specifed edac_mc_device */ @@ -649,6 +654,8 @@ static int edac_create_dimm_object(struct mem_ctl_info *mci, edac_dbg(0, "creating rank/dimm device %s\n", dev_name(&dimm->dev)); + if (err) + put_device(&dimm->dev); return err; } @@ -928,6 +935,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); goto out; } ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: EDAC: Fix memory leak in creating CSROW object 2019-04-27 21:49 ` Greg KH @ 2019-05-08 10:57 ` Borislav Petkov 2019-05-08 12:47 ` Greg KH 2019-05-08 11:02 ` [PATCH 1/2] EDAC/sysfs: Fix memory leak when creating a csrow object Borislav Petkov 2019-05-08 11:06 ` [PATCH 2/2] EDAC/sysfs: Drop device references properly Borislav Petkov 2 siblings, 1 reply; 11+ messages in thread From: Borislav Petkov @ 2019-05-08 10:57 UTC (permalink / raw) To: Greg KH Cc: PanBian, Mauro Carvalho Chehab, James Morse, linux-edac, linux-kernel On Sat, Apr 27, 2019 at 11:49:25PM +0200, Greg KH wrote: > How about this patch, I think it fixes up everything you need to do > here, right? Almost, see the two patches as a reply to this message. I've taken Pan's original patch because it is correct and I doubt you're dying for attribution :-) Then, I productized yours, with some additions. :) Thoughts? Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: EDAC: Fix memory leak in creating CSROW object 2019-05-08 10:57 ` Borislav Petkov @ 2019-05-08 12:47 ` Greg KH 2019-05-08 18:50 ` Borislav Petkov 0 siblings, 1 reply; 11+ messages in thread From: Greg KH @ 2019-05-08 12:47 UTC (permalink / raw) To: Borislav Petkov Cc: PanBian, Mauro Carvalho Chehab, James Morse, linux-edac, linux-kernel On Wed, May 08, 2019 at 12:57:43PM +0200, Borislav Petkov wrote: > On Sat, Apr 27, 2019 at 11:49:25PM +0200, Greg KH wrote: > > How about this patch, I think it fixes up everything you need to do > > here, right? > > Almost, see the two patches as a reply to this message. I've taken > Pan's original patch because it is correct and I doubt you're dying for > attribution :-) Nope, no need for that :) > Then, I productized yours, with some additions. :) Looks good to me, ship it! ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: EDAC: Fix memory leak in creating CSROW object 2019-05-08 12:47 ` Greg KH @ 2019-05-08 18:50 ` Borislav Petkov 0 siblings, 0 replies; 11+ messages in thread From: Borislav Petkov @ 2019-05-08 18:50 UTC (permalink / raw) To: Greg KH Cc: PanBian, Mauro Carvalho Chehab, James Morse, linux-edac, linux-kernel On Wed, May 08, 2019 at 02:47:54PM +0200, Greg KH wrote: > Looks good to me, ship it! Thx, done! :-) -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] EDAC/sysfs: Fix memory leak when creating a csrow object 2019-04-27 21:49 ` Greg KH 2019-05-08 10:57 ` Borislav Petkov @ 2019-05-08 11:02 ` Borislav Petkov 2019-05-08 12:45 ` Greg KH 2019-05-08 11:06 ` [PATCH 2/2] EDAC/sysfs: Drop device references properly Borislav Petkov 2 siblings, 1 reply; 11+ messages in thread From: Borislav Petkov @ 2019-05-08 11:02 UTC (permalink / raw) To: Greg KH Cc: PanBian, Mauro Carvalho Chehab, James Morse, linux-edac, linux-kernel From 28e7f23939208bea639d6cd3d492cde3f65a7e4f Mon Sep 17 00:00:00 2001 From: Pan Bian <bianpan2016@163.com> Date: Thu, 18 Apr 2019 10:27:18 +0800 In edac_create_csrow_object(), the reference to the object is not released when adding the device to the device hierarchy fails (device_add()). This may result in a memory leak. Signed-off-by: Pan Bian <bianpan2016@163.com> Signed-off-by: Borislav Petkov <bp@suse.de> Cc: Greg KH <gregkh@linuxfoundation.org> Cc: James Morse <james.morse@arm.com> Cc: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: linux-edac <linux-edac@vger.kernel.org> Link: https://lkml.kernel.org/r/1555554438-103953-1-git-send-email-bianpan2016@163.com --- drivers/edac/edac_mc_sysfs.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c index 464174685589..9b7d396f26e9 100644 --- a/drivers/edac/edac_mc_sysfs.c +++ b/drivers/edac/edac_mc_sysfs.c @@ -404,6 +404,8 @@ static inline int nr_pages_per_csrow(struct csrow_info *csrow) static int edac_create_csrow_object(struct mem_ctl_info *mci, struct csrow_info *csrow, int index) { + int err; + csrow->dev.type = &csrow_attr_type; csrow->dev.groups = csrow_dev_groups; device_initialize(&csrow->dev); @@ -415,7 +417,11 @@ static int edac_create_csrow_object(struct mem_ctl_info *mci, edac_dbg(0, "creating (virtual) csrow node %s\n", dev_name(&csrow->dev)); - return device_add(&csrow->dev); + err = device_add(&csrow->dev); + if (err) + put_device(&csrow->dev); + + return err; } /* Create a CSROW object under specifed edac_mc_device */ -- 2.21.0 -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] EDAC/sysfs: Fix memory leak when creating a csrow object 2019-05-08 11:02 ` [PATCH 1/2] EDAC/sysfs: Fix memory leak when creating a csrow object Borislav Petkov @ 2019-05-08 12:45 ` Greg KH 0 siblings, 0 replies; 11+ messages in thread From: Greg KH @ 2019-05-08 12:45 UTC (permalink / raw) To: Borislav Petkov Cc: PanBian, Mauro Carvalho Chehab, James Morse, linux-edac, linux-kernel On Wed, May 08, 2019 at 01:02:50PM +0200, Borislav Petkov wrote: > >From 28e7f23939208bea639d6cd3d492cde3f65a7e4f Mon Sep 17 00:00:00 2001 > From: Pan Bian <bianpan2016@163.com> > Date: Thu, 18 Apr 2019 10:27:18 +0800 > > In edac_create_csrow_object(), the reference to the object is not > released when adding the device to the device hierarchy fails > (device_add()). This may result in a memory leak. > > Signed-off-by: Pan Bian <bianpan2016@163.com> > Signed-off-by: Borislav Petkov <bp@suse.de> > Cc: Greg KH <gregkh@linuxfoundation.org> > Cc: James Morse <james.morse@arm.com> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: linux-edac <linux-edac@vger.kernel.org> > Link: https://lkml.kernel.org/r/1555554438-103953-1-git-send-email-bianpan2016@163.com > --- > drivers/edac/edac_mc_sysfs.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] EDAC/sysfs: Drop device references properly 2019-04-27 21:49 ` Greg KH 2019-05-08 10:57 ` Borislav Petkov 2019-05-08 11:02 ` [PATCH 1/2] EDAC/sysfs: Fix memory leak when creating a csrow object Borislav Petkov @ 2019-05-08 11:06 ` Borislav Petkov 2019-05-08 12:47 ` Greg KH 2 siblings, 1 reply; 11+ messages in thread From: Borislav Petkov @ 2019-05-08 11:06 UTC (permalink / raw) To: Greg KH Cc: PanBian, Mauro Carvalho Chehab, James Morse, linux-edac, linux-kernel -- From: Greg KH <gregkh@linuxfoundation.org> Do put_device() if device_add() fails. [ bp: do device_del() for the successfully created devices in edac_create_csrow_objects(), on the unwind path. ] Signed-off-by: Greg KH <gregkh@linuxfoundation.org> Signed-off-by: Borislav Petkov <bp@suse.de> Link: https://lkml.kernel.org/r/20190427214925.GE16338@kroah.com --- drivers/edac/edac_mc_sysfs.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c index 9b7d396f26e9..7c01e1cc030c 100644 --- a/drivers/edac/edac_mc_sysfs.c +++ b/drivers/edac/edac_mc_sysfs.c @@ -449,7 +449,8 @@ static int edac_create_csrow_objects(struct mem_ctl_info *mci) csrow = mci->csrows[i]; if (!nr_pages_per_csrow(csrow)) continue; - put_device(&mci->csrows[i]->dev); + + device_del(&mci->csrows[i]->dev); } return err; @@ -651,9 +652,11 @@ static int edac_create_dimm_object(struct mem_ctl_info *mci, dev_set_drvdata(&dimm->dev, dimm); pm_runtime_forbid(&mci->dev); - err = device_add(&dimm->dev); + err = device_add(&dimm->dev); + if (err) + put_device(&dimm->dev); - edac_dbg(0, "creating rank/dimm device %s\n", dev_name(&dimm->dev)); + edac_dbg(0, "created rank/dimm device %s\n", dev_name(&dimm->dev)); return err; } @@ -934,6 +937,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); goto out; } -- 2.21.0 -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] EDAC/sysfs: Drop device references properly 2019-05-08 11:06 ` [PATCH 2/2] EDAC/sysfs: Drop device references properly Borislav Petkov @ 2019-05-08 12:47 ` Greg KH 0 siblings, 0 replies; 11+ messages in thread From: Greg KH @ 2019-05-08 12:47 UTC (permalink / raw) To: Borislav Petkov Cc: PanBian, Mauro Carvalho Chehab, James Morse, linux-edac, linux-kernel On Wed, May 08, 2019 at 01:06:05PM +0200, Borislav Petkov wrote: > -- > From: Greg KH <gregkh@linuxfoundation.org> > > Do put_device() if device_add() fails. > > [ bp: do device_del() for the successfully created devices in > edac_create_csrow_objects(), on the unwind path. ] Yes, good catch, looks good, thanks! greg k-h ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-05-08 18:50 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-18 2:27 EDAC: Fix memory leak in creating CSROW object Pan Bian 2019-04-18 17:25 ` Borislav Petkov [not found] ` <20190419003536.GA57795@bianpan2016@163.com> 2019-04-19 0:45 ` Borislav Petkov 2019-04-27 21:49 ` Greg KH 2019-05-08 10:57 ` Borislav Petkov 2019-05-08 12:47 ` Greg KH 2019-05-08 18:50 ` Borislav Petkov 2019-05-08 11:02 ` [PATCH 1/2] EDAC/sysfs: Fix memory leak when creating a csrow object Borislav Petkov 2019-05-08 12:45 ` Greg KH 2019-05-08 11:06 ` [PATCH 2/2] EDAC/sysfs: Drop device references properly Borislav Petkov 2019-05-08 12:47 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).