On Tue, Feb 16, 2016 at 07:26:58PM -0500, Oleg Drokin wrote:
>
> On Feb 16, 2016, at 3:53 PM, Dan Carpenter wrote:
>
> > I have re-written your test.c file and written a patch:
>
> Thanks!
>
> It seems your standalone test app does not trigger the error either with or without the patch, though?
> (before the patch)
> $ ./smatch -p=kernel lock_test.c
> lock_test.c:18 test_func() [check_locking] 'mutex:&a_mutex' = 'merged' (locked, unlocked, merged)
>
> (after the patch):
> $ ./smatch -p=kernel lock_test.c
> lock_test.c:17 test_func() [check_locking] 'mutex:&a_mutex' = 'merged' (unlocked, impossible, merged)
>
Those warnings are disabled unless --spammy is turned on.
./smatch -p=kernel --spammy lock_test.c
>
> > diff --git a/check_locking.c b/check_locking.c
> > index d74ba9e..0d9ae01 100644
> > --- a/check_locking.c
> > +++ b/check_locking.c
> > @@ -38,6 +38,7 @@ static int func_has_transition;
> > STATE(locked);
> > STATE(start_state);
> > STATE(unlocked);
> > +STATE(impossible);
>
> > +static void pre_merge_hook(struct sm_state *sm)
> > +{
> > + if (is_impossible_path())
> > + set_state(my_id, sm->name, sm->sym, &impossible);
>
> Ah, unchecked for state to override whatever the state was assumed for that path - sneaky ;)
> Luckily there's no printing of where we locked as that merge would totally throw it off.
>
> Anyway, while this test case is fixed, I run it across our codebase and noticed that one
> true positive has been silenced too.
>
> void up(struct semaphore *);
> void down(struct semaphore *);
>
> int some_random_stuff(void);
>
> void test_func2(void)
> {
> struct semaphore a_sem;
>
> if (some_random_stuff)
> goto out;
This should be a function call:
if (some_random_stuff())
goto out;
Or it marks the rest as &impossible.
>
> up(&a_sem);
Did you mean down()?
>
> for (;;) {
> if (some_random_stuff)
> break;
> }
>
> out:
> up(&a_sem);
> __smatch_states("check_locking");
The state is obviously going to be unlocked because we unlocked on the
line before. The state before the up() is &merged (&start_state +
&impossible) If I change the condition then it's (&start_state +
&unlocked) so it generates a double unlock warning.
If you unlock a (&start_state + &unlocked) then it probably should
generate a warning but it doesn't... Check_locking is one of the oldest
remaining checks and it needs to be re-written.
Thanks for the help on getting lustre to compile. I'm looking at the
warning:
lustre/llite/llite_lib.c:1709 ll_setattr_raw() warn: 'mutex:&inode->i_mutex' is sometimes locked here and sometimes unlocked.
I see a bug in the flow analysis handling:
if ((((inode->i_mode) & S_IFMT) == S_IFREG))
The smatch_stored_conditions.c file ignores comparisons because
smatch_extra.c is supposed to handle those. But smatch_extra.c doesn't
handle it either...
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe smatch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
