check_freeing_devm: Also track erroneous usage of kfree when the pointer has been re-assigned
RFC PATCH 1/1 check_freeing_devm: Also track erroneous usage of kfree when the pointer has been re-assigned
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.
---
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(+)
check_freeing_devm.c +25 -0
@@ -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 ); }
Signature · 1 line
2.34.1
Dan Carpenter
AppliedQuoted 12 lines · 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.
Quoted 6 lines
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 <[email protected]>
Thanks! Applied.
Quoted 4 lines
---
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.
Quoted 3 lines
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
Le 26/05/2022 à 11:51, Dan Carpenter a écrit :
Quoted 4 lines
Quoted 3 lines
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.
Quoted 1 line
I have been trying to speed things up on my system these past two weeks.
:)
CJ
Quoted 4 lines
regards,
dan carpenter
Quoted 8 lines · Christophe JAILLET
Le 26/05/2022 à 11:51, Dan Carpenter a écrit :
Quoted 4 lines
Quoted 3 lines
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
Le 26/05/2022 à 13:07, Dan Carpenter a écrit :
Quoted 9 lines · Christophe JAILLET
Quoted 5 lines
Le 26/05/2022 à 11:51, Dan Carpenter a écrit :
Quoted 3 lines
Quoted 2 lines
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.shscript does a make -j${NR_CPU}.
Yep.
I've also tweaked smatch_scripts/kchecker which doesn't have any -j, IIUC.
CJ
Quoted 3 lines
regards,
dan carpenter
Quoted 7 lines · Marion & Christophe JAILLET
Le 26/05/2022 à 13:07, Dan Carpenter a écrit :
Quoted 5 lines · Christophe JAILLET
Quoted 4 lines
Le 26/05/2022 à 11:51, Dan Carpenter a écrit :
Quoted 3 lines
Quoted 2 lines
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?
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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.shscript 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
Le 26/05/2022 à 14:00, Dan Carpenter a écrit :
Quoted 11 lines · Marion & Christophe JAILLET
Quoted 10 lines
Le 26/05/2022 à 13:07, Dan Carpenter a écrit :
Quoted 9 lines · Christophe JAILLET
Quoted 5 lines
Le 26/05/2022 à 11:51, Dan Carpenter a écrit :
Quoted 3 lines
Quoted 2 lines
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.shscript does a make -j${NR_CPU}.
Quoted 6 lines
Quoted 3 lines
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
Quoted 3 lines
regards,
dan carpenter
Quoted 20 lines · Christophe JAILLET
Le 26/05/2022 à 14:00, Dan Carpenter a écrit :
Quoted 11 lines · Marion & Christophe JAILLET
Quoted 10 lines
Le 26/05/2022 à 13:07, Dan Carpenter a écrit :
Quoted 9 lines · Christophe JAILLET
Quoted 5 lines
Le 26/05/2022 à 11:51, Dan Carpenter a écrit :
Quoted 3 lines
Quoted 2 lines
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.shscript does a make -j${NR_CPU}.Quoted 6 lines
Quoted 3 lines
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.
Quoted 8 lines
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
Le 26/05/2022 à 15:49, Dan Carpenter a écrit :
Quoted 25 lines · Christophe JAILLET
Quoted 24 lines
Le 26/05/2022 à 14:00, Dan Carpenter a écrit :
Quoted 11 lines · Marion & Christophe JAILLET
Quoted 10 lines
Le 26/05/2022 à 13:07, Dan Carpenter a écrit :
Quoted 9 lines · Christophe JAILLET
Quoted 5 lines
Le 26/05/2022 à 11:51, Dan Carpenter a écrit :
Quoted 3 lines
Quoted 2 lines
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.shscript does a make -j${NR_CPU}.Quoted 6 lines
Quoted 3 lines
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/
Quoted 8 lines
Quoted 2 lines
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
Quoted 4 lines
regards,
dan carpenter