[PATCH v3] nvmet: add missing lock around nvmet_ns_changed in nvmet_ns_revalidate

[Sagi Grimberg]

On 3/10/22 14:51, 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. nvmet_ns_revalidate has 3 callers, of which
> 2 do not acquire that lock: nvmet_execute_identify_cns_cs_ns and
> nvmet_execute_identify_ns. The other caller
> nvmet_ns_revalidate_size_store does acquire the lock. Add a parameter to
> nvmet_ns_revalidate to indicate whether the lock was already taken or
> not, and thus whether the function still needs to take a lock when
> calling nvmet_ns_changed.
> 
> The alternative solution is to let nvmet_ns_revalidate return a bool
> which indicates whether nvmet_ns_changed needs to be called and let the
> callers handle the locking responsibility. This however places the
> responsibility with its callers and causes more duplicate code and
> potential to forget to check the return value.
> 
> 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 at gmail.com>
> ---
> 
> Changes in v3:
>   - improve commit description
>   - do the locking locally
> 
> Changes in v2:
>   - added sentence about how the issue was found.
>   - added missing &
> 
>   drivers/nvme/target/admin-cmd.c | 2 +-
>   drivers/nvme/target/configfs.c  | 2 +-
>   drivers/nvme/target/core.c      | 9 +++++++--
>   drivers/nvme/target/nvmet.h     | 2 +-
>   drivers/nvme/target/zns.c       | 3 ++-
>   5 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 6fb24746de06..efa462374783 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -511,7 +511,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
>   		goto done;
>   	}
>   
> -	nvmet_ns_revalidate(req->ns);
> +	nvmet_ns_revalidate(req->ns, true);
>   
>   	/*
>   	 * nuse = ncap = nsze isn't always true, but we have no way to find
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 091a0ca16361..a803cd66dc4b 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -586,7 +586,7 @@ static ssize_t nvmet_ns_revalidate_size_store(struct config_item *item,
>   		mutex_unlock(&ns->subsys->lock);
>   		return -EINVAL;
>   	}
> -	nvmet_ns_revalidate(ns);
> +	nvmet_ns_revalidate(ns, false);
>   	mutex_unlock(&ns->subsys->lock);
>   	return count;
>   }
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 5119c687de68..0ceef97e4093 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -531,7 +531,7 @@ static void nvmet_p2pmem_ns_add_p2p(struct nvmet_ctrl *ctrl,
>   		ns->nsid);
>   }
>   
> -void nvmet_ns_revalidate(struct nvmet_ns *ns)
> +void nvmet_ns_revalidate(struct nvmet_ns *ns, bool should_acquire_lock)
>   {
>   	loff_t oldsize = ns->size;
>   
> @@ -540,8 +540,13 @@ void nvmet_ns_revalidate(struct nvmet_ns *ns)
>   	else
>   		nvmet_file_ns_revalidate(ns);
>   
> -	if (oldsize != ns->size)
> +	if (oldsize != ns->size) {
> +		if (should_acquire_lock)
> +			mutex_lock(&ns->subsys->lock);
>   		nvmet_ns_changed(ns->subsys, ns->nsid);
> +		if (should_acquire_lock)
> +			mutex_unlock(&ns->subsys->lock);
> +	}

What is the harm locking it always and avoid the conditional?