Re: [PATCH] nvmet: add missing locks around nvmet_ns_revalidate

[Niels Dossche <dossche.niels@gmail.com>]

On 10/03/2022 06:10, Bart Van Assche wrote:
> On 3/9/22 16:24, Niels Dossche wrote:
>> The main focus of the analyzer is not on lockdep assertions actually.
>> It works roughly in the following way:
>> 1) The analyzer searches for *_lock and *_unlock calls in order to know which fields are locks.
>> 2) It searches wrappers for those lock and unlock calls (e.g. task_lock locks task_struct->alloc_lock)
>> 3) It determines which field accesses of the same struct type occur guarded by a lock (e.g. A->field guarded by A->lock). This is used to (try to) determine which fields need to be locked by which lock.
>> 4) It searches for violations by counting for each field access how many paths are guarded by the lock and how many are not. If the count of unguarded is way smaller than the count of guarded, then it is reported as a possible violation.
>>
>> The analysis works interprocedurally. It also uses Multi-Layer Type Analysis of K. Lu et al. in order to improve the global call graph with respect to indirect calls.
> 
> That sounds interesting but does that algorithm also cover initialization and cleanup code for which it is guaranteed that only a single thread accesses the data?
> 

This is indeed a problem with the algorithm. I already did some work to introduce heuristics which can detect initialization and cleanup functions in order to reduce the false positive rate. Currently the false positive rate is a little high, but I have plans to improve that.

>> The lockdep part of the analyzer is not more powerful than the clang assertions. To be honest, I'm not sure that it can be easily integrated into Clang itself. The analyzer currently uses LLVM bitcode files as an input.
> 
> This is not a big deal. I was asking about clang integration because it is more convenient to run a single tool (compiler) than two tools (compiler + static analyzer). As you may know the Linux kernel supports the __acquires(), __releases() and __must_hold() annotations that are recognized by the sparse static analyzer (https://sparse.docs.kernel.org/en/latest/). The clang annotations however are more powerful than the sparse annotations. Additionally, it seems to me that the popularity of 'sparse' is declining a bit.
> 
> Bart.
> 

Oh yeah, a single tool would indeed be very convenient! Maybe this is something I can look into for the future.

Thanks
Niels