2) process and decoding the device features.
2. There was also another implicit assumption that is
broken by
this patch. There could be a vdpa tool query of config via
vdpa_dev_net_config_fill()->vdpa_get_config_unlocked() that races
with the first vdpa_set_features() call from VMM e.g.
QEMU. Since
the S_FEATURES_OK blocking condition is removed, if the
vdpa tool
query occurs earlier than the first
set_driver_features() call from
VMM, the following code will treat the guest as legacy
and then
trigger an erroneous vdpa_set_features_unlocked(... ,
0) call to
the vdpa driver:
374 /*
375 * Config accesses aren't supposed to
trigger before
features are set.
376 * If it does happen we assume a legacy
guest.
377 */
378 if (!vdev->features_valid)
379 vdpa_set_features_unlocked(vdev, 0);
380 ops->get_config(vdev, offset, buf, len);
Depending on vendor driver's implementation, L380 may
either return
invalid config data (or invalid endianness if on BE) or
only config
fields that are valid in legacy layout. What's more
severe is that,
vdpa tool query in theory shouldn't affect feature
negotiation at
all by making confusing calls to the device, but now it
is possible
with the patch. Fixing this would require more delicate
work on the
other paths involving the cf_lock reader/write semaphore.
Not sure what you plan to do next, post the fixes for
both issues
and get the community review? Or simply revert the
patch in
question? Let us know.
The spec says:
The device MUST allow reading of any device-specific
configuration
field before FEATURES_OK is set by
the driver. This includes fields which are conditional on
feature bits,
as long as those feature bits are offered
by the device.
so whether FEATURES_OK should not block reading the device
config
space. vdpa_get_config_unlocked() will read the features, I
don't know
why it has a comment:
/*
* Config accesses aren't supposed to trigger
before features
are set.
* If it does happen we assume a legacy guest.
*/
This conflicts with the spec.
vdpa_get_config_unlocked() checks vdev->features_valid, if
not valid,
it will set the drivers_features 0, I think this intends to
prevent
reading random driver_features. This function does not hold
any locks,
and didn't change anything.
So what is the race?
You'll see the race if you keep 'vdpa dev config show ...'
running in a
tight loop while launching a VM with the vDPA device under query.
-Siwei
Thanks
Thanks,
-Siwei
On 8/12/2022 3:44 AM, Zhu Lingshan wrote:
Users may want to query the config space of a vDPA
device,
to choose a appropriate one for a certain guest.
This means the
users need to read the config space before
FEATURES_OK, and
the existence of config space contents does not
depend on
FEATURES_OK.
The spec says:
The device MUST allow reading of any device-specific
configuration
field before FEATURES_OK is set by the driver. This
includes
fields which are conditional on feature bits, as
long as those
feature bits are offered by the device.
Signed-off-by: Zhu Lingshan <[email protected]>
---
drivers/vdpa/vdpa.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 6eb3d972d802..bf312d9c59ab 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -855,17 +855,9 @@ vdpa_dev_config_fill(struct
vdpa_device
*vdev, struct sk_buff *msg, u32 portid,
{
u32 device_id;
void *hdr;
- u8 status;
int err;
down_read(&vdev->cf_lock);
- status = vdev->config->get_status(vdev);
- if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
- NL_SET_ERR_MSG_MOD(extack, "Features
negotiation not
completed");
- err = -EAGAIN;
- goto out;
- }
-
hdr = genlmsg_put(msg, portid, seq,
&vdpa_nl_family,
flags,
VDPA_CMD_DEV_CONFIG_GET);
if (!hdr) {
