On 8/16/2022 4:41 PM, Michael S. Tsirkin wrote:
On Tue, Aug 16, 2022 at 04:29:04PM +0800, Zhu, Lingshan wrote:
On 8/16/2022 3:41 PM, Si-Wei Liu wrote:
Hi Michael,
I just noticed this patch got pulled to linux-next prematurely without
getting consensus on code review, am not sure why. Hope it was just an
oversight.
Unfortunately this introduced functionality regression to at least two
cases so far as I see:
1. (bogus) VDPA_ATTR_DEV_NEGOTIATED_FEATURES are inadvertently exposed and
displayed in "vdpa dev config show" before feature negotiation is done.
Noted the corresponding features name shown in vdpa tool is called
"negotiated_features" rather than "driver_features". I see in no way the
intended change of the patch should break this user level expectation
regardless of any spec requirement. Do you agree on this point?
I will post a patch for iptour2, doing:
1) if iprout2 does not get driver_features from the kernel, then don't show
negotiated features in the command output
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.
Yea well. On the other hand the spec also calls for features to be
used to detect legacy versus modern driver.
This part of the spec needs work generally.
so from what I see, there are no race conditions, if features
negotiation not done,
just assume the driver features are all zero, then return the device
config space contents.
It can do this even without this comment.
Please help correct me if I misunderstand these
Thanks
Zhu Lingshan
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?
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 <lingshan.zhu@xxxxxxxxx>
---
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) {
