[PATCH v2] nvmet: add missing locks around nvmet_ns_revalidate

[PATCH v2] 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.

This issue was found using a static type-based analyser and manually
verified.

Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
---

Changes in v2:
 - added sentence about how the issue was found.
 - added missing &

 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..56e650a84bfa 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 v2] nvmet: add missing locks around nvmet_ns_revalidate

[Chaitanya Kulkarni]

On 3/9/22 17:25, 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.
> 
> This issue was found using a static type-based analyser and manually
> verified.
> 
> Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
> ---
> 
> Changes in v2:
>   - added sentence about how the issue was found.
>   - added missing &
> 


Thanks, looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

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

[Christoph Hellwig]

On Thu, Mar 10, 2022 at 02:25:11AM +0100, 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.

So acquire it in nvmet_ns_revalidate only when we actually call
nvmet_ns_changed.  Otherwise we take a subsystem-wide lock for every
Identify Namespace all.

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

[Niels Dossche]

On 10/03/2022 07:38, Christoph Hellwig wrote:
> On Thu, Mar 10, 2022 at 02:25:11AM +0100, 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.
> 
> So acquire it in nvmet_ns_revalidate only when we actually call
> nvmet_ns_changed.  Otherwise we take a subsystem-wide lock for every
> Identify Namespace all.

There are 3 callers of nvmet_ns_revalidate, of which 2 do not acquire the lock and 1 does.
I'll send a v3 patch which does the locking locally inside that if check by adding an additional argument to indicate whether the lock should be acquired or not.
Thanks.