Date:   Wed, 19 May 2021 17:16:35 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Martin Kaiser <martin@...ser.cx>
Cc:     Larry Finger <Larry.Finger@...inger.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Guenter Roeck <linux@...ck-us.net>,
        linux-staging@...ts.linux.dev, kernel-janitors@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] staging: rtl8188eu: use safe iterator in
 rtw_free_network_queue

On Tue, May 18, 2021 at 11:28:55AM +0300, Dan Carpenter wrote:
> On Mon, May 17, 2021 at 06:57:33PM +0300, Dan Carpenter wrote:
> > Thanks for catching these...  I've created a new Smatch static checker
> > warning for this but it only works for list_for_each_entry().
> > Eventually someone would have run the coccinelle script to convert these
> > list_for_each loops into list_for_each_entry().  Otherwise you have to
> > parse container_of() and I've been meaning to do that for a while but I
> > haven't yet.
> > 
> > Anyway, I'm going to test it out overnight and see what it finds.  It's
> > sort a new use for the modification_hook(), before I had only ever used
> > it to silence warnings but this check uses it to trigger warnings.  So
> > perhaps it will generate a lot of false positives.  We'll see.
> > 
> > It sets the state of the iterator to &start at the start of the loop
> > and if it's not &start state at the end then it prints a warning.
> > 
> > regards,
> > dan carpenter
> > 
> 
> That Smatch check didn't work at all.  :P  Back to the drawing board.
> 
> regards,
> dan carpenter

It's sort of working now.  I just had to get some ordering issues fixed.
Also I had to fix a bug in smatch_conditions.c...

The check itself works very well, but the heuristic is a bit off.  It
turns out we some times mess with the list iterator very intentionally.
For example, one thing people do is add a new item directly after the
current item.

About 8 out of the 32 warnings listed below are real issues.  I'm going
to send some patches and bug fixes for them.  A couple are too ancient
for and complicated to bother with.

regards,
dan carpenter

kernel/exit.c:1397 do_wait_thread() warn: iterator 'p->sibling.next' changed during iteration
kernel/exit.c:1411 ptrace_do_wait() warn: iterator 'p->ptrace_entry.next' changed during iteration
kernel/events/core.c:3220 perf_event_modify_attr() warn: iterator 'child->child_list.next' changed during iteration
kernel/events/core.c:5451 perf_event_for_each_child() warn: iterator 'child->child_list.next' changed during iteration
kernel/locking/locktorture.c:420 torture_ww_mutex_lock() warn: iterator 'll->link.next' changed during iteration
kernel/locking/test-ww_mutex.c:464 stress_reorder_work() warn: iterator 'll->link.next' changed during iteration
arch/x86/kvm/../../../virt/kvm/kvm_main.c:4866 vm_stat_clear() warn: iterator 'kvm->vm_list.next' changed during iteration
fs/btrfs/extent-tree.c:4230 find_free_extent() warn: iterator 'block_group->list.next' changed during iteration
fs/nilfs2/segment.c:1565 nilfs_segctor_update_payload_blocknr() warn: iterator 'bh->b_assoc_buffers.next' changed during iteration
fs/super.c:657 __iterate_supers() warn: iterator 'sb->s_list.next' changed during iteration
drivers/spi/spi-fsi.c:469 fsi_spi_transfer_one_message() warn: iterator 'transfer->transfer_list.next' changed during iteration
drivers/spi/spi-tegra210-quad.c:990 tegra_qspi_transfer_one_message() warn: iterator 'transfer->transfer_list.next' changed during iteration
drivers/spi/spi.c:3299 spi_split_transfers_maxsize() warn: iterator 'xfer->transfer_list.next' changed during iteration
drivers/staging/emxx_udc/emxx_udc.c:2079 _nbu2ss_nuke() warn: iterator 'req->queue.next' changed during iteration
drivers/pnp/quirks.c:59 quirk_awe32_resources() warn: iterator 'option->list.next' changed during iteration
drivers/usb/dwc3/gadget.c:1298 dwc3_prepare_trbs() warn: iterator 'req->list.next' changed during iteration
drivers/gpu/drm/ttm/ttm_device.c:145 ttm_device_swapout() warn: iterator 'bo->lru.next' changed during iteration
drivers/gpu/drm/ttm/ttm_execbuf_util.c:91 ttm_eu_reserve_buffers() warn: iterator 'entry->head.next' changed during iteration
drivers/gpu/drm/i915/gt/intel_reset.c:901 __intel_gt_unset_wedged() warn: iterator 'tl->link.next' changed during iteration
drivers/target/target_core_user.c:1264 tcmu_tmr_notify() warn: iterator 'se_cmd->state_list.next' changed during iteration
drivers/w1/w1.c:1265 w1_fini() warn: iterator 'dev->w1_master_entry.next' changed during iteration
drivers/scsi/libsas/sas_port.c:47 sas_resume_port() warn: iterator 'dev->dev_list_node.next' changed during iteration
drivers/scsi/lpfc/lpfc_hbadisc.c:993 lpfc_linkup_cleanup_nodes() warn: iterator 'ndlp->nlp_listp.next' changed during iteration
drivers/scsi/lpfc/lpfc_hbadisc.c:4983 lpfc_unreg_hba_rpis() warn: iterator 'ndlp->nlp_listp.next' changed during iteration
drivers/block/drbd/drbd_debugfs.c:311 seq_print_resource_transfer_log_summary() warn: iterator 'req->tl_requests.next' changed during iteration
drivers/infiniband/hw/mlx5/main.c:3285 mlx5_ib_init_multiport_master() warn: iterator 'mpi->list.next' changed during iteration
drivers/infiniband/core/verbs.c:1112 __ib_shared_qp_event_handler() warn: iterator 'event->element.qp->open_list.next' changed during iteration
net/core/devlink.c:10589 devlink_pernet_pre_exit() warn: iterator 'devlink->list.next' changed during iteration
net/bluetooth/l2cap_core.c:1726 l2cap_conn_ready() warn: iterator 'chan->list.next' changed during iteration
net/bluetooth/l2cap_core.c:6265 l2cap_ecred_reconf_rsp() warn: iterator 'chan->list.next' changed during iteration
net/bluetooth/l2cap_core.c:8209 l2cap_security_cfm() warn: iterator 'chan->list.next' changed during iteration
security/landlock/fs.c:347 hook_sb_delete() warn: iterator 'inode->i_sb_list.next' changed during iteration

/*
 * 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;

STATE(start);
STATE(watch);

static struct statement *iterator_stmt, *pre_stmt, *post_stmt;

static bool stmt_matches(struct expression *expr, struct statement *stmt)
{
	struct expression *tmp;
	struct statement *parent;

	if (!stmt)
		return false;
	while ((tmp = expr_get_parent_expr(expr)))
		expr = tmp;

	parent = expr_get_parent_stmt(expr);
	return parent == stmt;
}

static void set_watch(struct sm_state *sm, struct expression *mod_expr)
{
	if (mod_expr && stmt_matches(mod_expr, post_stmt))
		return;
	set_state(my_id, sm->name, sm->sym, &watch);
}

static bool is_list_macro(const char *macro)
{
	if (strcmp(macro, "list_for_each_entry") == 0)
		return true;
	return false;
}

static void match_iterator_statement(struct statement *stmt)
{
	const char *macro;

	if (stmt->type != STMT_ITERATOR ||
	    !stmt->iterator_pre_statement ||
	    !stmt->iterator_post_statement)
		return;

	macro = get_macro_name(stmt->pos);
	if (!macro)
		return;
	if (!is_list_macro(macro))
		return;

	iterator_stmt = stmt;
	pre_stmt = stmt->iterator_pre_statement;
	post_stmt = stmt->iterator_post_statement;
}

static char *get_iterator_member(void)
{
	struct expression *expr;

	if (!iterator_stmt ||
	    !iterator_stmt->iterator_pre_condition)
		return NULL;

	expr = iterator_stmt->iterator_pre_condition;
	if (expr->type != EXPR_PREOP || expr->op != '!')
		return NULL;
	expr = strip_parens(expr->unop);
	if (expr->type != EXPR_COMPARE)
		return NULL;
	expr = strip_parens(expr->left);
	if (expr->type != EXPR_PREOP || expr->op != '&')
		return NULL;
	expr = strip_expr(expr->unop);
	if (expr->type != EXPR_DEREF || !expr->member)
		return NULL;
	return expr->member->name;
}

static void match_pre_statement(struct expression *expr)
{
	char *name, *member;
	struct symbol *sym;
	char buf[64];

	if (!stmt_matches(expr, pre_stmt))
		return;

	name = expr_to_var_sym(expr->left, &sym);
	if (!name)
		return;
	member = get_iterator_member();

	snprintf(buf, sizeof(buf), "%s->%s.next", name, member);
	set_state(my_id, buf, sym, &start);
}

static void match_post_statement(struct expression *expr)
{
	struct smatch_state *state;
	char *name, *member;
	struct symbol *sym;
	char buf[64];

	if (!stmt_matches(expr, post_stmt))
		return;

	name = expr_to_var_sym(expr->left, &sym);
	if (!name)
		return;
	member = get_iterator_member();

	snprintf(buf, sizeof(buf), "%s->%s.next", name, member);
	state = get_state(my_id, buf, sym);
	if (!state || state == &start)
		return;

	sm_warning("iterator '%s' changed during iteration", buf);
}

void check_list_set_inside(int id)
{
	my_id = id;

	if (option_project != PROJ_KERNEL)
		return;

	add_hook(match_iterator_statement, STMT_HOOK);
	add_hook(match_pre_statement, ASSIGNMENT_HOOK_AFTER);
	add_hook(match_post_statement, ASSIGNMENT_HOOK);

	add_modification_hook(my_id, &set_watch);
}

Powered by blists - more mailing lists