From 57b7ad7a7b33d1b6f2feeb8b693efab7492658cf Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Wed, 18 Feb 2009 14:52:15 +0300 Subject: [PATCH] Rewrite completely with new goal. This time instead of checking that we don't return negative with a lock held, we check that locks are consistent across all returns. Add a validation script as well. --- check_locking.c | 153 ++++++++++++++++++++++++++++-------------------- validation/sm_locking.c | 36 ++++++++++++ 2 files changed, 127 insertions(+), 62 deletions(-) create mode 100644 validation/sm_locking.c diff --git a/check_locking.c b/check_locking.c index fd8c15f1..5fff84e2 100644 --- a/check_locking.c +++ b/check_locking.c @@ -8,13 +8,10 @@ */ /* - * For this test let's look for functions that return a negative value - * with a spinlock held. - * - * One short coming is that it assumes a function isn't supposed - * to return negative with a lock held. Perhaps the function was - * called with the lock held. A more complicated script could check that. - * + * This test checks that locks are held the same across all returns. + * + * Of course, some functions are designed to only hold the locks on success. + * Oh well... We can rewrite it later if we want. */ #include "parse.h" @@ -70,26 +67,19 @@ static struct locked_call lock_needed[] = { static int my_id; -STATE(locked); -STATE(unlocked); +static struct tracker_list *starts_locked; +static struct tracker_list *starts_unlocked; -/* - * merge_func() can go away when we fix the core to just store all the possible - * states. - * - * The parameters are passed in alphabetical order with NULL at the beginning - * of the alphabet. (s2 is never NULL). - */ - -static struct smatch_state *merge_func(const char *name, struct symbol *sym, - struct smatch_state *s1, - struct smatch_state *s2) -{ - if (s1 == NULL) - return s2; - return &undefined; +struct locks_on_return { + int line; + struct tracker_list *locked; + struct tracker_list *unlocked; +}; +DECLARE_PTR_LIST(return_list, struct locks_on_return); +static struct return_list *all_returns; -} +STATE(locked); +STATE(unlocked); static char kernel[] = "kernel"; static char *match_lock_func(char *fn_name, struct expression_list *args) @@ -133,7 +123,7 @@ static void check_locks_needed(const char *fn_name) if (!strcmp(fn_name, lock_needed[i].function)) { state = get_state(lock_needed[i].lock, my_id, NULL); if (state != &locked) { - smatch_msg("%s called without holding %s lock", + smatch_msg("%s called without holding '%s' lock", lock_needed[i].function, lock_needed[i].lock); } @@ -150,11 +140,15 @@ static void match_call(struct expression *expr) if (!fn_name) return; - if ((lock_name = match_lock_func(fn_name, expr->args))) + if ((lock_name = match_lock_func(fn_name, expr->args))) { + if (!get_state(lock_name, my_id, NULL)) + add_tracker(&starts_unlocked, lock_name, my_id, NULL); set_state(lock_name, my_id, NULL, &locked); - else if ((lock_name = match_unlock_func(fn_name, expr->args))) + } else if ((lock_name = match_unlock_func(fn_name, expr->args))) { + if (!get_state(lock_name, my_id, NULL)) + add_tracker(&starts_locked, lock_name, my_id, NULL); set_state(lock_name, my_id, NULL, &unlocked); - else + } else check_locks_needed(fn_name); free_string(fn_name); return; @@ -165,57 +159,92 @@ static void match_condition(struct expression *expr) /* __raw_spin_is_locked */ } -static int possibly_negative(struct expression *expr) +static struct locks_on_return *alloc_return(int line) +{ + struct locks_on_return *ret; + + ret = malloc(sizeof(*ret)); + ret->line = line; + ret->locked = NULL; + ret->unlocked = NULL; + return ret; +} + +static void match_return(struct statement *stmt) { - char *name; - struct symbol *sym; + struct locks_on_return *ret; struct state_list *slist; struct sm_state *tmp; + + ret = alloc_return(get_lineno()); - name = get_variable_from_expr(expr, &sym); - if (!name || !sym) - return 0; - slist = get_possible_states(name, SMATCH_EXTRA, sym); + slist = get_all_states(my_id); FOR_EACH_PTR(slist, tmp) { - int value = 0; - - if (tmp->state->data) - value = *(int *)tmp->state->data; - - if (value < 0) { - return 1; + if (tmp->state == &locked) { + add_tracker(&ret->locked, tmp->name, tmp->owner, + tmp->sym); + } else if (tmp->state == &unlocked) { + add_tracker(&ret->unlocked, tmp->name, tmp->owner, + tmp->sym); + } else { + smatch_msg("Unclear if '%s' is locked or unlocked.", + tmp->name); } } END_FOR_EACH_PTR(tmp); - return 0; + add_ptr_list(&all_returns, ret); } -static void match_return(struct statement *stmt) +static void check_returns_consistently(struct tracker *lock, + struct smatch_state *start) { - int ret_val; - struct state_list *slist; - struct sm_state *tmp; + int returns_locked = 0; + int returns_unlocked = 0; + struct locks_on_return *tmp; + + FOR_EACH_PTR(all_returns, tmp) { + if (in_tracker_list(tmp->unlocked, lock->name, lock->owner, + lock->sym)) + returns_unlocked = tmp->line; + else if (in_tracker_list(tmp->locked, lock->name, lock->owner, + lock->sym)) + returns_locked = tmp->line; + else if (start == &locked) + returns_locked = tmp->line; + else if (start == &unlocked) + returns_unlocked = tmp->line; + } END_FOR_EACH_PTR(tmp); - ret_val = get_value(stmt->ret_value); - if (ret_val >= 0) { - return; - } - if (ret_val == UNDEFINED) { - if (!possibly_negative(stmt->ret_value)) - return; - } + if (returns_locked && returns_unlocked) + smatch_msg("Lock '%s' held on line %d but not on %d.", + lock->name, returns_locked, returns_unlocked); - slist = get_all_states(my_id); - FOR_EACH_PTR(slist, tmp) { - if (tmp->state != &unlocked) - smatch_msg("returned negative with %s lock held", - tmp->name); +} + +static void check_consistency(struct symbol *sym) +{ + struct tracker *tmp; + + FOR_EACH_PTR(starts_locked, tmp) { + if (in_tracker_list(starts_unlocked, tmp->name, tmp->owner, + tmp->sym)) + smatch_msg("Locking inconsistency. We assume '%s' is " + "both locked and unlocked at the start.", + tmp->name); + } END_FOR_EACH_PTR(tmp); + + FOR_EACH_PTR(starts_locked, tmp) { + check_returns_consistently(tmp, &locked); + } END_FOR_EACH_PTR(tmp); + + FOR_EACH_PTR(starts_unlocked, tmp) { + check_returns_consistently(tmp, &unlocked); } END_FOR_EACH_PTR(tmp); } void register_locking(int id) { my_id = id; - add_merge_hook(my_id, &merge_func); add_hook(&match_call, FUNCTION_CALL_HOOK); add_hook(&match_return, RETURN_HOOK); + add_hook(&check_consistency, END_FUNC_HOOK); } diff --git a/validation/sm_locking.c b/validation/sm_locking.c new file mode 100644 index 00000000..ac53ec25 --- /dev/null +++ b/validation/sm_locking.c @@ -0,0 +1,36 @@ +_spin_lock(int name); +_spin_unlock(int name); + +int a; +int b; +int func (void) +{ + int mylock = 1; + int mylock2 = 1; + int mylock3 = 1; + + if (a) { + return; + } + + _spin_lock(mylock); + _spin_unlock(mylock); + + if (b) { + _spin_unlock(mylock2); + return; + } + + if (a) + _spin_lock(mylock3); + return; +} +/* + * check-name: Locking inconsistencies + * check-command: smatch sm_locking.c + * + * check-output-start +sm_locking.c +26 func(20) Unclear if 'mylock3' is locked or unlocked. +sm_locking.c +26 func(20) Lock 'mylock2' held on line 26 but not on 21. + * check-output-end + */ -- 2.11.4.GIT