On 12/21/2021 9:55 PM, Eli Cohen wrote:
On Tue, Dec 21, 2021 at 05:55:56PM -0800, Si-Wei Liu wrote:
quoted
On 12/21/2021 9:19 AM, Eli Cohen wrote:
quoted
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 <redacted>
---
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.
quoted
-Siwei
quoted
+ 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)
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization