We should free p after con_release_unimap(p) like the call points of
con_release_unimap() do in the same file.
This patch adds the missing kfree() after con_release_unimap(p).
Signed-off-by: Jianglei Nie <[email protected]>
---
drivers/tty/vt/consolemap.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index d815ac98b39e..5279c3d27720 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -520,6 +520,7 @@ static int con_do_clear_unimap(struct vc_data *vc)
p->refcount++;
p->sum = 0;
con_release_unimap(p);
+ kfree(p);
}
return 0;
}
--
2.25.1
On Thu, Mar 03, 2022 at 10:06:30AM +0800, Jianglei Nie wrote:
> We should free p after con_release_unimap(p) like the call points of
> con_release_unimap() do in the same file.
>
> This patch adds the missing kfree() after con_release_unimap(p).
>
> Signed-off-by: Jianglei Nie <[email protected]>
> ---
> drivers/tty/vt/consolemap.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> index d815ac98b39e..5279c3d27720 100644
> --- a/drivers/tty/vt/consolemap.c
> +++ b/drivers/tty/vt/consolemap.c
> @@ -520,6 +520,7 @@ static int con_do_clear_unimap(struct vc_data *vc)
> p->refcount++;
> p->sum = 0;
> con_release_unimap(p);
> + kfree(p);
> }
> return 0;
> }
> --
> 2.25.1
>
How did you test this code?
On Wed, Mar 09, 2022 at 08:34:47PM +0800, 聂江磊 wrote:
<snip>
Please respond not in html email so it gets to the mailing list, and try
not to top-post. I'll be glad to respond that way.
thanks,
greg k-h
A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
A: No.
Q: Should I include quotations after my reply?
http://daringfireball.net/2007/07/on_top
On Wed, Mar 09, 2022 at 08:34:47PM +0800, 聂江磊 wrote:
> I found this bug by using clang static analyse checkers. I found that function con_release_unimap() is only called in this file(drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c). There are totally 5 times that con_release_unimap() is called
>
> (line 430, 466, 522, 599, 673) while con_release_unimap() is not followed by kfree() only in line 522. So I think it is a bug
>
> and make this patch.
Given that we do not have any reports of this leaking memory, I do not
think your analysis is correct, so I am loath to apply this, sorry.
greg k-h
Hi,
On 09. 03. 22, 13:34, 聂江磊 wrote:
> I found this bug by using clang static analyse checkers. I found that
> function con_release_unimap() is only called in this
> file(drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c). There
> are totally 5 times that con_release_unimap() is called
> (line 430, 466, 522, 599, 673) while con_release_unimap() is not
> followed by kfree() only in line 522. So I think it is a bug
> and make this patch.
>
>
> At 2022-03-03 10:06:30, "Jianglei Nie" <[email protected]> wrote:
>>We should free p after con_release_unimap(p) like the call points of
>>con_release_unimap() do in the same file.
But this one does not free it on purpose, right? See below.
>>This patch adds the missing kfree() after con_release_unimap(p).
>>
>>Signed-off-by: Jianglei Nie <[email protected]>
>>---
>> drivers/tty/vt/consolemap.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>>diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
>>index d815ac98b39e..5279c3d27720 100644
>>--- a/drivers/tty/vt/consolemap.c
>>+++ b/drivers/tty/vt/consolemap.c
>>@@ -520,6 +520,7 @@ static int con_do_clear_unimap(struct vc_data *vc)
>> p->refcount++;
>> p->sum = 0;
>> con_release_unimap(p);
>>+ kfree(p);
You've just broken con_set_unimap(), or do I miss something?
>> }
>> return 0;
>> }
--
js
suse labs
On 25. 04. 22, 8:59, Jiri Slaby wrote:
> Hi,
>
> On 09. 03. 22, 13:34, 聂江磊 wrote:
>> I found this bug by using clang static analyse checkers. I found that
>> function con_release_unimap() is only called in this
>> file(drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c). There
>> are totally 5 times that con_release_unimap() is called
>> (line 430, 466, 522, 599, 673) while con_release_unimap() is not
>> followed by kfree() only in line 522. So I think it is a bug
>> and make this patch.
>>
>>
>> At 2022-03-03 10:06:30, "Jianglei Nie" <[email protected]> wrote:
>>> We should free p after con_release_unimap(p) like the call points of
>>> con_release_unimap() do in the same file.
>
> But this one does not free it on purpose, right? See below.
>
>>> This patch adds the missing kfree() after con_release_unimap(p).
>>>
>>> Signed-off-by: Jianglei Nie <[email protected]>
>>> ---
>>> drivers/tty/vt/consolemap.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
>>> index d815ac98b39e..5279c3d27720 100644
>>> --- a/drivers/tty/vt/consolemap.c
>>> +++ b/drivers/tty/vt/consolemap.c
>>> @@ -520,6 +520,7 @@ static int con_do_clear_unimap(struct vc_data *vc)
>>> p->refcount++;
>>> p->sum = 0;
>>> con_release_unimap(p);
>>> + kfree(p);
>
> You've just broken con_set_unimap(), or do I miss something?
No, you did not. The interface is terrible and deserves cleanup.
I found this, likely related, syzkaller report in my INBOX:
https://lore.kernel.org/all/[email protected]/
Care to test the reproducer both with and without your change? Does your
patch fixes the issue. And if it does, could you add this to your patch:
Reported-by: [email protected]
? So that syzbot verifies the patch.
Once you do all this, I will re-review the patch and the code. The code
is really very hard to follow, so I cannot decide whether your patch is
correct or not ATM.
And provided the above, I put a note to my TODO list to restructure the
code, so that people know what's going on there.
thanks,
--
js