alloc_page leaks tracing

alloc_page leaks tracing

[Oleg Drokin]

Hello!

   I am wondering why alloc_page and friends are not considering an allocation function?
   Found a bit of code where there was an obvious alloc_page leak that was not caught that is caught if I change alloc_page to kmalloc.

   And while trying to put the support into smatch I suddenly found the structure changed so much from the previous time I looked at it it’s very non-obvious how to add it.

   I tried adding hooks in check_free_strict.c, check_frees_argument.c, check_leaks.c, smatch_constraints_required.c, smatch_fresh_alloc.c, smatch_parse_call_math.c
  and tried to insert it alongside kmalloc in smatch_scripts/gen_allocation_list.sh and I still cannot make
   it work in the actual kernel code even though a modified testcase from validation/sm_memory.c works.


   Any hint?

   Thanks.

Bye,
   Oleg

Re: alloc_page leaks tracing

[Dan Carpenter]

On Tue, Mar 08, 2022 at 12:28:59PM -0500, Oleg Drokin wrote:
> Hello!
> 
>    I am wondering why alloc_page and friends are not considering an allocation function?
>    Found a bit of code where there was an obvious alloc_page leak that was not caught that is caught if I change alloc_page to kmalloc.
> 
>    And while trying to put the support into smatch I suddenly found the structure changed so much from the previous time I looked at it it’s very non-obvious how to add it.
> 
>    I tried adding hooks in check_free_strict.c, check_frees_argument.c, check_leaks.c, smatch_constraints_required.c, smatch_fresh_alloc.c, smatch_parse_call_math.c
>   and tried to insert it alongside kmalloc in smatch_scripts/gen_allocation_list.sh and I still cannot make
>    it work in the actual kernel code even though a modified testcase from validation/sm_memory.c works.
> 
> 
>    Any hint?

The check_leaks.c function is really limitted in the type of leaks it
looks for.  It has basically no false positives, but misses 90% of bugs.

If you're looking for leaks the right place to add it is probably in
check_unwind.c.

regards,
dan carpenter

diff --git a/check_unwind.c b/check_unwind.c
index 569792ad5a57..7ef040d2ca59 100644
--- a/check_unwind.c
+++ b/check_unwind.c
@@ -92,6 +92,10 @@ static struct ref_func_info func_table[] = {
 
 	{ "ieee80211_alloc_hw", ALLOC,  -1, "$", &valid_ptr_min_sval, &valid_ptr_max_sval },
 	{ "ieee80211_free_hw",  RELEASE, 0, "$" },
+
+	{ "alloc_pages", ALLOC,  -1, "$", &valid_ptr_min_sval, &valid_ptr_max_sval },
+	{ "__get_free_pages", ALLOC, -1, "$", &valid_ptr_min_sval, &valid_ptr_max_sval },
+	{ "free_pages", RELEASE, 0, "$" },
 };
 
 static struct smatch_state *unmatched_state(struct sm_state *sm)

Re: alloc_page leaks tracing

[Dan Carpenter]

On Thu, Mar 10, 2022 at 04:30:42PM -0500, Oleg Drokin wrote:
> 
> Thanks, I added this and there’s still no detection.
> Example code:
> 
>         __page = alloc_page(GFP_KERNEL);
>         if (__page == NULL)
>                 return -ENOMEM;
> 
>         req = cfs_crypto_hash_init(cfs_alg, NULL, 0);
>         if (IS_ERR(req)) {
>                 CERROR("%s: unable to initialize checksum hash %s\n",
>                        tgt_name(tgt), cfs_crypto_hash_name(cfs_alg));
>                 return PTR_ERR(req);
>         }
> 
>         buffer = kmap(__page);
> 
> 
> diff --git a/check_unwind.c b/check_unwind.c
> index 569792ad..7611f377 100644
> --- a/check_unwind.c
> +++ b/check_unwind.c
> @@ -92,6 +92,11 @@ static struct ref_func_info func_table[] = {
>  
>         { "ieee80211_alloc_hw", ALLOC,  -1, "$", &valid_ptr_min_sval, &valid_ptr_max_sval },
>         { "ieee80211_free_hw",  RELEASE, 0, "$" },
> +       { "alloc_pages", ALLOC,  -1, "$", &valid_ptr_min_sval, &valid_ptr_max_sval },
> +       { "alloc_page", ALLOC,  -1, "$", &valid_ptr_min_sval, &valid_ptr_max_sval },
> +       { "__get_free_pages", ALLOC, -1, "$", &valid_ptr_min_sval, &valid_ptr_max_sval },
> +       { "free_pages", RELEASE, 0, "$" },
> +       { "__free_page", RELEASE, 0, "$" },
>  };

Hey Oleg,

Sorry, I should have tested this better...

There are two issues.  The easy issue is that apparently I didn't stop
to consider error pointers as a failure return.

The more complicated problem is that the allocation hook will only
trigger for cases where Smatch knows what the return is.  Right now
Smatch knows that alloc_pages either returns NULL, or non-NULL (but
potentially error pointers).  This is return_implies_param_key_exact()
vs return_implies_param_key().

Of course, I changed it to _exact() deliberately to silence false
positives.  I don't remember the details so I'll retest tonight and look
again on Monday.

An easy "fix" would be to add "alloc_pages 1-u64max 4096-ptr_max" so it
gets hardcoded on the next db rebuild, but that's kind of a hack as
well.

diff --git a/check_unwind.c b/check_unwind.c
index 569792ad5a57..c3015cca3374 100644
--- a/check_unwind.c
+++ b/check_unwind.c
@@ -92,6 +92,10 @@ static struct ref_func_info func_table[] = {
 
 	{ "ieee80211_alloc_hw", ALLOC,  -1, "$", &valid_ptr_min_sval, &valid_ptr_max_sval },
 	{ "ieee80211_free_hw",  RELEASE, 0, "$" },
+
+	{ "alloc_pages", ALLOC,  -1, "$", &valid_ptr_min_sval, &valid_ptr_max_sval },
+	{ "__get_free_pages", ALLOC, -1, "$", &valid_ptr_min_sval, &valid_ptr_max_sval },
+	{ "free_pages", RELEASE, 0, "$" },
 };
 
 static struct smatch_state *unmatched_state(struct sm_state *sm)
@@ -237,11 +241,14 @@ enum {
 static int success_fail_positive(struct range_list *rl)
 {
 	if (!rl)
-		return UNKNOWN;
+		return SUCCESS;
 
 	if (sval_is_negative(rl_min(rl)) && sval_is_negative(rl_max(rl)))
 		return FAIL;
 
+	if (type_is_ptr(rl_type(rl)) && is_err_or_null(rl))
+		return FAIL;
+
 	if (rl_min(rl).value == 0)
 		return SUCCESS;
 
@@ -334,7 +341,7 @@ void check_unwind(int id)
 		if (info->call_back) {
 			add_function_hook(info->name, info->call_back, info);
 		} else if (info->implies_start && info->type == ALLOC) {
-			return_implies_param_key_exact(info->name,
+			return_implies_param_key(info->name,
 					*info->implies_start,
 					*info->implies_end,
 					&return_param_alloc,

Re: alloc_page leaks tracing

[Dan Carpenter]

I've pushed some changes to the check_unwind.c script so now the results
say which function allocated it.

I still haven't added the alloc_pages() and related functions to the
function table.  I need to do that.  I'm also still thinking about the
correct solution to the return_implies_param_key_exact() problem.  I
guess on my system, I just hardcoded the return values.  There is no
harm in pushing that so let me do that.

There are bunch of common false positives like:
1) put_device() frees the page.  I've been sort of working towards
   making put_device() handled in the correct way where we figure out
   what function it calls and parse the container_of() correctly and
   then it's all handled transparently by the check_kernel.c.  But for
   now probably the right thing is to add a special case to
   check_unwind.c.
2) Arrays.  Again, probably add a special case in check_unwind.c
3) Direct calls to a function which frees pointers from container_of().
   Eventually this will work correctly.  Another option for this would
   be to do what check_locking does and just record whenever a function
   frees something and then in the caller try to guess which resource
   it was based on the name.

So there is quite a bit of work which can be done still.

regards,
dan carpenter

Re: alloc_page leaks tracing

[Dan Carpenter]

Another option to free the page is put_page() right?

To me it looks like r5l_recovery_verify_data_checksum_for_mb() should
use free_page() but put_page() also works I think.  My understanding is
that you could swap all the free_page() calls for put_page() but not the
other way round.

regards,
dan carpenter