Re: alloc_page leaks tracing

[Dan Carpenter <dan.carpenter@oracle.com>]

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,