From: Dan Carpenter <dan.carpenter@oracle.com>
To: Nigel Christian <nigel.l.christian@gmail.com>
Cc: kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL
Date: Tue, 1 Jun 2021 22:00:40 +0300 [thread overview]
Message-ID: <20210601190040.GG24442@kadam> (raw)
In-Reply-To: <YLZ0Q6S2A9kxXk6c@fedora>
On Tue, Jun 01, 2021 at 01:54:11PM -0400, Nigel Christian wrote:
> On Tue, Jun 01, 2021 at 12:25:40PM +0300, Dan Carpenter wrote:
> > The alloc_migrate_huge_page() doesn't return error pointers, it returns
> > NULL.
>
> Hi Dan,
>
> I'm trying to start using smatch. Is this in the warns report?
> Wasn't able to find using smatch_scripts/kchecker mm/hugetlb.c (T_T)
>
No, it's not. I was never able to make the check work well enough to
publish. To many false positives, because some functions return error
pointers depending on the .config. Also I should have modified it to
ignore paramaters and only look at returned pointers.
The test is from 12 years ago and it's really bad... :P Also it
references some debugfs rules which changed a many years ago. I have
to go for a call but it would probably take 15 minutes to write it
correctly these days.
regards,
dan carpenter
/*
* Copyright (C) 2009 Dan Carpenter.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* as published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
*/
#include "smatch.h"
#include "smatch_slist.h"
#include "smatch_extra.h"
static int my_id;
STATE(is_null);
STATE(ok);
static void ok_to_use(struct sm_state *sm, struct expression *mod_expr)
{
set_state(my_id, sm->name, sm->sym, &ok);
}
static bool is_basically_unknown(struct expression *expr)
{
struct sm_state *sm, *tmp;
bool known = false;
sm = get_extra_sm_state(expr);
if (!sm)
return false;
FOR_EACH_PTR(sm->possible, tmp) {
if (!is_whole_rl(estate_rl(tmp->state)))
known = true;
} END_FOR_EACH_PTR(tmp);
return !known;
}
static int rl_is_null_and_valid(struct range_list *rl)
{
struct data_range *range;
int i;
i = -1;
FOR_EACH_PTR(rl, range) {
i++;
if (i == 0) {
if (range->min.value != 0 || range->max.value != 0)
return 0;
continue;
}
if (i == 1) {
if (range->min.value == valid_ptr_min && range->max.value == valid_ptr_max)
return 1;
}
return 0;
} END_FOR_EACH_PTR(range);
return 0;
}
static struct expression *get_call(struct expression *pointer)
{
struct expression *assigned;
assigned = get_assigned_expr(pointer);
if (!assigned)
return NULL;
if (assigned->type != EXPR_CALL)
return NULL;
return assigned;
}
static int is_a_debugfs_thing(struct expression *call)
{
if (!call)
return 0;
if (call->fn->type != EXPR_SYMBOL)
return 0;
if (strstr(call->fn->symbol_name->name, "debugfs"))
return 1;
return 0;
}
static int returns_err_ptr_sometimes(struct expression *call)
{
struct range_list *rl;
if (!call)
return 0;
/* get_implied_rl() should normally return success here. */
if (!get_implied_rl(call, &rl))
return 0;
if (rl_max(rl).uvalue >= (unsigned long long)-4095)
return 1;
return 0;
}
static void match_is_err(const char *fn, struct expression *expr, void *unused)
{
struct expression *arg;
struct expression *call;
struct range_list *rl;
char *name;
arg = get_argument_from_call_expr(expr->args, 0);
if (!get_implied_rl(arg, &rl))
return;
if (is_basically_unknown(arg))
return;
if (rl_is_null_and_valid(rl))
set_state_expr(my_id, arg, &is_null);
if (rl_max(rl).uvalue >= (unsigned long long)-4095)
return;
call = get_call(arg);
if (is_a_debugfs_thing(call))
return;
if (returns_err_ptr_sometimes(call))
return;
name = expr_to_str(arg);
sm_msg("warn: '%s' isn't an ERR_PTR", name);
free_string(name);
}
static void match_dereferences(struct expression *expr)
{
struct sm_state *sm;
char *name;
expr = strip_expr(expr->unop);
sm = get_sm_state_expr(my_id, expr);
if (!sm)
return;
if (!slist_has_state(sm->possible, &is_null))
return;
if (implied_not_equal(expr, 0))
return;
name = expr_to_str(expr);
sm_msg("warn: possible NULL dereference of '%s'", name);
free_string(name);
}
void check_err_ptr_vs_null(int id)
{
if (option_project != PROJ_KERNEL)
return;
my_id = id;
add_function_hook("IS_ERR", &match_is_err, NULL);
add_hook(&match_dereferences, DEREF_HOOK);
add_modification_hook(my_id, &ok_to_use);
}
next prev parent reply other threads:[~2021-06-01 19:00 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-01 9:25 [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL Dan Carpenter
2021-06-01 10:52 ` Mina Almasry
2021-06-01 17:54 ` Nigel Christian
2021-06-01 19:00 ` Dan Carpenter [this message]
2021-06-01 19:51 ` Dan Carpenter
2021-06-01 20:50 ` Dan Carpenter
2021-06-01 21:23 ` Nigel Christian
2021-06-02 6:11 ` Dan Carpenter
2021-06-02 14:47 ` Dan Carpenter
2021-06-02 16:01 ` Nigel Christian
2021-06-04 13:34 ` Dan Carpenter
2021-06-04 14:14 ` Nigel Christian
2021-06-04 14:21 ` Dan Carpenter
2021-06-02 14:22 ` Dan Carpenter
2021-06-02 15:57 ` Nigel Christian
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210601190040.GG24442@kadam \
--to=dan.carpenter@oracle.com \
--cc=kernel-janitors@vger.kernel.org \
--cc=nigel.l.christian@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).