smatch.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Gordon Ross <gordon.w.ross@gmail.com>
Cc: smatch@vger.kernel.org
Subject: new locking check
Date: Mon, 30 May 2022 08:49:28 +0300	[thread overview]
Message-ID: <20220530054928.GT2168@kadam> (raw)

Ross was asking about where the mailing list was, and how to write a
different kind of locking check where we want to ensure that a given
lock is held when we access a struct member.

I've CC'd the list so the first part of the question is answered.  :)
The second part of the question is more difficult.

The check_locking.c file does cross function analysis.  When a function
is called it passes the list of locks held.  If all the callers are
holding a lock then the state is &locked.  If only some of the callers
are holding a lock then it's &half_locked.

You need to rebuild the cross function database a bunch of times to
populate the database.  Each time you rebuild it, then it uses the data
that it has and adds one more level to the call tree.  Generally I tell
people that rebuilding the DB five times makes it fully populated.

So then you want to implement the new heuristic:
1) Add a hook for dereferences.  Which is actually a PREOP_EXPR and
   expr->op == *.  It's an ancient ugliness that EXPR_DEREF is not what
   you want.  I need to clean that up.

	add_hook(&match_dereferences, DEREF_HOOK);

2) Get the struct member name, and the lock that we want:

static void match_dereferences(struct expression *expr)
{
	char *member_name, *expected_lock;

	if (expr->type != EXPR_PREOP)
		return;

	if (__in_fake_assign || __in_fake_parameter_assign)
		return;

	member_name = get_member_name(expr->unop);
	if (!member_name)
		return;
	expected_lock = get_expected_lock(member_name);
	if (!expect_lock)
		return;

	...

3) Search through all locked states and check if our lock is held:

static bool lock_is_held(const char *lock)
{
	struct sm_state *sm;
	static int lock_id;

	if (!lock_id)
		lock_id = id_from_name("check_locking");

	FOR_EACH_MY_SM(lock_id, __get_cur_stree(), sm) {
		if (strcmp(sm->state->name, "locked") != 0 &&
		    strcmp(sm->state->name, "half_locked") != 0)
			continue;
		if (strstr(sm->name, lock))
			return true;
	} END_FOR_EACH_SM(sm);

	return false;
}

4) Print the warning:

	if (!lock_is_held(expected_lock))
		sm_warning("struct member '%s' used without '%s'", member_name, expected_lock);

There are a few problems:

The first problem is that whenever you write an ambitious Smatch check
there are always issues.  You can't predict in advance what the issues
will be.  I'm always help out though, so write test case or whatever
and send to the list.

The other problem is that the code likely has special cases like
probe where we know the structure hasn't been exported yet so it
won't race.  Or we know that we're holding a different lock so we don't
need to take the other regular one.

One idea is that instead of printing a warning, you add that to a list
of suspicous exprssions paired with the lock.  Then if anyone takes the
lock later in the function, print the warning at that point.

	add_check_tracker("check_locking", &locking_hook);

static void locking_hook(int owner, const char *name, struct symbol *sym, struct smatch_state *state)
{
        struct state_list *slist = NULL;
        struct sm_state *sm;

        if (strcmp(state->name, "locked") != 0) {
                clear_suspicious_pairs(name);
                return;
        }

	print_all_warnings(name);
        clear_suspicious_pairs(name);
}

regards,
dan carpenter

                 reply	other threads:[~2022-05-30  5:49 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

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=20220530054928.GT2168@kadam \
    --to=dan.carpenter@oracle.com \
    --cc=gordon.w.ross@gmail.com \
    --cc=smatch@vger.kernel.org \
    /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

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 read-only IMAP folder(s) and NNTP newsgroup(s).