[PATCH] nvmet: add missing locks around nvmet_ns_revalidate

[PATCH] nvmet: add missing locks around nvmet_ns_revalidate

[Niels Dossche]

nvmet_ns_changed states via lockdep that the ns->subsys->lock must be
held. The only caller of nvmet_ns_changed which does not acquire that
lock is nvmet_ns_revalidate. The only 2 callers of nvmet_ns_revalidate
which do not acquire that lock are nvmet_execute_identify_cns_cs_ns and
nvmet_execute_identify_ns. Add a lock for around the call to
nvmet_ns_revalidate in those 2 functions.

Both of those identify functions are called from a common function
nvmet_execute_identify, which itself is called indirectly via the
req->execute function pointer.

Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
---
 drivers/nvme/target/admin-cmd.c | 2 ++
 drivers/nvme/target/zns.c       | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 6fb24746de06..6d1f5e02d27b 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -511,7 +511,9 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 		goto done;
 	}
 
+	mutex_lock(&req->ns->subsys->lock);
 	nvmet_ns_revalidate(req->ns);
+	mutex_unlock(&req->ns->subsys->lock);
 
 	/*
 	 * nuse = ncap = nsze isn't always true, but we have no way to find
diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c
index 46bc30fe85d2..5182b802de27 100644
--- a/drivers/nvme/target/zns.c
+++ b/drivers/nvme/target/zns.c
@@ -123,7 +123,10 @@ void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req)
 		goto done;
 	}
 
+	mutex_lock(req->ns->subsys->lock);
 	nvmet_ns_revalidate(req->ns);
+	mutex_unlock(req->ns->subsys->lock);
+
 	zsze = (bdev_zone_sectors(req->ns->bdev) << 9) >>
 					req->ns->blksize_shift;
 	id_zns->lbafe[0].zsze = cpu_to_le64(zsze);
-- 
2.35.1

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

[Bart Van Assche]

On 3/9/22 12:34, Niels Dossche wrote:
> nvmet_ns_changed states via lockdep that the ns->subsys->lock must be
> held. The only caller of nvmet_ns_changed which does not acquire that
> lock is nvmet_ns_revalidate. The only 2 callers of nvmet_ns_revalidate
> which do not acquire that lock are nvmet_execute_identify_cns_cs_ns and
> nvmet_execute_identify_ns. Add a lock for around the call to
> nvmet_ns_revalidate in those 2 functions.
> 
> Both of those identify functions are called from a common function
> nvmet_execute_identify, which itself is called indirectly via the
> req->execute function pointer.

Please mention in the patch description whether this has been discovered 
by studying the source code or by software (static source code analyzer? 
runtime data race detector?).

Thanks,

Bart.

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

[Niels Dossche]

Hi,

On 09/03/2022 23:27, Bart Van Assche wrote:
> On 3/9/22 12:34, Niels Dossche wrote:
>> nvmet_ns_changed states via lockdep that the ns->subsys->lock must be
>> held. The only caller of nvmet_ns_changed which does not acquire that
>> lock is nvmet_ns_revalidate. The only 2 callers of nvmet_ns_revalidate
>> which do not acquire that lock are nvmet_execute_identify_cns_cs_ns and
>> nvmet_execute_identify_ns. Add a lock for around the call to
>> nvmet_ns_revalidate in those 2 functions.
>>
>> Both of those identify functions are called from a common function
>> nvmet_execute_identify, which itself is called indirectly via the
>> req->execute function pointer.
> 
> Please mention in the patch description whether this has been discovered by studying the source code or by software (static source code analyzer? runtime data race detector?).

This was discovered by first using a static analyzer and then verifying it by manual inspection of the source code.

> 
> Thanks,
> 
> Bart.

Thanks,
Niels

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

[Bart Van Assche]

On 3/9/22 14:30, Niels Dossche wrote:
> On 09/03/2022 23:27, Bart Van Assche wrote:
>> On 3/9/22 12:34, Niels Dossche wrote:
>>> nvmet_ns_changed states via lockdep that the ns->subsys->lock
>>> must be held. The only caller of nvmet_ns_changed which does not
>>> acquire that lock is nvmet_ns_revalidate. The only 2 callers of
>>> nvmet_ns_revalidate which do not acquire that lock are
>>> nvmet_execute_identify_cns_cs_ns and nvmet_execute_identify_ns.
>>> Add a lock for around the call to nvmet_ns_revalidate in those 2
>>> functions.
>>> 
>>> Both of those identify functions are called from a common
>>> function nvmet_execute_identify, which itself is called
>>> indirectly via the req->execute function pointer.
>> 
>> Please mention in the patch description whether this has been
>> discovered by studying the source code or by software (static
>> source code analyzer? runtime data race detector?).
> 
> This was discovered by first using a static analyzer and then
> verifying it by manual inspection of the source code.

Hi Niels,

Are there any plans to make that static analyzer available to other 
kernel developers?

Is the static analyzer more powerful than clang thread safety 
annotations? If it is more powerful, is it possible to integrate the 
static analyzer in clang?

See also:
* "C/C++ Thread Safety Analysis" 
(https://static.googleusercontent.com/media/research.google.com/en//pubs/archive/42958.pdf).
* "Thread Safety Annotations for Clang" 
(https://llvm.org/devmtg/2011-11/Hutchins_ThreadSafety.pdf).

Thanks,

Bart.

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

[Niels Dossche]

On 10/03/2022 00:12, Bart Van Assche wrote:
> On 3/9/22 14:30, Niels Dossche wrote:
>> On 09/03/2022 23:27, Bart Van Assche wrote:
>>> On 3/9/22 12:34, Niels Dossche wrote:
>>>> nvmet_ns_changed states via lockdep that the ns->subsys->lock
>>>> must be held. The only caller of nvmet_ns_changed which does not
>>>> acquire that lock is nvmet_ns_revalidate. The only 2 callers of
>>>> nvmet_ns_revalidate which do not acquire that lock are
>>>> nvmet_execute_identify_cns_cs_ns and nvmet_execute_identify_ns.
>>>> Add a lock for around the call to nvmet_ns_revalidate in those 2
>>>> functions.
>>>>
>>>> Both of those identify functions are called from a common
>>>> function nvmet_execute_identify, which itself is called
>>>> indirectly via the req->execute function pointer.
>>>
>>> Please mention in the patch description whether this has been
>>> discovered by studying the source code or by software (static
>>> source code analyzer? runtime data race detector?).
>>
>> This was discovered by first using a static analyzer and then
>> verifying it by manual inspection of the source code.
> 
> Hi Niels,
> 
> Are there any plans to make that static analyzer available to other kernel developers?
> 
> Is the static analyzer more powerful than clang thread safety annotations? If it is more powerful, is it possible to integrate the static analyzer in clang?
> 
> See also:
> * "C/C++ Thread Safety Analysis" (https://static.googleusercontent.com/media/research.google.com/en//pubs/archive/42958.pdf).
> * "Thread Safety Annotations for Clang" (https://llvm.org/devmtg/2011-11/Hutchins_ThreadSafety.pdf).
> 
> Thanks,
> 
> Bart.

Hi Bart,

I'm currently developing this static analyzer as part of my master's thesis in order to obtain my master's degree.
The analyzer is currently still a work-in-progress.
I plan on making the source code of this analyzer available when my thesis is finished, which should be sometime in June.

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.

The aforementioned methodology tries to obtain knowledge about which field requires which lock, without using assertions or annotations. I manually inspect the cases this analyzer outputs in order to determine whether the reported case is a false positive or a true positive. I have submitted patches for some of the true positives over the past few weeks, of which some patches have already been accepted into the mainline or linux-next.

I recently submitted a patch for a case which was missing a lock, but had a lockdep assertions. So that got me wondering whether there are more cases that have a lockdep assertion but don't actually have the expected lock acquired. I added the ability for the analyzer to leverage information about the lockdep assertions. The patch in this email thread was to fix a case that it found. I checked manually too to be extra certain.

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.

Thanks,
Niels

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

[Bart Van Assche]

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?

> 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.

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

[Niels Dossche]

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

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

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

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

[Niels Dossche]

On 3/13/22 13:43, Sagi Grimberg wrote:
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Hi, thanks for the review.
This patch is actually superseded by a v3 patch submission I sent shortly after this one, since this contains a mistake and its solution is not optimal.
Sorry for the inconvenience.
Thanks

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

[Sagi Grimberg]

>> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> 
> Hi, thanks for the review.
> This patch is actually superseded by a v3 patch submission I sent shortly after this one, since this contains a mistake and its solution is not optimal.
> Sorry for the inconvenience.

Yes, noticed that, thanks