| Message ID | 20220812104500.163625-5-lingshan.zhu@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | ifcvf/vDPA: support query device config space through netlink | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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@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.
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. > 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@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 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@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 > 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.
On 8/16/2022 2:13 PM, Michael S. Tsirkin wrote: > 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. Leave it around then, until the next person shout out aloud. I don't mind taking personal time to help, though my impression of the past conversation is that this is less productive way of cooperation and collaboration. Please safely ignore me from now on. -Siwei > It will also help if you stress whether you describe theoretical > issues or something observed in practice above > discussion does not make this clear. >
On Wed, Aug 17, 2022 at 10:11:36AM +0800, Zhu, Lingshan wrote: > > > On 8/17/2022 6:48 AM, Si-Wei Liu wrote: > > > > On 8/16/2022 1:29 AM, 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 > > This won't work as the vdpa userspace tool won't know *when* features are > negotiated. There's no guarantee in the kernel to assume 0 will be returned > from vendor driver during negotiation. On the other hand, with the supposed > change, userspace can't tell if there's really none of features negotiated, > or the feature negotiation is over. Before the change the userspace either > gets all the attributes when feature negotiation is over, or it gets > nothing when it's ongoing, so there was a distinction.This expectation of > what "negotiated_features" represents is established from day one, I see no > reason the intended kernel change to show other attributes should break > userspace behavior and user's expectation. > > User space can only read valid *driver_features* after the features negotiation > is done, *device_features* does not require the negotiation. > > If you want to prevent random values read from driver_features, here I propose > a fix: only read driver_features when the negotiation is done, this means to > check (status & VIRTIO_CONFIG_S_FEATURES_OK) before reading the > driver_features. > Sounds good? > > @MST, if this is OK, I can include this change in my next version patch series. > > Thanks, > Zhu Lingshan Sorry I don't get it. Is there going to be a new version? Do you want me to revert this one and then apply a new one? It's ok if yes. > 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 <lingshan.zhu@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 8/17/2022 2:14 PM, Michael S. Tsirkin wrote: > On Wed, Aug 17, 2022 at 10:11:36AM +0800, Zhu, Lingshan wrote: >> >> On 8/17/2022 6:48 AM, Si-Wei Liu wrote: >> >> >> >> On 8/16/2022 1:29 AM, 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 >> >> This won't work as the vdpa userspace tool won't know *when* features are >> negotiated. There's no guarantee in the kernel to assume 0 will be returned >> from vendor driver during negotiation. On the other hand, with the supposed >> change, userspace can't tell if there's really none of features negotiated, >> or the feature negotiation is over. Before the change the userspace either >> gets all the attributes when feature negotiation is over, or it gets >> nothing when it's ongoing, so there was a distinction.This expectation of >> what "negotiated_features" represents is established from day one, I see no >> reason the intended kernel change to show other attributes should break >> userspace behavior and user's expectation. >> >> User space can only read valid *driver_features* after the features negotiation >> is done, *device_features* does not require the negotiation. >> >> If you want to prevent random values read from driver_features, here I propose >> a fix: only read driver_features when the negotiation is done, this means to >> check (status & VIRTIO_CONFIG_S_FEATURES_OK) before reading the >> driver_features. >> Sounds good? >> >> @MST, if this is OK, I can include this change in my next version patch series. >> >> Thanks, >> Zhu Lingshan > Sorry I don't get it. Is there going to be a new version? Do you want me > to revert this one and then apply a new one? It's ok if yes. Not a new version, it is a new patch, though I still didn't get the race condition, but I believe it is reasonable to block reading the *driver_features* before FEATURES_OK. So, I added code to check whether _FEATURES_OK is set: 861 /* only read driver features after the feature negotiation is done */ 862 status = vdev->config->get_status(vdev); 863 if (status & VIRTIO_CONFIG_S_FEATURES_OK) { 864 features_driver = vdev->config->get_driver_features(vdev); 865 if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, 866 VDPA_ATTR_PAD)) 867 return -EMSGSIZE; 868 } If this solution looks good, I will add this patch in my V2 series. Thanks Zhu Lingshan > > >> 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 <lingshan.zhu@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) { >> >> >> >> >> >> >> >>
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) {
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@intel.com> --- drivers/vdpa/vdpa.c | 8 -------- 1 file changed, 8 deletions(-)