Virtualization - Aug 2022 - [PATCH V5 4/6] vDPA: !FEATURES_OK should not block querying device config space

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?

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.

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 at intel.com>
> ---
>   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) {

On Tue, Aug 16, 2022 at 12:41:21AM -0700, 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?
> 
> 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.
> 
> Thanks,
> -Siwei
If we can get fixes that's good. If not I can apply a revert.
I'm on vacation next week, you guys will have the time
to figure out the best plan of action.

-- 
MST

On Tue, Aug 16, 2022 at 12:41:21AM -0700, 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?
> 
> 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.
> 
> Thanks,
> -Siwei
> 
I'm not sure who you are asking. I didn't realize this is so
controversial. If you feel it should be reverted I suggest
you post a revert patch with a detailed motivation and this
will get the discussion going.
It will also help if you stress whether you describe theoretical
issues or something observed in practice above
discussion does not make this clear.

-- 
MST