Message-ID: <20220817021324-mutt-send-email-mst@kernel.org>
Date:   Wed, 17 Aug 2022 02:14:05 -0400
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     "Zhu, Lingshan" <lingshan.zhu@...el.com>
Cc:     Si-Wei Liu <si-wei.liu@...cle.com>,
        virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
        kvm@...r.kernel.org, parav@...dia.com, xieyongji@...edance.com,
        gautam.dawar@....com, jasowang@...hat.com
Subject: Re: [PATCH V5 4/6] vDPA: !FEATURES_OK should not block querying
 device config space

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@...el.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) {
> 
> 
> 
> 
> 
> 
> 
> 

Powered by blists - more mailing lists