[PATCH v1] console: fix an issue about ioremap leak.

[PATCH v1] console: fix an issue about ioremap leak.

[Dejin Zheng]

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.

Fixes: e84de0c6190503 ("MIPS: GIO bus support for SGI IP22/28")
CC: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
 drivers/video/console/newport_con.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/video/console/newport_con.c b/drivers/video/console/newport_con.c
index 00dddf6e08b0..6bfc8e3ffd4a 100644
--- a/drivers/video/console/newport_con.c
+++ b/drivers/video/console/newport_con.c
@@ -720,6 +720,9 @@ 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);
 	return err;
 }
 
-- 
2.25.0

Re: [PATCH v1] console: fix an issue about ioremap leak.

[Markus Elfring]

> 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.

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

   Subject:
   [PATCH v2] console: Complete exception handling in newport_probe()

   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 the function “iounmap” together with the check
   of a failure predicate.


I would like to point out that there is a script for the semantic
patch language which would detect other questionable source code.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/scripts/coccinelle/free/iounmap.cocci

How do you think about to extend presented software analysis approaches?

Regards,
Markus

Re: [PATCH v1] console: fix an issue about ioremap leak.

[Dejin Zheng]

On Thu, Apr 23, 2020 at 01:10:14PM +0200, Markus Elfring wrote:
> > 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.
> 
> How do you think about a wording variant like the following?
> 
>    Subject:
>    [PATCH v2] console: Complete exception handling in newport_probe()
> 
>    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 the function “iounmap” together with the check
>    of a failure predicate.
>
Thanks!

> 
> I would like to point out that there is a script for the semantic
> patch language which would detect other questionable source code.
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/scripts/coccinelle/free/iounmap.cocci
> 
> How do you think about to extend presented software analysis approaches?
>
Sorry, I am not familiar with it, I don't know.

BR,
Dejin

> Regards,
> Markus

Re: console: Complete exception handling in newport_probe()

[Markus Elfring]

>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/scripts/coccinelle/free/iounmap.cocci
>>
>> How do you think about to extend presented software analysis approaches?
>>
> Sorry, I am not familiar with it, I don't know.

Do you find the comments helpful at the beginning of this SmPL script?

Would you like to let any more source code analysis tools help you
to find remaining update candidates?

Regards,
Markus

Re: console: Complete exception handling in newport_probe()

[Dejin Zheng]

On Thu, Apr 23, 2020 at 05:23:29PM +0200, Markus Elfring wrote:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/scripts/coccinelle/free/iounmap.cocci
> >>
> >> How do you think about to extend presented software analysis approaches?
> >>
> > Sorry, I am not familiar with it, I don't know.
> 
> Do you find the comments helpful at the beginning of this SmPL script?
>
Sorry, I do not know how to use the SmPL script. 

> Would you like to let any more source code analysis tools help you
> to find remaining update candidates?
>
yes, but I think the source code analysis tools only can found the simple
repetitive issue. and need spend some time learning to use it. at different
stages, it should have different methods for me. now, I think the best for
me may be that read and check the source code. Thanks!

BR,
Dejin

> Regards,
> Markus

Re: console: Complete exception handling in newport_probe()

[Markus Elfring]

>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/scripts/coccinelle/free/iounmap.cocci
…
> Sorry, I do not know how to use the SmPL script.

I proposed to take another look at the header of such a file.
I imagine that provided information can trigger further development ideas
for the continuation of a constructive patch review.


>> Would you like to let any more source code analysis tools help you
>> to find remaining update candidates?
>>
> yes, but I think the source code analysis tools only can found the simple
> repetitive issue.

I suggest to reconsider this view.


> and need spend some time learning to use it.

This approach can be usual.


> at different stages, it should have different methods for me.

I hope so.


> now, I think the best for me may be that read and check the source code.

This can be also usual for a while.

I am curious how much special tools can make your software developments
more convenient and safe over time.

Regards,
Markus

Re: console: Complete exception handling in newport_probe()

[Greg Kroah-Hartman]

On Fri, Apr 24, 2020 at 01:02:45AM +0800, Dejin Zheng wrote:
> On Thu, Apr 23, 2020 at 05:23:29PM +0200, Markus Elfring wrote:
> > >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/scripts/coccinelle/free/iounmap.cocci
> > >>
> > >> How do you think about to extend presented software analysis approaches?
> > >>
> > > Sorry, I am not familiar with it, I don't know.
> > 
> > Do you find the comments helpful at the beginning of this SmPL script?
> >
> Sorry, I do not know how to use the SmPL script. 

<snip>

Please note you are responding to someone who many kernel maintainers,
myself included, have on their blacklist as they are totally unhelpful.
Please feel free to ignore them like the rest of us do.

greg k-h

Re: console: Complete exception handling in newport_probe()

[Markus Elfring]

> Please note you are responding to someone who many kernel maintainers,
> myself included, have on their blacklist

You configured your communication filters for some reasons in this way.


> as they are totally unhelpful.

The development views can vary also around my software contributions.
It seems to happen often enough that specific suggestions get accepted
from my selection of change possibilities.


> Please feel free to ignore them like the rest of us do.

This is generally also possible.

But I hope still that our discussions can become more constructive again.

Regards,
Markus

Re: console: Complete exception handling in newport_probe()

[Markus Elfring]

> Sorry, I do not know how to use the SmPL script.

I would like to try again to make you more familiar with applications
of the Coccinelle software.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/dev-tools/coccinelle.rst?id=b4f633221f0aeac102e463a4be46a643b2e3b819#n9

Another small example according to a script for the semantic patch language:
@display@
expression e, x;
statement is, es;
type t;
@@
*e = (t) ioremap(...)
 ... when != if( \( !e \| e == NULL \| unlikely( \( !e \| e == NULL \) ) \| WARN_ON( \( !e \| e == NULL \) ) \) ) is else es
     when != iounmap(e)
 return x;



>> Would you like to let any more source code analysis tools help you
>> to find remaining update candidates?
>>
> yes, but I think the source code analysis tools only can found the simple
> repetitive issue.

The shown analysis approach can point 127 source files out from
the software “Linux next-20200423”. These files contain function implementations
which might need further development considerations.


> and need spend some time learning to use it.

You came along a few files where you noticed questionable source code.
Would you become interested to find similar places for another check?


elfring@Sonne:~/Projekte/Linux/next-patched> XX=$(date) && spatch --no-loops --timeout 12 -j 4 --chunksize 1 -dir ~/Projekte/Linux/next-patched ~/Projekte/Coccinelle/janitor/show_questionable_ioremap_usage3.cocci > ~/Projekte/Bau/Linux/scripts/Coccinelle/ioremap/20200423/show_questionable_ioremap_usage3.diff 2> ~/Projekte/Bau/Linux/scripts/Coccinelle/ioremap/20200423/show_questionable_ioremap_usage3-errors.txt; YY=$(date) && echo "$XX | $YY"
Fr 24. Apr 09:08:44 CEST 2020 | Fr 24. Apr 09:10:37 CEST 2020

Would you like to extend your learning experiences accordingly?


> now, I think the best for me may be that read and check the source code.

How do you think about to reduce efforts for manual code inspection?

Regards,
Markus