On Feb 19, 2016, at 2:49 AM, Dan Carpenter wrote:
> On Thu, Feb 18, 2016 at 05:35:47PM -0500, Oleg Drokin wrote:
>>
>> On Feb 18, 2016, at 5:04 PM, Dan Carpenter wrote:
>>
>>> On Wed, Feb 17, 2016 at 10:47:45AM -0500, Oleg Drokin wrote:
>>>> Does not matter, I noticed the lack of the function call and changed it to be one and did not
>>>> see any difference in smatch output.
>>>>
>>>
>>> For me it works... I get a double unlock bug.
>>
>> I think we are talking about different things.
>> I get double unlock bug too every time. Unless your patch is applied and then I don't.
> I'm totally lost. What do you want it to do?
I think the code with double up should report as a bug (double unlock).
Your patch (the one about checking impossible state) silences it which makes me think
some other true positives would be silenced too?
The testcase I came up with does not work, but the original code still shows this, that's how I noticed it.
>>>> After all this is just a small attempt at distilling down the actual real world code
>>>> (see live example in drivers/staging/lustre/lustre/llite/lloop.c::loop_thread() )
>>>
>>> It's saying that that loop is a forever loop because we set
>>> "lo->lo_state = LLOOP_BOUND;" at the start of the function and only
>>> exit when it is set to LLOOP_RUNDOWN which I guess hapens in another
>>> thread?
>>>
>>> Smatch doesn't understand about threads... I don't even have a roadmap
>>> in my head how to handle this situation.
>>
>> I understand.
>> That's why it should always be complaining about the double unlock, right? With your patch
>> that tracks impossible states it no longer does this.
>>
>
> No. It's working (if you ignore threads).
>
> // not locked.
>
> goto out;
>
> up();
>
> // state is unlocked
>
> for (;;) {
>
> if (impossible)
> break;
huh? state is "true" here, right? Because if we go by my original code, it's a function pointer so should not be NULL.
if we convert it to a functional call, it's also possible, though not always.
Seems there's an unrelated bug somewhere that assumes a pointer is always false or something like that
(if we make the condition into "if (!some_random_stuff)" then the warning comes back and the merged state is suddenly unlocked
where it should be impossible instead?)
I guess my distilled test failed to live up to the expectations, but the original code in lloop.c is still affected.
> }
>
> // state is impossible
But should be unlocked - we know that some_random_stuff is not NULL, i.e. evaluates as true.
> out:
> // state is merged (impossible + start_state) so calling up is
> // fine
> up();
>>>>> 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.
>>>> I actually noticed it has several modes of operation, sometimes just warning about
>>>> inconsistent locking and sometimes actually elaborating that here, here and here
>>>> we return locking, and there, thee and there we return unlocked.
>>> Yeah. It's not beautiful. The main thing though is that it doesn't do
>>> cross function analysis…
>>
>> The other parts of the code do, as in they know what function changes
>> what, don't they?
>
> Yeah. Because the states are named after variables but for check
> the states are named "mutex:&var".
Well, I mostly meant the regular variables, not locks, to more accurately trace possible/impossible
paths.
--
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
