From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49244 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229711AbiCKWqP (ORCPT ); Fri, 11 Mar 2022 17:46:15 -0500 Received: from mx0b-00069f02.pphosted.com (mx0b-00069f02.pphosted.com [205.220.177.32]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2099E28256C for ; Fri, 11 Mar 2022 14:21:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=date : from : to : cc : subject : message-id : references : content-type : content-transfer-encoding : in-reply-to : mime-version; s=corp-2021-07-09; bh=SYTZCNN1xeK1mjus4u7/VXHcxsXpuL0OkzCSyAVubyA=; b=QDKdkFxePUHDH6DiICCM/3vwiSghxu2a8HbuYpe9k91PBsYiYCQddz/C5R08i7KIpERx nVVQzNPo5dvt2KCW3ZlUhMdV5/TgfSfF2Ig7lep/qup9TgvO4aRmB2ofCI+fORxKUfJc XknNea7KvPa6CbV+NJl/BVe8r+jkA+nT9VQderm3KIgVQeMFoMYvrVVg9GLhhsZZIV53 K8NracxkezrNQGuGR+WDPEaC+vHnmEZa40iW7Ae8/eW7wy4Xwx3OBav9ZfebpR/0U15u O3RNeiaS7cLeLau8HkATrw1hXRQRtiT4AgFUdbk2YWlrhxscIqJ0VDjWD1NSaN6lcLg0 MQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.onmicrosoft.com; s=selector2-oracle-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=SYTZCNN1xeK1mjus4u7/VXHcxsXpuL0OkzCSyAVubyA=; b=tQH9vga7Q+mpPusznLpB0JLlVvWn+tiQF2N6oM4LHLPBZ1xXcYF+iUOVCLH8IH/o7hVilAS/WKBnkdH62YDlCulctFE7MeUDsWg8i9qHFfMDugwwDs1UZYUDYqYRwGXWl/OVUQG6Ym2WQk1O5vLLYMZTn1TqGzsNlh5INaFMqgM= Date: Sat, 12 Mar 2022 00:12:15 +0300 From: Dan Carpenter Subject: Re: alloc_page leaks tracing Message-ID: <20220311211215.GN3315@kadam> References: <1E9B599B-743A-4327-A36A-9DB2AE9E6E51@linuxhacker.ru> <20220310094235.GH3293@kadam> <82B7079C-AD17-4E5E-A245-71E9ED53E5A3@linuxhacker.ru> Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <82B7079C-AD17-4E5E-A245-71E9ED53E5A3@linuxhacker.ru> MIME-Version: 1.0 List-ID: To: Oleg Drokin Cc: smatch@vger.kernel.org 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,