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

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

On 3/13/22 14:31, Sagi Grimberg wrote:
> 
> 
> 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.
> 

Alright, I can send in a patch that does it like that.

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

The callsite that has the lock is nvmet_ns_revalidate_size_store. It checks for the enabled flag under that lock.
If nvmet_ns_revalidate_size_store calls nvmet_ns_revalidate without that lock taken, but with the lock acquired inside the nvmet_ns_revalidate_size_store function itself, is it not possible that the ns->enabled flag changes in between the ns->enabled check and the call to nvmet_ns_revalidate_size_store? I thought the locking in that function was to also make sure that the enabled flag does not change during the execution of nvmet_ns_revalidate?