[PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Dan Carpenter]

The alloc_migrate_huge_page() doesn't return error pointers, it returns
NULL.

Fixes: ab45bc8b5910 ("mm, hugetlb: fix resv_huge_pages underflow on UFFDIO_COPY")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 mm/hugetlb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 69a4b551c157..3221c94b4749 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5103,7 +5103,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
 			 */
 			page = alloc_migrate_huge_page(h, gfp_mask, node,
 						       nodemask);
-			if (IS_ERR(page)) {
+			if (!page) {
 				ret = -ENOMEM;
 				goto out;
 			}
-- 
2.30.2

Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Mina Almasry]

On Tue, Jun 1, 2021 at 2:26 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> The alloc_migrate_huge_page() doesn't return error pointers, it returns
> NULL.
>
> Fixes: ab45bc8b5910 ("mm, hugetlb: fix resv_huge_pages underflow on UFFDIO_COPY")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  mm/hugetlb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Gah, my bad.

Reviewed-by: Mina Almasry <almasrymina@google.com>

> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 69a4b551c157..3221c94b4749 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5103,7 +5103,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
>                          */
>                         page = alloc_migrate_huge_page(h, gfp_mask, node,
>                                                        nodemask);
> -                       if (IS_ERR(page)) {
> +                       if (!page) {
>                                 ret = -ENOMEM;
>                                 goto out;
>                         }
> --
> 2.30.2
>

Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Mina Almasry]

On Tue, Jun 1, 2021 at 2:26 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> The alloc_migrate_huge_page() doesn't return error pointers, it returns
> NULL.
>
> Fixes: ab45bc8b5910 ("mm, hugetlb: fix resv_huge_pages underflow on UFFDIO_COPY")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  mm/hugetlb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Gah, my bad.

Reviewed-by: Mina Almasry <almasrymina@google.com>

> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 69a4b551c157..3221c94b4749 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5103,7 +5103,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
>                          */
>                         page = alloc_migrate_huge_page(h, gfp_mask, node,
>                                                        nodemask);
> -                       if (IS_ERR(page)) {
> +                       if (!page) {
>                                 ret = -ENOMEM;
>                                 goto out;
>                         }
> --
> 2.30.2
>

Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Nigel Christian]

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)

> 
> Fixes: ab45bc8b5910 ("mm, hugetlb: fix resv_huge_pages underflow on UFFDIO_COPY")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  mm/hugetlb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 69a4b551c157..3221c94b4749 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5103,7 +5103,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
>  			 */
>  			page = alloc_migrate_huge_page(h, gfp_mask, node,
>  						       nodemask);
> -			if (IS_ERR(page)) {
> +			if (!page) {
>  				ret = -ENOMEM;
>  				goto out;
>  			}
> -- 
> 2.30.2
> 

Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Dan Carpenter]

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);
}

Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Dan Carpenter]

Here is my next attempt at this check.

Back in 2009, I used to write Smatch checks which were too complicated.
Ideally, each Smatch check should only print one warning.  The state
engine should only have one custom state, and &undefined and &merged.
That check I sent violated all those rules.

The other thing which might be interesting is if you pass a NULL
to IS_ERR() and then dereference the NULL then print a warning about
that.  This has a lot of overlaps with some of my existing checks, but
it's still a new idea so it belongs in a separate check.  It's fine and
good even if one bug triggers a lot of different warnings.  I'll write
that, hang on, brb.

regards,
dan carpenter

/*
 * Copyright (C) 2021 Oracle.
 *
 * 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_extra.h"

static int my_id;

static void match_is_err(const char *fn, struct expression *expr, void *unused)
{
	struct expression *arg, *call;
	struct range_list *rl;
	char *name;

	arg = get_argument_from_call_expr(expr->args, 0);
	/* ignore unknown values */
	if (!get_implied_rl(arg, &rl))
		return;
	/* error pointers are what we expect */
	if (rl_max(rl).uvalue >= (unsigned long long)-4095)
		return;
	/* ignore valid pointers */
	if (rl_min(rl).uvalue != 0)
		return;
	/*
	 * Don't warn if people are using IS_ERR() to sanity check their
	 * parameters.
	 */
	call = get_assigned_expr(arg);
	call = strip_expr(call);
	if (!call || call->type != EXPR_CALL)
		return;

	name = expr_to_str(arg);
	sm_warning("'%s' is not an error pointer", name);
	free_string(name);
}

void check_not_an_err_ptr(int id)
{
	my_id = id;

	if (option_project != PROJ_KERNEL)
		return;

	add_function_hook("IS_ERR", &match_is_err, NULL);
}

Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Dan Carpenter]

On Tue, Jun 01, 2021 at 10:51:23PM +0300, Dan Carpenter wrote:
> The other thing which might be interesting is if you pass a NULL
> to IS_ERR() and then dereference the NULL then print a warning about
> that.  This has a lot of overlaps with some of my existing checks, but
> it's still a new idea so it belongs in a separate check.  It's fine and
> good even if one bug triggers a lot of different warnings.  I'll write
> that, hang on, brb.

100% untested.  :)  I'll test it tonight.

There are a bunch of ways to write a check like this.  This test is
based on copy and paste, guess work, and instinct.  I normally just
start writing the simplest check I can and test that, then I refine it
based on whatever the common false postives are.

In this code, do I need to have a modification hook?  Probably not, but
it was in the original code I copy and pasted and it seemed harmless.
Slightly ugly perhaps?

I knew from experience that I want to check if it's an explicit NULL
pointer passed to IS_ERR().  There are a few ways to write that.  I
could have looked at the values or I could have looked at the ->possible
values.  I probably should have looked at the values instead...

The __in_fake_assign assignment is copy and paste.  I shoud probably
delete that but it's harmless and a potential speed up.  It was in the
check_check_deref.c and I don't remember why.  Probably it's essential.

I'm not happy with the DEREF_HOOK api.  I've been planning to re-write
that but I haven't yet.

regards,
dan carpenter

/*
 * Copyright (C) 2021 Oracle.
 *
 * 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_extra.h"
#include "smatch_slist.h"

static int my_id;

STATE(null);

static void is_ok(struct sm_state *sm, struct expression *mod_expr)
{
	set_state(my_id, sm->name, sm->sym, &undefined);
}

/*
 * The expr_has_null_exact() function means that it was explicitly
 * assigned NULL, not just that it is potentially NULL.
 */
static bool expr_has_null_exact(struct expression *expr)
{
	struct sm_state *sm, *tmp;
	sval_t sval;

	sm = get_sm_state_expr(SMATCH_EXTRA, expr);
	if (!sm)
		return false;

	FOR_EACH_PTR(sm->possible, tmp) {
		if (!estate_get_single_value(tmp->state, &sval))
			continue;
		if (sval.value == 0)
			return true;
	} END_FOR_EACH_PTR(tmp);

	return false;
}

static void match_is_err(const char *fn, struct expression *expr, void *unused)
{
	struct expression *arg;

	arg = get_argument_from_call_expr(expr->args, 0);
	if (!expr_has_null_exact(arg))
		return;
	set_state_expr(my_id, arg, &null);
}

static void check_dereference(struct expression *expr)
{
	char *name;

	if (__in_fake_assign)
		return;

	if (get_state_expr(my_id, expr) != &null)
		return;
	if (implied_not_equal(expr, 0))
		return;

	name = expr_to_str(expr);
	sm_error("potential NULL dereference '%s'", name);
	free_string(name);
}

static void match_dereferences(struct expression *expr)
{
	if (expr->type != EXPR_PREOP)
		return;
	check_dereference(expr->unop);
}

void check_null_deref_after_IS_ERR(int id)
{
	my_id = id;

	if (option_project != PROJ_KERNEL)
		return;

	add_function_hook("IS_ERR", &match_is_err, NULL);
	add_hook(&match_dereferences, DEREF_HOOK);

	add_modification_hook(my_id, &is_ok);
}

Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Nigel Christian]

On Tue, Jun 01, 2021 at 11:50:06PM +0300, Dan Carpenter wrote:
> On Tue, Jun 01, 2021 at 10:51:23PM +0300, Dan Carpenter wrote:
> > The other thing which might be interesting is if you pass a NULL
> > to IS_ERR() and then dereference the NULL then print a warning about
> > that.  This has a lot of overlaps with some of my existing checks, but
> > it's still a new idea so it belongs in a separate check.  It's fine and
> > good even if one bug triggers a lot of different warnings.  I'll write
> > that, hang on, brb.
> 
> 100% untested.  :)  I'll test it tonight.

Ha, you make it look easy. Let me know if I can help with testing
Should I just add below to my smatch and recompile,
or is there an experimental branch to build from?

> 
> There are a bunch of ways to write a check like this.  This test is
> based on copy and paste, guess work, and instinct.  I normally just
> start writing the simplest check I can and test that, then I refine it
> based on whatever the common false postives are.
> 
> In this code, do I need to have a modification hook?  Probably not, but
> it was in the original code I copy and pasted and it seemed harmless.
> Slightly ugly perhaps?
> 
> I knew from experience that I want to check if it's an explicit NULL
> pointer passed to IS_ERR().  There are a few ways to write that.  I
> could have looked at the values or I could have looked at the ->possible
> values.  I probably should have looked at the values instead...
> 
> The __in_fake_assign assignment is copy and paste.  I shoud probably
> delete that but it's harmless and a potential speed up.  It was in the
> check_check_deref.c and I don't remember why.  Probably it's essential.
> 
> I'm not happy with the DEREF_HOOK api.  I've been planning to re-write
> that but I haven't yet.
> 
> regards,
> dan carpenter
> 
> /*
>  * Copyright (C) 2021 Oracle.
>  *
>  * 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_extra.h"
> #include "smatch_slist.h"
> 
> static int my_id;
> 
> STATE(null);
> 
> static void is_ok(struct sm_state *sm, struct expression *mod_expr)
> {
> 	set_state(my_id, sm->name, sm->sym, &undefined);
> }
> 
> /*
>  * The expr_has_null_exact() function means that it was explicitly
>  * assigned NULL, not just that it is potentially NULL.
>  */
> static bool expr_has_null_exact(struct expression *expr)
> {
> 	struct sm_state *sm, *tmp;
> 	sval_t sval;
> 
> 	sm = get_sm_state_expr(SMATCH_EXTRA, expr);
> 	if (!sm)
> 		return false;
> 
> 	FOR_EACH_PTR(sm->possible, tmp) {
> 		if (!estate_get_single_value(tmp->state, &sval))
> 			continue;
> 		if (sval.value == 0)
> 			return true;
> 	} END_FOR_EACH_PTR(tmp);
> 
> 	return false;
> }
> 
> static void match_is_err(const char *fn, struct expression *expr, void *unused)
> {
> 	struct expression *arg;
> 
> 	arg = get_argument_from_call_expr(expr->args, 0);
> 	if (!expr_has_null_exact(arg))
> 		return;
> 	set_state_expr(my_id, arg, &null);
> }
> 
> static void check_dereference(struct expression *expr)
> {
> 	char *name;
> 
> 	if (__in_fake_assign)
> 		return;
> 
> 	if (get_state_expr(my_id, expr) != &null)
> 		return;
> 	if (implied_not_equal(expr, 0))
> 		return;
> 
> 	name = expr_to_str(expr);
> 	sm_error("potential NULL dereference '%s'", name);
> 	free_string(name);
> }
> 
> static void match_dereferences(struct expression *expr)
> {
> 	if (expr->type != EXPR_PREOP)
> 		return;
> 	check_dereference(expr->unop);
> }
> 
> void check_null_deref_after_IS_ERR(int id)
> {
> 	my_id = id;
> 
> 	if (option_project != PROJ_KERNEL)
> 		return;
> 
> 	add_function_hook("IS_ERR", &match_is_err, NULL);
> 	add_hook(&match_dereferences, DEREF_HOOK);
> 
> 	add_modification_hook(my_id, &is_ok);
> }
> 
> 

Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Dan Carpenter]

On Tue, Jun 01, 2021 at 05:23:46PM -0400, Nigel Christian wrote:
> On Tue, Jun 01, 2021 at 11:50:06PM +0300, Dan Carpenter wrote:
> > On Tue, Jun 01, 2021 at 10:51:23PM +0300, Dan Carpenter wrote:
> > > The other thing which might be interesting is if you pass a NULL
> > > to IS_ERR() and then dereference the NULL then print a warning about
> > > that.  This has a lot of overlaps with some of my existing checks, but
> > > it's still a new idea so it belongs in a separate check.  It's fine and
> > > good even if one bug triggers a lot of different warnings.  I'll write
> > > that, hang on, brb.
> > 
> > 100% untested.  :)  I'll test it tonight.
> 
> Ha, you make it look easy. Let me know if I can help with testing
> Should I just add below to my smatch and recompile,
> or is there an experimental branch to build from?
> 

Yeah.  :)  Copy and paste that to check_null_deref_after_IS_ERR.c and
add it to the check_list.h file then recompile.

regards,
dan carpenter

Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Dan Carpenter]

On Tue, Jun 01, 2021 at 11:50:06PM +0300, Dan Carpenter wrote:
> On Tue, Jun 01, 2021 at 10:51:23PM +0300, Dan Carpenter wrote:
> > The other thing which might be interesting is if you pass a NULL
> > to IS_ERR() and then dereference the NULL then print a warning about
> > that.  This has a lot of overlaps with some of my existing checks, but
> > it's still a new idea so it belongs in a separate check.  It's fine and
> > good even if one bug triggers a lot of different warnings.  I'll write
> > that, hang on, brb.
> 
> 100% untested.  :)  I'll test it tonight.
> 

This test is decent, but I ended up making a few changes:

1)  My devel version of Smatch had a new bug in it which caused some
    false positives.  Fixed now, hopefully.

2)  The test:

	if (get_state_expr(my_id, expr) != &null)
		return;

    check was not strict enough.  I realized that I knew that from
    square one but I was lazy.  So now I have introduced a global helper
    function and updated the code:

bool expr_has_possible_state(int owner, struct expression *expr, struct smatch_state *state)
{
        struct sm_state *sm;

        sm = get_sm_state_expr(owner, expr);
        if (!sm)
                return false;

        return slist_has_state(sm->possible, state);
}

    I replaced the test with:

	if (!expr_has_possible_state(my_id, expr, &null))

3)  The warning message was too vague and too similar to other warning
    messages.  It should be something unique to the test.  It's now:

	sm_error("potential NULL/IS_ERR bug '%s'", name);

I'll post the results tomorrow.

regards,
dan carpenter

Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Nigel Christian]

On Wed, Jun 02, 2021 at 05:47:52PM +0300, Dan Carpenter wrote:
> On Tue, Jun 01, 2021 at 11:50:06PM +0300, Dan Carpenter wrote:
> > On Tue, Jun 01, 2021 at 10:51:23PM +0300, Dan Carpenter wrote:
> > > The other thing which might be interesting is if you pass a NULL
> > > to IS_ERR() and then dereference the NULL then print a warning about
> > > that.  This has a lot of overlaps with some of my existing checks, but
> > > it's still a new idea so it belongs in a separate check.  It's fine and
> > > good even if one bug triggers a lot of different warnings.  I'll write
> > > that, hang on, brb.
> > 
> > 100% untested.  :)  I'll test it tonight.
> > 
> 
> This test is decent, but I ended up making a few changes:
> 
> 1)  My devel version of Smatch had a new bug in it which caused some
>     false positives.  Fixed now, hopefully.
  
Many thanks! Gonna check today.

> 
> 2)  The test:
> 
> 	if (get_state_expr(my_id, expr) != &null)
> 		return;
> 
>     check was not strict enough.  I realized that I knew that from
>     square one but I was lazy.  So now I have introduced a global helper
>     function and updated the code:
> 
> bool expr_has_possible_state(int owner, struct expression *expr, struct smatch_state *state)
> {
>         struct sm_state *sm;
> 
>         sm = get_sm_state_expr(owner, expr);
>         if (!sm)
>                 return false;
> 
>         return slist_has_state(sm->possible, state);
> }
> 
>     I replaced the test with:
> 
> 	if (!expr_has_possible_state(my_id, expr, &null))
> 
> 3)  The warning message was too vague and too similar to other warning
>     messages.  It should be something unique to the test.  It's now:
> 
> 	sm_error("potential NULL/IS_ERR bug '%s'", name);
> 
> I'll post the results tomorrow.

This is great, will be a nice bug to check for beginners like me.

> 
> regards,
> dan carpenter
> 

Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Dan Carpenter]

On Wed, Jun 02, 2021 at 05:47:52PM +0300, Dan Carpenter wrote:
> On Tue, Jun 01, 2021 at 11:50:06PM +0300, Dan Carpenter wrote:
> > On Tue, Jun 01, 2021 at 10:51:23PM +0300, Dan Carpenter wrote:
> > > The other thing which might be interesting is if you pass a NULL
> > > to IS_ERR() and then dereference the NULL then print a warning about
> > > that.  This has a lot of overlaps with some of my existing checks, but
> > > it's still a new idea so it belongs in a separate check.  It's fine and
> > > good even if one bug triggers a lot of different warnings.  I'll write
> > > that, hang on, brb.
> > 
> > 100% untested.  :)  I'll test it tonight.
> > 
> 

I also added a check for:

	if (is_impossible_path())
		return;

to silence some of the false positives.  But the results are all still
false positives.  They're "high quality" false positives, because often
the code looks buggy.  I think that someone went through and fixed all
the real bugs with this.

I reported the snd_media_device_create() bug and sent a fix for the
nfs_init_server() but the rest, I'm just going to leave.

sound/usb/media.c:287 snd_media_device_create() error: potential NULL/IS_ERR bug 'mdev'
mm/mempolicy.c:1293 do_mbind() error: potential NULL/IS_ERR bug 'new'
fs/nfs/client.c:701 nfs_init_server() error: potential NULL/IS_ERR bug 'clp'
fs/afs/server.c:652 afs_update_server_record() error: potential NULL/IS_ERR bug 'alist'
fs/afs/volume.c:322 afs_update_volume_status() error: potential NULL/IS_ERR bug 'vldb'
fs/ubifs/lpt_commit.c:583 next_pnode_to_dirty() error: potential NULL/IS_ERR bug 'nnode'
fs/ubifs/lpt_commit.c:583 next_pnode_to_dirty() error: potential NULL/IS_ERR bug 'nnode'
fs/ntfs/attrib.c:2188 ntfs_attr_extend_allocation() error: potential NULL/IS_ERR bug 'rl'
fs/ext4/namei.c:2375 ext4_add_entry() error: potential NULL/IS_ERR bug 'bh'
drivers/pinctrl/renesas/core.c:1144 sh_pfc_probe() error: potential NULL/IS_ERR bug 'info'
drivers/mfd/ene-kb3930.c:165 kb3930_probe() error: potential NULL/IS_ERR bug 'ddata->off_gpios'
drivers/mtd/ubi/attach.c:409 add_volume() error: potential NULL/IS_ERR bug 'av'
drivers/mtd/ubi/fastmap.c:185 add_vol() error: potential NULL/IS_ERR bug 'av'
drivers/gpu/drm/nouveau/nvkm/engine/nvdec/base.c:57 nvkm_nvdec_new_() error: potential NULL/IS_ERR bug 'fwif'
drivers/gpu/drm/nouveau/nvkm/engine/sec2/base.c:104 nvkm_sec2_new_() error: potential NULL/IS_ERR bug 'fwif'
drivers/gpu/drm/nouveau/nvkm/engine/gr/gf100.c:2108 gf100_gr_new_() error: potential NULL/IS_ERR bug 'fwif'
drivers/gpu/drm/nouveau/nvkm/engine/nvenc/base.c:59 nvkm_nvenc_new_() error: potential NULL/IS_ERR bug 'fwif'
drivers/gpu/drm/nouveau/nvkm/subdev/pmu/base.c:181 nvkm_pmu_ctor() error: potential NULL/IS_ERR bug 'fwif'
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/base.c:56 nvkm_gsp_new_() error: potential NULL/IS_ERR bug 'fwif'
drivers/gpu/drm/nouveau/nvkm/subdev/acr/base.c:430 nvkm_acr_new_() error: potential NULL/IS_ERR bug 'fwif'
drivers/clk/clk.c:417 clk_core_get() error: potential NULL/IS_ERR bug 'hw'
drivers/platform/chrome/cros_ec_chardev.c:225 cros_ec_chardev_read() error: potential NULL/IS_ERR bug 'event'
net/xfrm/xfrm_policy.c:2785 xfrm_policy_queue_process() error: potential NULL/IS_ERR bug 'dst'
net/xfrm/xfrm_interface.c:282 xfrmi_xmit2() error: potential NULL/IS_ERR bug 'dst'
net/ipv6/netfilter/nf_reject_ipv6.c:327 nf_send_reset6() error: potential NULL/IS_ERR bug 'dst'
net/ipv6/ip6_tunnel.c:1161 ip6_tnl_xmit() error: potential NULL/IS_ERR bug 'dst'
net/ipv6/ndisc.c:505 ndisc_send_skb() error: potential NULL/IS_ERR bug 'dst'
security/selinux/hooks.c:3157 selinux_inode_follow_link() error: potential NULL/IS_ERR bug 'isec'
security/selinux/hooks.c:3212 selinux_inode_permission() error: potential NULL/IS_ERR bug 'isec'

regards,
dan carpenter

Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Nigel Christian]

On Fri, Jun 04, 2021 at 04:34:00PM +0300, Dan Carpenter wrote:
> On Wed, Jun 02, 2021 at 05:47:52PM +0300, Dan Carpenter wrote:
> > On Tue, Jun 01, 2021 at 11:50:06PM +0300, Dan Carpenter wrote:
> > > On Tue, Jun 01, 2021 at 10:51:23PM +0300, Dan Carpenter wrote:
> > > > The other thing which might be interesting is if you pass a NULL
> > > > to IS_ERR() and then dereference the NULL then print a warning about
> > > > that.  This has a lot of overlaps with some of my existing checks, but
> > > > it's still a new idea so it belongs in a separate check.  It's fine and
> > > > good even if one bug triggers a lot of different warnings.  I'll write
> > > > that, hang on, brb.
> > > 
> > > 100% untested.  :)  I'll test it tonight.
> > > 
> > 
> 
> I also added a check for:
> 
> 	if (is_impossible_path())
> 		return;
> 
> to silence some of the false positives.  But the results are all still
> false positives.  They're "high quality" false positives, because often
> the code looks buggy.  I think that someone went through and fixed all
> the real bugs with this.

Double positives are good. Glad the bugs are getting fixed!
Back to hunting for deadcode. That seems to be the lowest
hanging fruit for me at the momemnt. (-_-)

> 
> I reported the snd_media_device_create() bug and sent a fix for the
> nfs_init_server() but the rest, I'm just going to leave.
> 
> sound/usb/media.c:287 snd_media_device_create() error: potential NULL/IS_ERR bug 'mdev'
> mm/mempolicy.c:1293 do_mbind() error: potential NULL/IS_ERR bug 'new'
> fs/nfs/client.c:701 nfs_init_server() error: potential NULL/IS_ERR bug 'clp'
> fs/afs/server.c:652 afs_update_server_record() error: potential NULL/IS_ERR bug 'alist'
> fs/afs/volume.c:322 afs_update_volume_status() error: potential NULL/IS_ERR bug 'vldb'
> fs/ubifs/lpt_commit.c:583 next_pnode_to_dirty() error: potential NULL/IS_ERR bug 'nnode'
> fs/ubifs/lpt_commit.c:583 next_pnode_to_dirty() error: potential NULL/IS_ERR bug 'nnode'
> fs/ntfs/attrib.c:2188 ntfs_attr_extend_allocation() error: potential NULL/IS_ERR bug 'rl'
> fs/ext4/namei.c:2375 ext4_add_entry() error: potential NULL/IS_ERR bug 'bh'
> drivers/pinctrl/renesas/core.c:1144 sh_pfc_probe() error: potential NULL/IS_ERR bug 'info'
> drivers/mfd/ene-kb3930.c:165 kb3930_probe() error: potential NULL/IS_ERR bug 'ddata->off_gpios'
> drivers/mtd/ubi/attach.c:409 add_volume() error: potential NULL/IS_ERR bug 'av'
> drivers/mtd/ubi/fastmap.c:185 add_vol() error: potential NULL/IS_ERR bug 'av'
> drivers/gpu/drm/nouveau/nvkm/engine/nvdec/base.c:57 nvkm_nvdec_new_() error: potential NULL/IS_ERR bug 'fwif'
> drivers/gpu/drm/nouveau/nvkm/engine/sec2/base.c:104 nvkm_sec2_new_() error: potential NULL/IS_ERR bug 'fwif'
> drivers/gpu/drm/nouveau/nvkm/engine/gr/gf100.c:2108 gf100_gr_new_() error: potential NULL/IS_ERR bug 'fwif'
> drivers/gpu/drm/nouveau/nvkm/engine/nvenc/base.c:59 nvkm_nvenc_new_() error: potential NULL/IS_ERR bug 'fwif'
> drivers/gpu/drm/nouveau/nvkm/subdev/pmu/base.c:181 nvkm_pmu_ctor() error: potential NULL/IS_ERR bug 'fwif'
> drivers/gpu/drm/nouveau/nvkm/subdev/gsp/base.c:56 nvkm_gsp_new_() error: potential NULL/IS_ERR bug 'fwif'
> drivers/gpu/drm/nouveau/nvkm/subdev/acr/base.c:430 nvkm_acr_new_() error: potential NULL/IS_ERR bug 'fwif'
> drivers/clk/clk.c:417 clk_core_get() error: potential NULL/IS_ERR bug 'hw'
> drivers/platform/chrome/cros_ec_chardev.c:225 cros_ec_chardev_read() error: potential NULL/IS_ERR bug 'event'
> net/xfrm/xfrm_policy.c:2785 xfrm_policy_queue_process() error: potential NULL/IS_ERR bug 'dst'
> net/xfrm/xfrm_interface.c:282 xfrmi_xmit2() error: potential NULL/IS_ERR bug 'dst'
> net/ipv6/netfilter/nf_reject_ipv6.c:327 nf_send_reset6() error: potential NULL/IS_ERR bug 'dst'
> net/ipv6/ip6_tunnel.c:1161 ip6_tnl_xmit() error: potential NULL/IS_ERR bug 'dst'
> net/ipv6/ndisc.c:505 ndisc_send_skb() error: potential NULL/IS_ERR bug 'dst'
> security/selinux/hooks.c:3157 selinux_inode_follow_link() error: potential NULL/IS_ERR bug 'isec'
> security/selinux/hooks.c:3212 selinux_inode_permission() error: potential NULL/IS_ERR bug 'isec'
> 
> regards,
> dan carpenter

Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Dan Carpenter]

On Fri, Jun 04, 2021 at 10:14:42AM -0400, Nigel Christian wrote:
> On Fri, Jun 04, 2021 at 04:34:00PM +0300, Dan Carpenter wrote:
> > On Wed, Jun 02, 2021 at 05:47:52PM +0300, Dan Carpenter wrote:
> > > On Tue, Jun 01, 2021 at 11:50:06PM +0300, Dan Carpenter wrote:
> > > > On Tue, Jun 01, 2021 at 10:51:23PM +0300, Dan Carpenter wrote:
> > > > > The other thing which might be interesting is if you pass a NULL
> > > > > to IS_ERR() and then dereference the NULL then print a warning about
> > > > > that.  This has a lot of overlaps with some of my existing checks, but
> > > > > it's still a new idea so it belongs in a separate check.  It's fine and
> > > > > good even if one bug triggers a lot of different warnings.  I'll write
> > > > > that, hang on, brb.
> > > > 
> > > > 100% untested.  :)  I'll test it tonight.
> > > > 
> > > 
> > 
> > I also added a check for:
> > 
> > 	if (is_impossible_path())
> > 		return;
> > 
> > to silence some of the false positives.  But the results are all still
> > false positives.  They're "high quality" false positives, because often
> > the code looks buggy.  I think that someone went through and fixed all
> > the real bugs with this.
> 
> Double positives are good. Glad the bugs are getting fixed!
> Back to hunting for deadcode. That seems to be the lowest
> hanging fruit for me at the momemnt. (-_-)

I really doubt you're going to find much dead code worth fixing unless
you're looking at defines.  Dead code is pretty easy to fix so it's been
picked over pretty well at this point.

regards,
dan carpenter

Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Dan Carpenter]

On Tue, Jun 01, 2021 at 10:51:23PM +0300, Dan Carpenter wrote:
> Here is my next attempt at this check.
> 
> Back in 2009, I used to write Smatch checks which were too complicated.
> Ideally, each Smatch check should only print one warning.  The state
> engine should only have one custom state, and &undefined and &merged.
> That check I sent violated all those rules.
> 
> The other thing which might be interesting is if you pass a NULL
> to IS_ERR() and then dereference the NULL then print a warning about
> that.  This has a lot of overlaps with some of my existing checks, but
> it's still a new idea so it belongs in a separate check.  It's fine and
> good even if one bug triggers a lot of different warnings.  I'll write
> that, hang on, brb.
> 
> regards,
> dan carpenter
> 

This check worked decently enough.  If you want to fix some of the bugs
here they are.  I'll look at them in a couple weeks.  I fixed a couple
of the first ones I looked at (not listed).

drivers/phy/microchip/sparx5_serdes.c:2474 sparx5_serdes_probe() warn: 'iomem' is not an error pointer
drivers/usb/musb/musb_core.c:2483 musb_init_controller() warn: 'musb->dma_controller' is not an error pointer
drivers/base/power/domain.c:2566 genpd_dev_pm_detach() warn: 'pd' is not an error pointer
drivers/base/power/domain.c:2599 genpd_dev_pm_sync() warn: 'pd' is not an error pointer
drivers/pci/controller/pci-ftpci100.c:496 faraday_pci_probe() warn: 'p->bus_clk' is not an error pointer
drivers/infiniband/core/cm.c:2348 ib_send_cm_rtu() warn: 'data' is not an error pointer
drivers/infiniband/core/cm.c:2761 ib_send_cm_drep() warn: 'data' is not an error pointer
drivers/infiniband/core/cm.c:3092 ib_send_cm_mra() warn: 'data' is not an error pointer
drivers/bluetooth/btqcomsmd.c:140 btqcomsmd_probe() warn: 'btq->acl_channel' is not an error pointer
drivers/bluetooth/btqcomsmd.c:145 btqcomsmd_probe() warn: 'btq->cmd_channel' is not an error pointer
drivers/media/platform/sti/bdisp/bdisp-v4l2.c:1402 bdisp_probe() warn: 'bdisp->clock' is not an error pointer
drivers/net/ipa/ipa_modem.c:360 ipa_modem_config() warn: 'notifier' is not an error pointer
drivers/net/wireless/ath/wcn36xx/main.c:1411 wcn36xx_probe() warn: 'wcn->smd_channel' is not an error pointer
drivers/net/can/spi/hi311x.c:854 hi3110_can_probe() warn: 'clk' is not an error pointer
drivers/net/can/spi/hi311x.c:941 hi3110_can_probe() warn: 'clk' is not an error pointer
net/bridge/br_forward.c:223 br_flood() warn: 'prev' is not an error pointer
net/bridge/br_forward.c:313 br_multicast_flood() warn: 'prev' is not an error pointer

One thing that I realized is that for functions the return NULL when
they are configured out like media_device_usb_allocate() is that these
are always a one liner:

struct foo *whatever(void) { return NULL; }

And they're always in the .h file so we have access to them and can add
a check for that.

static bool is_one_liner_function(struct expression *fn)
{
        struct symbol *sym;
        int lines;

        if (fn->type != EXPR_SYMBOL || !fn->symbol)
                return false;
        sym = get_base_type(fn->symbol);
        if (!sym)
                return false;
        if (sym->stmt && sym->stmt->type == STMT_COMPOUND)
                lines = ptr_list_size((struct ptr_list *)sym->stmt->stmts);
        else if (sym->inline_stmt && sym->inline_stmt->type == STMT_COMPOUND)
                lines = ptr_list_size((struct ptr_list *)sym->inline_stmt->stmts);
        else
                return false;

        if (lines == 1)
                return true;

        return false;
}

regards,
dan carpenter

Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Nigel Christian]

On Wed, Jun 02, 2021 at 05:22:24PM +0300, Dan Carpenter wrote:
> On Tue, Jun 01, 2021 at 10:51:23PM +0300, Dan Carpenter wrote:
> > Here is my next attempt at this check.
> > 
> > Back in 2009, I used to write Smatch checks which were too complicated.
> > Ideally, each Smatch check should only print one warning.  The state
> > engine should only have one custom state, and &undefined and &merged.
> > That check I sent violated all those rules.
> > 
> > The other thing which might be interesting is if you pass a NULL
> > to IS_ERR() and then dereference the NULL then print a warning about
> > that.  This has a lot of overlaps with some of my existing checks, but
> > it's still a new idea so it belongs in a separate check.  It's fine and
> > good even if one bug triggers a lot of different warnings.  I'll write
> > that, hang on, brb.
> > 
> > regards,
> > dan carpenter
> > 
> 
> This check worked decently enough.  If you want to fix some of the bugs
> here they are.  I'll look at them in a couple weeks.  I fixed a couple
> of the first ones I looked at (not listed).

Yes, for sure! I'll get started from the top of the list below (^_^)

> 
> drivers/phy/microchip/sparx5_serdes.c:2474 sparx5_serdes_probe() warn: 'iomem' is not an error pointer
> drivers/usb/musb/musb_core.c:2483 musb_init_controller() warn: 'musb->dma_controller' is not an error pointer
> drivers/base/power/domain.c:2566 genpd_dev_pm_detach() warn: 'pd' is not an error pointer
> drivers/base/power/domain.c:2599 genpd_dev_pm_sync() warn: 'pd' is not an error pointer
> drivers/pci/controller/pci-ftpci100.c:496 faraday_pci_probe() warn: 'p->bus_clk' is not an error pointer
> drivers/infiniband/core/cm.c:2348 ib_send_cm_rtu() warn: 'data' is not an error pointer
> drivers/infiniband/core/cm.c:2761 ib_send_cm_drep() warn: 'data' is not an error pointer
> drivers/infiniband/core/cm.c:3092 ib_send_cm_mra() warn: 'data' is not an error pointer
> drivers/bluetooth/btqcomsmd.c:140 btqcomsmd_probe() warn: 'btq->acl_channel' is not an error pointer
> drivers/bluetooth/btqcomsmd.c:145 btqcomsmd_probe() warn: 'btq->cmd_channel' is not an error pointer
> drivers/media/platform/sti/bdisp/bdisp-v4l2.c:1402 bdisp_probe() warn: 'bdisp->clock' is not an error pointer
> drivers/net/ipa/ipa_modem.c:360 ipa_modem_config() warn: 'notifier' is not an error pointer
> drivers/net/wireless/ath/wcn36xx/main.c:1411 wcn36xx_probe() warn: 'wcn->smd_channel' is not an error pointer
> drivers/net/can/spi/hi311x.c:854 hi3110_can_probe() warn: 'clk' is not an error pointer
> drivers/net/can/spi/hi311x.c:941 hi3110_can_probe() warn: 'clk' is not an error pointer
> net/bridge/br_forward.c:223 br_flood() warn: 'prev' is not an error pointer
> net/bridge/br_forward.c:313 br_multicast_flood() warn: 'prev' is not an error pointer
> 
> One thing that I realized is that for functions the return NULL when
> they are configured out like media_device_usb_allocate() is that these
> are always a one liner:

Interesting. 
> 
> struct foo *whatever(void) { return NULL; }
> 
> And they're always in the .h file so we have access to them and can add
> a check for that.
> 
> static bool is_one_liner_function(struct expression *fn)
> {
>         struct symbol *sym;
>         int lines;
> 
>         if (fn->type != EXPR_SYMBOL || !fn->symbol)
>                 return false;
>         sym = get_base_type(fn->symbol);
>         if (!sym)
>                 return false;
>         if (sym->stmt && sym->stmt->type == STMT_COMPOUND)
>                 lines = ptr_list_size((struct ptr_list *)sym->stmt->stmts);
>         else if (sym->inline_stmt && sym->inline_stmt->type == STMT_COMPOUND)
>                 lines = ptr_list_size((struct ptr_list *)sym->inline_stmt->stmts);
>         else
>                 return false;
> 
>         if (lines == 1)
>                 return true;
> 
>         return false;
> }
> 
> regards,
> dan carpenter