Re: [PATCH] virtio: acknowledge all features before access

[Michael S. Tsirkin]

On Tue, Jan 18, 2022 at 03:43:50PM +0100, Halil Pasic wrote:
> On Fri, 14 Jan 2022 15:09:14 -0500
> "Michael S. Tsirkin" <mst@xxxxxxxxxx> wrote:
> 
> > The feature negotiation was designed in a way that
> > makes it possible for devices to know which config
> > fields will be accessed by drivers.
> > 
> > This is broken since commit 404123c2db79 ("virtio: allow drivers to
> > validate features") with fallout in at least block and net.
> > We have a partial work-around in commit 2f9a174f918e ("virtio: write
> > back F_VERSION_1 before validate") which at least lets devices
> > find out which format should config space have, but this
> > is a partial fix: guests should not access config space
> > without acknowledging features since otherwise we'll never
> > be able to change the config space format.
> 
> I agree with that. The crux is what does "acknowledge features" exactly
> mean. Is it "write features" or "complete the feature negotiation,
> including setting FEATURES_OK".

Right. I think originally it was "write features". We then added
the FEATURES_OK in order to give devices a chance to reject
a set of features, and while doing this we failed to
notice that at this point "acknowledge" became confusing.

There's a spec proposal to make things more explicit, and
I think I will tweak it to actually use the term
"acknowledge".

On top of that, it makes sense to go back, scan all of the spec
and see whether any places that we changed from set not negotiated
for clarify actually should read "set in the written".

> My understanding is, that we should not rely on that the device is
> going to act according to the negotiated feature set unless FEATURES_OK
> was set successfully.


not for writes, but for reads there's little choice.

> That would mean, that this change ain't guaranteed to help with the
> stated problem. We simply don't know if the fact that features
> were written is going to have a side-effect or not. Also see below.

right. at the moment it's just the MTU field. In the future it
can be more, but by that future time we can fix the spec ;)

> > 
> > As a side effect, this also reduces the amount of hypervisor accesses -
> > we now only acknowledge features once unless we are clearing any
> > features when validating.
> 
> My understanding is that this patch basically does for all the features,
> what commit 2f9a174f918e ("virtio: write back F_VERSION_1 before
> validate") did only for F_VERSION_1 and under certain conditions to
> be minimally invasive.
> 
> I don't like when s390 is the oddball, so I'm very happy to see us
> moving away from that.
> 
> > 
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: 404123c2db79 ("virtio: allow drivers to validate features")
> > Fixes: 2f9a174f918e ("virtio: write back F_VERSION_1 before validate")
> > Cc: "Halil Pasic" <pasic@xxxxxxxxxxxxx>
> > Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > ---
> > 
> > Halil, I thought hard about our situation with transitional and
> > today I finally thought of something I am happy with.
> > Pls let me know what you think. Testing on big endian would
> > also be much appreciated!
> 
> Thanks! I will first provide some comments, and I intend to come back
> with the test results later.
> 
> > 
> >  drivers/virtio/virtio.c | 31 +++++++++++++++++--------------
> >  1 file changed, 17 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index d891b0a354b0..2ed6e2451fd8 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -168,12 +168,10 @@ EXPORT_SYMBOL_GPL(virtio_add_status);
> >  
> >  static int virtio_finalize_features(struct virtio_device *dev)
> >  {
> > -	int ret = dev->config->finalize_features(dev);
> >  	unsigned status;
> > +	int ret;
> >  
> >  	might_sleep();
> > -	if (ret)
> > -		return ret;
> >  
> >  	ret = arch_has_restricted_virtio_memory_access();
> >  	if (ret) {
> > @@ -244,17 +242,6 @@ static int virtio_dev_probe(struct device *_d)
> >  		driver_features_legacy = driver_features;
> >  	}
> >  
> > -	/*
> > -	 * Some devices detect legacy solely via F_VERSION_1. Write
> > -	 * F_VERSION_1 to force LE config space accesses before FEATURES_OK for
> > -	 * these when needed.
> > -	 */
> > -	if (drv->validate && !virtio_legacy_is_little_endian()
> > -			  && device_features & BIT_ULL(VIRTIO_F_VERSION_1)) {
> > -		dev->features = BIT_ULL(VIRTIO_F_VERSION_1);
> > -		dev->config->finalize_features(dev);
> > -	}
> > -
> >  	if (device_features & (1ULL << VIRTIO_F_VERSION_1))
> >  		dev->features = driver_features & device_features;
> >  	else
> > @@ -265,10 +252,22 @@ static int virtio_dev_probe(struct device *_d)
> >  		if (device_features & (1ULL << i))
> >  			__virtio_set_bit(dev, i);
> >  
> > +	err = dev->config->finalize_features(dev);
> 
> A side note: config->finalize_features() ain't the best name for what the
> thing does. After config->finalize_features() the features are not final.
> Unlike after virtio_finalize_features(). IMHO filter_and_write_features()
> would be a more accurate, although longer name.

I agree.
But I think I will start with figuring out the best name in the spec.
Maybe write back or acknowledge. Let's see what gets accepted by
the tc. Then I'll change code to match.

> After this point, the features aren't final yet, and one can not say
> that a some feature X has been negotiated. But with regards to features,
> the spec does not really consider this limbo state.
> 
> Should this change? Do we want to say: the device SHOULD pick up, and
> act upon the new features *before* FEATURES_OK is set?

Yes but only for handling config reads. See my spec proposal.

> ...
> 
> > +	if (err)
> > +		goto err;
> > +
> >  	if (drv->validate) {
> > +		u64 features = dev->features;
> > +
> >  		err = drv->validate(dev);
> 
> ... Consider the "we would like to introduce a new config space format"
> example. Here, I guess we would like to use the new format. But let's say
> _F_CFG_FMT_V2 aint negotiated yet. So to be sure about the format, we
> would need to specify, that the behavior of the device needs to change
> after the feature has been written, but before FEATURES_OK is set, at
> least for _F_CFG_FMT_V2.

for config space reads. yes.

> Please also consider the QEMU implementation of the vhost-user stuff. We
> push the features to the back-end only when FEATURES_OK status is
> written.

that has to change, anyway - it's needed to fix endian-ness.

> 
> >  		if (err)
> >  			goto err;
> > +
> > +		if (features != dev->features) {
> > +			err = dev->config->finalize_features(dev);
> 
> It is fine to call it again, because the features aren't finalized yet.
> And re-doing any transport-level filtering and validation is fine as
> well.

Will add a comment.

> > +			if (err)
> > +				goto err;
> > +		}
> >  	}
> >  
> >  	err = virtio_finalize_features(dev);
> 
> Here the features are finally negotiated and final.
> 
> > @@ -495,6 +494,10 @@ int virtio_device_restore(struct virtio_device *dev)
> >  	/* We have a driver! */
> >  	virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
> >  
> > +	ret = dev->config->finalize_features(dev);
> > +	if (ret)
> > +		goto err;
> > +
> >  	ret = virtio_finalize_features(dev);
> 
> Looks a little weird, because virtio_finalize_features() used to include
> filter + write + set FEATURES_OK. But it ain't too bad.
> 
> Better names would benefit readability though, if we can come up with
> some.
> 
> Regards,
> Halil

right. I think that can be a patch on top though.

> >  	if (ret)
> >  		goto err;