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

[Sagi Grimberg]

On 3/13/22 15:14, Niels Dossche wrote:
> On 3/13/22 14:03, Sagi Grimberg wrote:
>>
>>
>> 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?
> 
> In my patch v2 submission I wrote the following text in my commit message:
>> 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.
> on which Christoph Hellwig replied:
>> 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.

Yea, only wrap nvmet_ns_changed, but always.

> 
> Therefore, I changed it to a conditional lock in this patch submission.
> 
> My commit message in v2 did not clearly state that 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. Maybe I caused some confusion because of the unclear wording.

It is simpler to just move that call-site outside of the lock imo.