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

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.

Thanks