[RFC PATCH] check_freeing_devm: Also track erroneous usage of kfree when the pointer has been re-assigned

[RFC PATCH] check_freeing_devm: Also track erroneous usage of kfree when the pointer has been re-assigned

[Christophe JAILLET]

When some memory is allocated, a common pattern is to use a tmp variable
to check if the allocation has succeeded or not. This tmp variable is then
stored in another variable for future use.

   ptr = devm_kmalloc(...);
   if (!ptr)
      return -ENOMEM;
   another_val = ptr;

So trying to see if this 'alias' is incorrectly freed with kfree makes
sense.

To do that, add a new hook that check, when an assignment is detected if
the right part of the expression is already recorded.
If so, also record the right part, so that bogus kfree can check both
reference.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This is my first 'real' code proposal for smatch.
It is likely to be wrong/incomplete/imperfect. Maybe this functionality
is already implemented somewhere else and I just try to duplicate it.

All I know is that it seams to work for me, even if it has not detected any
issue yet :) (compiling takes SO MUCH time on my machine)
---
 check_freeing_devm.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/check_freeing_devm.c b/check_freeing_devm.c
index 11e8d8bc30b6..51fa990f1b4a 100644
--- a/check_freeing_devm.c
+++ b/check_freeing_devm.c
@@ -26,6 +26,29 @@ static void match_assign(const char *fn, struct expression *expr, void *unused)
 	set_state_expr(my_id, expr->left, &devm);
 }
 
+/*
+ * This hook deals with things like:
+ * ptr = devm_kmalloc(...);
+ * if (!ptr)
+ * 	return -ENOMEM;
+ * another_val = ptr;			<==
+ */
+static void match_reassign(struct expression *expr)
+{
+	struct expression *left, *right;
+
+	if (expr->op != '=')
+		return;
+
+	right = strip_expr(expr->right);
+	if (!get_state_expr(my_id, right))
+		return;
+
+	left = strip_expr(expr->left);
+
+	set_state_expr(my_id, left, &devm);
+}
+
 static void match_free_func(const char *fn, struct expression *expr, void *_arg)
 {
 	struct expression *arg_expr;
@@ -86,4 +109,6 @@ void check_freeing_devm(int id)
 	add_function_hook("kfree", &match_free_func, INT_PTR(0));
 	add_function_hook("krealloc", &match_free_func, INT_PTR(0));
 	register_funcs_from_file();
+
+	add_hook(&match_reassign, ASSIGNMENT_HOOK);
 }
-- 
2.34.1

Re: [RFC PATCH] check_freeing_devm: Also track erroneous usage of kfree when the pointer has been re-assigned

[Dan Carpenter]

On Thu, May 26, 2022 at 09:15:16AM +0200, Christophe JAILLET wrote:
> When some memory is allocated, a common pattern is to use a tmp variable
> to check if the allocation has succeeded or not. This tmp variable is then
> stored in another variable for future use.
> 
>    ptr = devm_kmalloc(...);
>    if (!ptr)
>       return -ENOMEM;
>    another_val = ptr;
> 
> So trying to see if this 'alias' is incorrectly freed with kfree makes
> sense.
> 
> To do that, add a new hook that check, when an assignment is detected if
> the right part of the expression is already recorded.
> If so, also record the right part, so that bogus kfree can check both
> reference.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Thanks! Applied.

> ---
> This is my first 'real' code proposal for smatch.
> It is likely to be wrong/incomplete/imperfect. Maybe this functionality
> is already implemented somewhere else and I just try to duplicate it.

No, it's fine.  There are a lot of different ways to write any check.
I wrote this check nine years ago.  If I were to write it now, I probably
would only hook into the free functions and not have any states.

	orig = get_assigned_expr_recurse();
	if (!orig || orig->type != EXPR_CALL)
		return;

	str = expr_to_str(orig);
	if (!str)
		return;
	if (strstr(str, "devm_"))
		sm_warning("devm_ data freed");
	free_string(str);

But, whatever, the check works fine as-is.  It's not like I have run out
of other work to do.

The other idea that I have, is to look at struct member types.  Make a
list of "(struct foo)->bar" was allocated with devm_ so that type cannot
be freed using kfree() within the same file.  It's not tied to the
variables at all, only to the struct member names.

That check would print many of the same warnings as this check but it's
fine to print duplicate warnings since the heuristic is different.

> 
> All I know is that it seams to work for me, even if it has not detected any
> issue yet :) (compiling takes SO MUCH time on my machine)

Yeah. :/  How big is your smatch_db.sqlite file?

I have been trying to speed things up on my system these past two weeks.

regards,
dan carpenter

Re: [RFC PATCH] check_freeing_devm: Also track erroneous usage of kfree when the pointer has been re-assigned

[Christophe JAILLET]

Le 26/05/2022 à 11:51, Dan Carpenter a écrit :
>
>> All I know is that it seams to work for me, even if it has not detected any
>> issue yet :) (compiling takes SO MUCH time on my machine)
> Yeah. :/  How big is your smatch_db.sqlite file?

I don't use any up-to-now. I just have a limited and basic usage of smatch.
In fact I'm just starting to learn how it works and how I could use it 
to implement my own checks.
And while at it, I send patches about small things I spot here and there.

> I have been trying to speed things up on my system these past two weeks.

:)

CJ

>
> regards,
> dan carpenter
>

Re: [RFC PATCH] check_freeing_devm: Also track erroneous usage of kfree when the pointer has been re-assigned

[Dan Carpenter]

On Thu, May 26, 2022 at 12:30:05PM +0200, Christophe JAILLET wrote:
> 
> Le 26/05/2022 à 11:51, Dan Carpenter a écrit :
> > 
> > > All I know is that it seams to work for me, even if it has not detected any
> > > issue yet :) (compiling takes SO MUCH time on my machine)
> > Yeah. :/  How big is your smatch_db.sqlite file?
> 
> I don't use any up-to-now.

Basically all my changes are targetted at making the smatch_db.sqlite
file smaller so they won't help you.  Are you at least doing a parallel
build?  The smatch_scripts/test_kernel.sh script does a make -j${NR_CPU}.

regards,
dan carpenter

Re: [RFC PATCH] check_freeing_devm: Also track erroneous usage of kfree when the pointer has been re-assigned

[Marion & Christophe JAILLET]

Le 26/05/2022 à 13:07, Dan Carpenter a écrit :
> On Thu, May 26, 2022 at 12:30:05PM +0200, Christophe JAILLET wrote:
>> Le 26/05/2022 à 11:51, Dan Carpenter a écrit :
>>>> All I know is that it seams to work for me, even if it has not detected any
>>>> issue yet :) (compiling takes SO MUCH time on my machine)
>>> Yeah. :/  How big is your smatch_db.sqlite file?
>> I don't use any up-to-now.
> Basically all my changes are targetted at making the smatch_db.sqlite
> file smaller so they won't help you.  Are you at least doing a parallel
> build?  The smatch_scripts/test_kernel.sh script does a make -j${NR_CPU}.

Yep.

I've also tweaked smatch_scripts/kchecker which doesn't have any -j, IIUC.

CJ


> regards,
> dan carpenter
>

Re: [RFC PATCH] check_freeing_devm: Also track erroneous usage of kfree when the pointer has been re-assigned

[Dan Carpenter]

On Thu, May 26, 2022 at 01:40:51PM +0200, Marion & Christophe JAILLET wrote:
> 
> Le 26/05/2022 à 13:07, Dan Carpenter a écrit :
> > On Thu, May 26, 2022 at 12:30:05PM +0200, Christophe JAILLET wrote:
> > > Le 26/05/2022 à 11:51, Dan Carpenter a écrit :
> > > > > All I know is that it seams to work for me, even if it has not detected any
> > > > > issue yet :) (compiling takes SO MUCH time on my machine)
> > > > Yeah. :/  How big is your smatch_db.sqlite file?
> > > I don't use any up-to-now.
> > Basically all my changes are targetted at making the smatch_db.sqlite
> > file smaller so they won't help you.  Are you at least doing a parallel
> > build?  The smatch_scripts/test_kernel.sh script does a make -j${NR_CPU}.
> 
> Yep.
> 
> I've also tweaked smatch_scripts/kchecker which doesn't have any -j, IIUC.

Normally, I only use kchecker on one file...  The problem with doing
parallel builds is that you have to print the warnings to a file instead
of to stdout.

regards,
dan carpenter

Re: [RFC PATCH] check_freeing_devm: Also track erroneous usage of kfree when the pointer has been re-assigned

[Christophe JAILLET]

Le 26/05/2022 à 14:00, Dan Carpenter a écrit :
> On Thu, May 26, 2022 at 01:40:51PM +0200, Marion & Christophe JAILLET wrote:
>> Le 26/05/2022 à 13:07, Dan Carpenter a écrit :
>>> On Thu, May 26, 2022 at 12:30:05PM +0200, Christophe JAILLET wrote:
>>>> Le 26/05/2022 à 11:51, Dan Carpenter a écrit :
>>>>>> All I know is that it seams to work for me, even if it has not detected any
>>>>>> issue yet :) (compiling takes SO MUCH time on my machine)
>>>>> Yeah. :/  How big is your smatch_db.sqlite file?
>>>> I don't use any up-to-now.
>>> Basically all my changes are targetted at making the smatch_db.sqlite
>>> file smaller so they won't help you.  Are you at least doing a parallel
>>> build?  The smatch_scripts/test_kernel.sh script does a make -j${NR_CPU}.
>> Yep.
>>
>> I've also tweaked smatch_scripts/kchecker which doesn't have any -j, IIUC.
> Normally, I only use kchecker on one file...  The problem with doing
> parallel builds is that you have to print the warnings to a file instead
> of to stdout.

smatch.txt:

You can also build a directory like this:

     ~/progs/smatch/devel/smatch_scripts/kchecker drivers/whatever/

and up no now, it is mostly the wayIi use it, :)


CJ


> regards,
> dan carpenter
>

Re: [RFC PATCH] check_freeing_devm: Also track erroneous usage of kfree when the pointer has been re-assigned

[Dan Carpenter]

On Thu, May 26, 2022 at 03:37:58PM +0200, Christophe JAILLET wrote:
> 
> Le 26/05/2022 à 14:00, Dan Carpenter a écrit :
> > On Thu, May 26, 2022 at 01:40:51PM +0200, Marion & Christophe JAILLET wrote:
> > > Le 26/05/2022 à 13:07, Dan Carpenter a écrit :
> > > > On Thu, May 26, 2022 at 12:30:05PM +0200, Christophe JAILLET wrote:
> > > > > Le 26/05/2022 à 11:51, Dan Carpenter a écrit :
> > > > > > > All I know is that it seams to work for me, even if it has not detected any
> > > > > > > issue yet :) (compiling takes SO MUCH time on my machine)
> > > > > > Yeah. :/  How big is your smatch_db.sqlite file?
> > > > > I don't use any up-to-now.
> > > > Basically all my changes are targetted at making the smatch_db.sqlite
> > > > file smaller so they won't help you.  Are you at least doing a parallel
> > > > build?  The smatch_scripts/test_kernel.sh script does a make -j${NR_CPU}.
> > > Yep.
> > > 
> > > I've also tweaked smatch_scripts/kchecker which doesn't have any -j, IIUC.
> > Normally, I only use kchecker on one file...  The problem with doing
> > parallel builds is that you have to print the warnings to a file instead
> > of to stdout.
> 
> smatch.txt:
> 
> You can also build a directory like this:
> 
>     ~/progs/smatch/devel/smatch_scripts/kchecker drivers/whatever/
> 
> and up no now, it is mostly the wayIi use it, :)
> 

I guess -j has no downside if you're building a single file.

The only problem is that if you're building a directory and sending
everything to stdout then the lines can get smooshed together.  Is it
better to just let them smoosh or build the whole directory and then
find -name \*.c.smatch -exec cat \{\} \;?

regards,
dan carpenter

Re: [RFC PATCH] check_freeing_devm: Also track erroneous usage of kfree when the pointer has been re-assigned

[Christophe JAILLET]

Le 26/05/2022 à 15:49, Dan Carpenter a écrit :
> On Thu, May 26, 2022 at 03:37:58PM +0200, Christophe JAILLET wrote:
>> Le 26/05/2022 à 14:00, Dan Carpenter a écrit :
>>> On Thu, May 26, 2022 at 01:40:51PM +0200, Marion & Christophe JAILLET wrote:
>>>> Le 26/05/2022 à 13:07, Dan Carpenter a écrit :
>>>>> On Thu, May 26, 2022 at 12:30:05PM +0200, Christophe JAILLET wrote:
>>>>>> Le 26/05/2022 à 11:51, Dan Carpenter a écrit :
>>>>>>>> All I know is that it seams to work for me, even if it has not detected any
>>>>>>>> issue yet :) (compiling takes SO MUCH time on my machine)
>>>>>>> Yeah. :/  How big is your smatch_db.sqlite file?
>>>>>> I don't use any up-to-now.
>>>>> Basically all my changes are targetted at making the smatch_db.sqlite
>>>>> file smaller so they won't help you.  Are you at least doing a parallel
>>>>> build?  The smatch_scripts/test_kernel.sh script does a make -j${NR_CPU}.
>>>> Yep.
>>>>
>>>> I've also tweaked smatch_scripts/kchecker which doesn't have any -j, IIUC.
>>> Normally, I only use kchecker on one file...  The problem with doing
>>> parallel builds is that you have to print the warnings to a file instead
>>> of to stdout.
>> smatch.txt:
>>
>> You can also build a directory like this:
>>
>>      ~/progs/smatch/devel/smatch_scripts/kchecker drivers/whatever/
>>
>> and up no now, it is mostly the wayIi use it, :)
>>
> I guess -j has no downside if you're building a single file.
>
> The only problem is that if you're building a directory and sending
> everything to stdout then the lines can get smooshed together.  Is it
> better to just let them smoosh or build the whole directory and then
> find -name \*.c.smatch -exec cat \{\} \;?

Not sure to follow you.

I've not seen any problem yet with using:
    ../git_smatch/smatch_scripts/kchecker drivers/net/ > results.txt

CJ


>
> regards,
> dan carpenter
>