Virtualization - Dec 2021 - [PATCH v5 04/13] vdpa: Read device configuration only if FEATURES_OK

On 12/21/2021 9:55 PM, Eli Cohen wrote:
> On Tue, Dec 21, 2021 at 05:55:56PM -0800, Si-Wei Liu wrote:
>>
>> On 12/21/2021 9:19 AM, Eli Cohen wrote:
>>> Avoid reading device configuration during feature negotiation. Read
>>> device status and verify that VIRTIO_CONFIG_S_FEATURES_OK is set.
>>>
>>> Otherwise, return -EAGAIN.
>>>
>>> Signed-off-by: Eli Cohen <elic at nvidia.com>
>>> ---
>>>    drivers/vdpa/vdpa.c | 10 ++++++++++
>>>    1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>> index 5134c83c4a22..3285cebd89c0 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -838,8 +838,18 @@ vdpa_dev_config_fill(struct vdpa_device *vdev,
struct sk_buff *msg, u32 portid,
>>>    {
>>>    	u32 device_id;
>>>    	void *hdr;
>>> +	u8 status;
>>>    	int err;
>>> +	mutex_lock(&vdev->cf_mutex);
>>> +	status = vdev->config->get_status(vdev);
>> Atomicity and data consistency not guaranteed against vdpa_get_config
and
>> get_features in vdpa_dev_net_config_fill(). Need to use coarse locking.
> Your concern is that vdpa_get_config() is vdpa_dev_net_config_fill
> () is not being read under the lock?
Not exactly. vdpa_get_config() is already in the cf_mutex lock. However, 
the config data may become invalid getting out of the FEATURES_OK check, 
as theoretically it's possible the guest could change the config via 
set_config(), set_status(), or reset() in between. You'd need to use a 
single cf_mutex to protect almost the whole code block in 
vdpa_dev_config_fill() to ensure data consistency.

-Siwei
>
> I will put it under the lock.
>
>> -Siwei
>>> +	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
>>> +		NL_SET_ERR_MSG_MOD(extack, "Features negotiation not
completed");
>>> +		mutex_unlock(&vdev->cf_mutex);
>>> +		return -EAGAIN;
>>> +	}
>>> +	mutex_unlock(&vdev->cf_mutex);
>>> +
>>>    	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>>>    			  VDPA_CMD_DEV_CONFIG_GET);
>>>    	if (!hdr)