[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220816042157-mutt-send-email-mst@kernel.org>
Date: Tue, 16 Aug 2022 04:23:21 -0400
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Si-Wei Liu <si-wei.liu@...cle.com>
Cc: Zhu Lingshan <lingshan.zhu@...el.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 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
Powered by blists - more mailing lists