[PATCH v3] console: newport_con: fix an issue about leak related system resources

[PATCH v3] console: newport_con: fix an issue about leak related system resources

[Dejin Zheng]

A call of the function do_take_over_console() can fail here.
The corresponding system resources were not released then.
Thus add a call of iounmap() and release_mem_region()
together with the check of a failure predicate. and also
add release_mem_region() on device removal.

Fixes: e86bb8acc0fdc ("[PATCH] VT binding: Make newport_con support binding")
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Suggested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
v2 -> v3:
	- modify commit tag CC to Cc by Andy's suggestion.
	- modify Subject 'console: console:' to 'console: newport_con:'
	  by Bartlomiej's suggestion.
	- add missing release_mem_region() on error and on device removal
	  by Bartlomiej's suggestion.
	- add correct fixes commit, before this patch, add a wrong 'Fixes:
	  e84de0c6190503 ("MIPS: GIO bus support for SGI IP22/28")'
	  thanks Bartlomiej again!

v1 -> v2:
	- modify the commit comments. The commit comments have some more
	  appropriate instructions by Markus'suggestion. here is my first
	  version commit comments:

	  if do_take_over_console() return an error in the newport_probe(),
	  due to the io virtual address is not released, it will cause a
	  leak.
	 
 drivers/video/console/newport_con.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/video/console/newport_con.c b/drivers/video/console/newport_con.c
index 00dddf6e08b0..2d2ee17052e8 100644
--- a/drivers/video/console/newport_con.c
+++ b/drivers/video/console/newport_con.c
@@ -32,6 +32,8 @@
 #include <linux/linux_logo.h>
 #include <linux/font.h>
 
+#define NEWPORT_LEN	0x10000
+
 #define FONT_DATA ((unsigned char *)font_vga_8x16.data)
 
 /* borrowed from fbcon.c */
@@ -43,6 +45,7 @@
 static unsigned char *font_data[MAX_NR_CONSOLES];
 
 static struct newport_regs *npregs;
+static unsigned long newport_addr;
 
 static int logo_active;
 static int topscan;
@@ -702,7 +705,6 @@ const struct consw newport_con = {
 static int newport_probe(struct gio_device *dev,
 			 const struct gio_device_id *id)
 {
-	unsigned long newport_addr;
 	int err;
 
 	if (!dev->resource.start)
@@ -712,7 +714,7 @@ static int newport_probe(struct gio_device *dev,
 		return -EBUSY; /* we only support one Newport as console */
 
 	newport_addr = dev->resource.start + 0xF0000;
-	if (!request_mem_region(newport_addr, 0x10000, "Newport"))
+	if (!request_mem_region(newport_addr, NEWPORT_LEN, "Newport"))
 		return -ENODEV;
 
 	npregs = (struct newport_regs *)/* ioremap cannot fail */
@@ -720,6 +722,11 @@ static int newport_probe(struct gio_device *dev,
 	console_lock();
 	err = do_take_over_console(&newport_con, 0, MAX_NR_CONSOLES - 1, 1);
 	console_unlock();
+
+	if (err) {
+		iounmap((void *)npregs);
+		release_mem_region(newport_addr, NEWPORT_LEN);
+	}
 	return err;
 }
 
@@ -727,6 +734,7 @@ static void newport_remove(struct gio_device *dev)
 {
 	give_up_console(&newport_con);
 	iounmap((void *)npregs);
+	release_mem_region(newport_addr, NEWPORT_LEN);
 }
 
 static struct gio_device_id newport_ids[] = {
-- 
2.25.0

Re: [PATCH v3] console: newport_con: fix an issue about leak related system resources

[Andy Shevchenko]

On Thu, Apr 23, 2020 at 7:42 PM Dejin Zheng <zhengdejin5@gmail.com> wrote:
>
> A call of the function do_take_over_console() can fail here.
> The corresponding system resources were not released then.
> Thus add a call of iounmap() and release_mem_region()
> together with the check of a failure predicate. and also
> add release_mem_region() on device removal.

FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Fixes: e86bb8acc0fdc ("[PATCH] VT binding: Make newport_con support binding")
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Suggested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> ---
> v2 -> v3:
>         - modify commit tag CC to Cc by Andy's suggestion.
>         - modify Subject 'console: console:' to 'console: newport_con:'
>           by Bartlomiej's suggestion.
>         - add missing release_mem_region() on error and on device removal
>           by Bartlomiej's suggestion.
>         - add correct fixes commit, before this patch, add a wrong 'Fixes:
>           e84de0c6190503 ("MIPS: GIO bus support for SGI IP22/28")'
>           thanks Bartlomiej again!
>
> v1 -> v2:
>         - modify the commit comments. The commit comments have some more
>           appropriate instructions by Markus'suggestion. here is my first
>           version commit comments:
>
>           if do_take_over_console() return an error in the newport_probe(),
>           due to the io virtual address is not released, it will cause a
>           leak.
>
>  drivers/video/console/newport_con.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/console/newport_con.c b/drivers/video/console/newport_con.c
> index 00dddf6e08b0..2d2ee17052e8 100644
> --- a/drivers/video/console/newport_con.c
> +++ b/drivers/video/console/newport_con.c
> @@ -32,6 +32,8 @@
>  #include <linux/linux_logo.h>
>  #include <linux/font.h>
>
> +#define NEWPORT_LEN    0x10000
> +
>  #define FONT_DATA ((unsigned char *)font_vga_8x16.data)
>
>  /* borrowed from fbcon.c */
> @@ -43,6 +45,7 @@
>  static unsigned char *font_data[MAX_NR_CONSOLES];
>
>  static struct newport_regs *npregs;
> +static unsigned long newport_addr;
>
>  static int logo_active;
>  static int topscan;
> @@ -702,7 +705,6 @@ const struct consw newport_con = {
>  static int newport_probe(struct gio_device *dev,
>                          const struct gio_device_id *id)
>  {
> -       unsigned long newport_addr;
>         int err;
>
>         if (!dev->resource.start)
> @@ -712,7 +714,7 @@ static int newport_probe(struct gio_device *dev,
>                 return -EBUSY; /* we only support one Newport as console */
>
>         newport_addr = dev->resource.start + 0xF0000;
> -       if (!request_mem_region(newport_addr, 0x10000, "Newport"))
> +       if (!request_mem_region(newport_addr, NEWPORT_LEN, "Newport"))
>                 return -ENODEV;
>
>         npregs = (struct newport_regs *)/* ioremap cannot fail */
> @@ -720,6 +722,11 @@ static int newport_probe(struct gio_device *dev,
>         console_lock();
>         err = do_take_over_console(&newport_con, 0, MAX_NR_CONSOLES - 1, 1);
>         console_unlock();
> +
> +       if (err) {
> +               iounmap((void *)npregs);
> +               release_mem_region(newport_addr, NEWPORT_LEN);
> +       }
>         return err;
>  }
>
> @@ -727,6 +734,7 @@ static void newport_remove(struct gio_device *dev)
>  {
>         give_up_console(&newport_con);
>         iounmap((void *)npregs);
> +       release_mem_region(newport_addr, NEWPORT_LEN);
>  }
>
>  static struct gio_device_id newport_ids[] = {
> --
> 2.25.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] console: newport_con: fix an issue about leak related system resources

[Bartlomiej Zolnierkiewicz]

On 4/23/20 6:42 PM, Dejin Zheng wrote:
> A call of the function do_take_over_console() can fail here.
> The corresponding system resources were not released then.
> Thus add a call of iounmap() and release_mem_region()
> together with the check of a failure predicate. and also
> add release_mem_region() on device removal.
> 
> Fixes: e86bb8acc0fdc ("[PATCH] VT binding: Make newport_con support binding")
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Suggested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>

Applied to drm-misc-next tree (patch should show up in v5.9), thanks.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> ---
> v2 -> v3:
> 	- modify commit tag CC to Cc by Andy's suggestion.
> 	- modify Subject 'console: console:' to 'console: newport_con:'
> 	  by Bartlomiej's suggestion.
> 	- add missing release_mem_region() on error and on device removal
> 	  by Bartlomiej's suggestion.
> 	- add correct fixes commit, before this patch, add a wrong 'Fixes:
> 	  e84de0c6190503 ("MIPS: GIO bus support for SGI IP22/28")'
> 	  thanks Bartlomiej again!
> 
> v1 -> v2:
> 	- modify the commit comments. The commit comments have some more
> 	  appropriate instructions by Markus'suggestion. here is my first
> 	  version commit comments:
> 
> 	  if do_take_over_console() return an error in the newport_probe(),
> 	  due to the io virtual address is not released, it will cause a
> 	  leak.
> 	 
>  drivers/video/console/newport_con.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/console/newport_con.c b/drivers/video/console/newport_con.c
> index 00dddf6e08b0..2d2ee17052e8 100644
> --- a/drivers/video/console/newport_con.c
> +++ b/drivers/video/console/newport_con.c
> @@ -32,6 +32,8 @@
>  #include <linux/linux_logo.h>
>  #include <linux/font.h>
>  
> +#define NEWPORT_LEN	0x10000
> +
>  #define FONT_DATA ((unsigned char *)font_vga_8x16.data)
>  
>  /* borrowed from fbcon.c */
> @@ -43,6 +45,7 @@
>  static unsigned char *font_data[MAX_NR_CONSOLES];
>  
>  static struct newport_regs *npregs;
> +static unsigned long newport_addr;
>  
>  static int logo_active;
>  static int topscan;
> @@ -702,7 +705,6 @@ const struct consw newport_con = {
>  static int newport_probe(struct gio_device *dev,
>  			 const struct gio_device_id *id)
>  {
> -	unsigned long newport_addr;
>  	int err;
>  
>  	if (!dev->resource.start)
> @@ -712,7 +714,7 @@ static int newport_probe(struct gio_device *dev,
>  		return -EBUSY; /* we only support one Newport as console */
>  
>  	newport_addr = dev->resource.start + 0xF0000;
> -	if (!request_mem_region(newport_addr, 0x10000, "Newport"))
> +	if (!request_mem_region(newport_addr, NEWPORT_LEN, "Newport"))
>  		return -ENODEV;
>  
>  	npregs = (struct newport_regs *)/* ioremap cannot fail */
> @@ -720,6 +722,11 @@ static int newport_probe(struct gio_device *dev,
>  	console_lock();
>  	err = do_take_over_console(&newport_con, 0, MAX_NR_CONSOLES - 1, 1);
>  	console_unlock();
> +
> +	if (err) {
> +		iounmap((void *)npregs);
> +		release_mem_region(newport_addr, NEWPORT_LEN);
> +	}
>  	return err;
>  }
>  
> @@ -727,6 +734,7 @@ static void newport_remove(struct gio_device *dev)
>  {
>  	give_up_console(&newport_con);
>  	iounmap((void *)npregs);
> +	release_mem_region(newport_addr, NEWPORT_LEN);
>  }
>  
>  static struct gio_device_id newport_ids[] = {
> 

Re: [PATCH v3] console: newport_con: fix an issue about leak related system resources

[Dejin Zheng]

On Fri, Apr 24, 2020 at 06:54:41PM +0200, Markus Elfring wrote:
> > The corresponding system resources were not released then.
> 
> How do you think about a wording variant like the following?
>
Markus, I think my commit comments is a sufficiently clear description
for this patch. Someone has told me not to send commit comments again
and again when it is enough clear. Because it only wastes the precious
time of the maintainer and very very little help for patch improvement.

BTW, In the past week, you asked me to change the commit comments in my
6 patches like this one. Let me return to the essence of patch, point
out the code problems and better solutions will be more popular.

>    Subject:
>    [PATCH v4] console: newport_con: Fix incomplete releasing of system resources
> 
>    Change description:
>    * A call of the function do_take_over_console() can fail here.
>      The corresponding system resources were not released then.
>      Thus add a call of iounmap() and release_mem_region()
>      together with the check of a failure predicate.
> 
>    * Add also a call of release_mem_region() for the completion
>      of resource clean-up on device removal.
> 
> 
> It can be nicer if all patch reviewers (including me) will be explicitly specified
> as recipients for such messages, can't it?
> 
> Regards,
> Markus

Re: [PATCH v3] console: newport_con: fix an issue about leak related system resources

[Markus Elfring]

> The corresponding system resources were not released then.

How do you think about a wording variant like the following?

   Subject:
   [PATCH v4] console: newport_con: Fix incomplete releasing of system resources

   Change description:
   * A call of the function do_take_over_console() can fail here.
     The corresponding system resources were not released then.
     Thus add a call of iounmap() and release_mem_region()
     together with the check of a failure predicate.

   * Add also a call of release_mem_region() for the completion
     of resource clean-up on device removal.


It can be nicer if all patch reviewers (including me) will be explicitly specified
as recipients for such messages, can't it?

Regards,
Markus