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
