[1/1] virtio: write back F_VERSION_1 before validate

Commit Message

Halil Pasic Oct. 6, 2021, 2:25 p.m. UTC

The virtio specification virtio-v1.1-cs01 states: Transitional devices
MUST detect Legacy drivers by detecting that VIRTIO_F_VERSION_1 has not
been acknowledged by the driver.  This is exactly what QEMU as of 6.1
has done relying solely on VIRTIO_F_VERSION_1 for detecting that.

However, the specification also says: driver MAY read (but MUST NOT
write) the device-specific configuration fields to check that it can
support the device before setting FEATURES_OK.

In that case, any transitional device relying solely on
VIRTIO_F_VERSION_1 for detecting legacy drivers will return data in
legacy format.  In particular, this implies that it is in big endian
format for big endian guests. This naturally confuses the driver which
expects little endian in the modern mode.

It is probably a good idea to amend the spec to clarify that
VIRTIO_F_VERSION_1 can only be relied on after the feature negotiation
is complete. However, we already have regression so let's try to address
it.

Signed-off-by: Halil Pasic <[e-mail]>
Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config space")
Fixes: fe36cbe0671e ("virtio_net: clear MTU when out of range")
Reported-by: [e-mail]
---
 drivers/virtio/virtio.c | 10 ++++++++++
 1 file changed, 10 insertions(+)


base-commit: 60a9483534ed0d99090a2ee1d4bb0b8179195f51

Comments

Cornelia Huck Oct. 7, 2021, 11:52 a.m. UTC | #1

On Wed, Oct 06 2021, Halil Pasic <[e-mail]> wrote:

> The virtio specification virtio-v1.1-cs01 states: Transitional devices
> MUST detect Legacy drivers by detecting that VIRTIO_F_VERSION_1 has not
> been acknowledged by the driver.  This is exactly what QEMU as of 6.1
> has done relying solely on VIRTIO_F_VERSION_1 for detecting that.
>
> However, the specification also says: driver MAY read (but MUST NOT
> write) the device-specific configuration fields to check that it can
> support the device before setting FEATURES_OK.

Suggest to put the citations from the spec into quotes, so that they are
distinguishable from the rest of the text.

>
> In that case, any transitional device relying solely on
> VIRTIO_F_VERSION_1 for detecting legacy drivers will return data in
> legacy format.  In particular, this implies that it is in big endian
> format for big endian guests. This naturally confuses the driver which
> expects little endian in the modern mode.
>
> It is probably a good idea to amend the spec to clarify that
> VIRTIO_F_VERSION_1 can only be relied on after the feature negotiation
> is complete. However, we already have regression so let's try to address

s/regression/a regression/

> it.

Maybe mention what the regression is?

Also mention that we use this workaround for modern on BE only?

>
> Signed-off-by: Halil Pasic <[e-mail]>
> Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config space")
> Fixes: fe36cbe0671e ("virtio_net: clear MTU when out of range")
> Reported-by: [e-mail]
> ---
>  drivers/virtio/virtio.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 0a5b54034d4b..494cfecd3376 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -239,6 +239,16 @@ 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 for these when needed.

"...to force LE config space accesses before FEATURES_OK for these when
needed (BE)."

?

> +	 */
> +	if (drv->validate && !virtio_legacy_is_little_endian()
> +			  && BIT_ULL(VIRTIO_F_VERSION_1) & device_features) {

Nit: putting device_features first would read more naturally to me.

> +		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

Patch LGTM.

Halil Pasic Oct. 7, 2021, 2:32 p.m. UTC | #2

On Thu, 07 Oct 2021 13:52:24 +0200
Cornelia Huck <[e-mail]> wrote:

> On Wed, Oct 06 2021, Halil Pasic <[e-mail]> wrote:
> 
> > The virtio specification virtio-v1.1-cs01 states: Transitional devices
> > MUST detect Legacy drivers by detecting that VIRTIO_F_VERSION_1 has not
> > been acknowledged by the driver.  This is exactly what QEMU as of 6.1
> > has done relying solely on VIRTIO_F_VERSION_1 for detecting that.
> >
> > However, the specification also says: driver MAY read (but MUST NOT
> > write) the device-specific configuration fields to check that it can
> > support the device before setting FEATURES_OK.  
> 
> Suggest to put the citations from the spec into quotes, so that they are
> distinguishable from the rest of the text.

For the record: I basically took Michael's description, the one which you
said you prefer, with some minor changes.

This is one of the changes, which renders this a paraphrase and not a
quote. Michael didn't use quotation marks so I was not sure it is was
a word by word quote anyway. It was. But the spec depends on "During this
step" which does not make any sense without the context. That is why I made
the end of step explicit.

I think we are fine without quotation marks. Those who care can read the
spec.

> 
> >
> > In that case, any transitional device relying solely on
> > VIRTIO_F_VERSION_1 for detecting legacy drivers will return data in
> > legacy format.  In particular, this implies that it is in big endian
> > format for big endian guests. This naturally confuses the driver which
> > expects little endian in the modern mode.
> >
> > It is probably a good idea to amend the spec to clarify that
> > VIRTIO_F_VERSION_1 can only be relied on after the feature negotiation
> > is complete. However, we already have regression so let's try to address  
> 
> s/regression/a regression/
> 

Yes. Was like this in the original. Will change

> > it.  
> 
> Maybe mention what the regression is?

How about the following?

The regressions affect the VIRTIO_NET_F_MTU feature of virtio-net and the
VIRTIO_BLK_F_BLK_SIZE feature of virtio-blk for BE guests when virtio
1.0 is used on both sides. The latter renders virtio-blk unusable with
DASD backing, because things simply don't work with the default.

> 
> Also mention that we use this workaround for modern on BE only?

We have that already, don't we. The sentence that starts with "In
particular". The regression description should reinforce that
sufficiently IMHO.

> 
> >
> > Signed-off-by: Halil Pasic <[e-mail]>
> > Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config space")
> > Fixes: fe36cbe0671e ("virtio_net: clear MTU when out of range")
> > Reported-by: [e-mail]
> > ---
> >  drivers/virtio/virtio.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index 0a5b54034d4b..494cfecd3376 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -239,6 +239,16 @@ 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 for these when needed.  
> 
> "...to force LE config space accesses before FEATURES_OK for these when
> needed (BE)."
> 
> ?

Can do, but I would rather omit the (BE) at the end. All the conditions
are necessary:
* have validate callback
* device offered VERSION_1
* virtio legacy is be

> 
> > +	 */
> > +	if (drv->validate && !virtio_legacy_is_little_endian()
> > +			  && BIT_ULL(VIRTIO_F_VERSION_1) & device_features) {  
> 
> Nit: putting device_features first would read more naturally to me.
> 

Can do.

> > +		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  
> 
> Patch LGTM.
> 
> 

Thanks for having a look. If you are fine with the proposed solution
please tell me, so I can send out a v2.

If not let us work towards an acceptable solution.

Regards,
Halil

Cornelia Huck Oct. 7, 2021, 3:25 p.m. UTC | #3

On Thu, Oct 07 2021, Halil Pasic <[e-mail]> wrote:

> On Thu, 07 Oct 2021 13:52:24 +0200
> Cornelia Huck <[e-mail]> wrote:
>
>> On Wed, Oct 06 2021, Halil Pasic <[e-mail]> wrote:
>> 
>> > The virtio specification virtio-v1.1-cs01 states: Transitional devices
>> > MUST detect Legacy drivers by detecting that VIRTIO_F_VERSION_1 has not
>> > been acknowledged by the driver.  This is exactly what QEMU as of 6.1
>> > has done relying solely on VIRTIO_F_VERSION_1 for detecting that.
>> >
>> > However, the specification also says: driver MAY read (but MUST NOT
>> > write) the device-specific configuration fields to check that it can
>> > support the device before setting FEATURES_OK.  
>> 
>> Suggest to put the citations from the spec into quotes, so that they are
>> distinguishable from the rest of the text.
>
> For the record: I basically took Michael's description, the one which you
> said you prefer, with some minor changes.

Well I did look at what the text said, not the details in the formatting...

>
> This is one of the changes, which renders this a paraphrase and not a
> quote. Michael didn't use quotation marks so I was not sure it is was
> a word by word quote anyway. It was. But the spec depends on "During this
> step" which does not make any sense without the context. That is why I made
> the end of step explicit.

I still think that would be nicer while using some quotation marks, even
if you are just doing a partial quote.

In the first paragraph, however, we really should mark the quote
properly. It gave me a stop when I first read it.

>
> I think we are fine without quotation marks. Those who care can read the
> spec.
>
>> 
>> >
>> > In that case, any transitional device relying solely on
>> > VIRTIO_F_VERSION_1 for detecting legacy drivers will return data in
>> > legacy format.  In particular, this implies that it is in big endian
>> > format for big endian guests. This naturally confuses the driver which
>> > expects little endian in the modern mode.
>> >
>> > It is probably a good idea to amend the spec to clarify that
>> > VIRTIO_F_VERSION_1 can only be relied on after the feature negotiation
>> > is complete. However, we already have regression so let's try to address  
>> 
>> s/regression/a regression/
>> 
>
> Yes. Was like this in the original. Will change
>
>> > it.  
>> 
>> Maybe mention what the regression is?
>
> How about the following?
>
> The regressions affect the VIRTIO_NET_F_MTU feature of virtio-net and the
> VIRTIO_BLK_F_BLK_SIZE feature of virtio-blk for BE guests when virtio
> 1.0 is used on both sides. The latter renders virtio-blk unusable with
> DASD backing, because things simply don't work with the default.

Sounds good to me.

>
>> 
>> Also mention that we use this workaround for modern on BE only?
>
> We have that already, don't we. The sentence that starts with "In
> particular". The regression description should reinforce that
> sufficiently IMHO.

No strong opinion here. Anyone else?

>
>> 
>> >
>> > Signed-off-by: Halil Pasic <[e-mail]>
>> > Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config space")
>> > Fixes: fe36cbe0671e ("virtio_net: clear MTU when out of range")
>> > Reported-by: [e-mail]
>> > ---
>> >  drivers/virtio/virtio.c | 10 ++++++++++
>> >  1 file changed, 10 insertions(+)
>> >
>> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> > index 0a5b54034d4b..494cfecd3376 100644
>> > --- a/drivers/virtio/virtio.c
>> > +++ b/drivers/virtio/virtio.c
>> > @@ -239,6 +239,16 @@ 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 for these when needed.  
>> 
>> "...to force LE config space accesses before FEATURES_OK for these when
>> needed (BE)."
>> 
>> ?
>
> Can do, but I would rather omit the (BE) at the end. All the conditions
> are necessary:
> * have validate callback
> * device offered VERSION_1
> * virtio legacy is be
>

Ok, let's use that without the trailing BE.

>> 
>> > +	 */
>> > +	if (drv->validate && !virtio_legacy_is_little_endian()
>> > +			  && BIT_ULL(VIRTIO_F_VERSION_1) & device_features) {  
>> 
>> Nit: putting device_features first would read more naturally to me.
>> 
>
> Can do.
>
>> > +		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  
>> 
>> Patch LGTM.
>> 
>> 
>
> Thanks for having a look. If you are fine with the proposed solution
> please tell me, so I can send out a v2.

No further comments other than what I wrote above, but maybe others have
comments as well?

Halil Pasic Oct. 7, 2021, 3:52 p.m. UTC | #4

On Thu, 07 Oct 2021 17:25:52 +0200
Cornelia Huck <[e-mail]> wrote:

> On Thu, Oct 07 2021, Halil Pasic <[e-mail]> wrote:
> 
> > On Thu, 07 Oct 2021 13:52:24 +0200
> > Cornelia Huck <[e-mail]> wrote:
> >  
> >> On Wed, Oct 06 2021, Halil Pasic <[e-mail]> wrote:
> >>   
> >> > The virtio specification virtio-v1.1-cs01 states: "Transitional devices
> >> > MUST detect Legacy drivers by detecting that VIRTIO_F_VERSION_1 has not
> >> > been acknowledged by the driver."  This is exactly what QEMU as of 6.1
> >> > has done relying solely on VIRTIO_F_VERSION_1 for detecting that.
> >> >
> >> > However, the specification also says: "... driver MAY read (but MUST NOT
> >> > write) the device-specific configuration fields to check that it can
> >> > support the device ..." before setting FEATURES_OK.    
> >> 
> >> Suggest to put the citations from the spec into quotes, so that they are
> >> distinguishable from the rest of the text.  
> >
> > For the record: I basically took Michael's description, the one which you
> > said you prefer, with some minor changes.  
> 
> Well I did look at what the text said, not the details in the formatting...
> 
> >
> > This is one of the changes, which renders this a paraphrase and not a
> > quote. Michael didn't use quotation marks so I was not sure it is was
> > a word by word quote anyway. It was. But the spec depends on "During this
> > step" which does not make any sense without the context. That is why I made
> > the end of step explicit.  
> 
> I still think that would be nicer while using some quotation marks, even
> if you are just doing a partial quote.
> 
> In the first paragraph, however, we really should mark the quote
> properly. It gave me a stop when I first read it.

I've added in some quotation marks and ellipsis marks. Does that look
good for you?

> 
> >
> > I think we are fine without quotation marks. Those who care can read the
> > spec.
> >  
> >>   
> >> >
> >> > In that case, any transitional device relying solely on
> >> > VIRTIO_F_VERSION_1 for detecting legacy drivers will return data in
> >> > legacy format.  In particular, this implies that it is in big endian
> >> > format for big endian guests. This naturally confuses the driver which
> >> > expects little endian in the modern mode.
> >> >
> >> > It is probably a good idea to amend the spec to clarify that
> >> > VIRTIO_F_VERSION_1 can only be relied on after the feature negotiation
> >> > is complete. However, we already have regression so let's try to address    
> >> 
> >> s/regression/a regression/
> >>   
> >
> > Yes. Was like this in the original. Will change
> >  
> >> > it.    
> >> 
> >> Maybe mention what the regression is?  
> >
> > How about the following?
> >
> > The regressions affect the VIRTIO_NET_F_MTU feature of virtio-net and the
> > VIRTIO_BLK_F_BLK_SIZE feature of virtio-blk for BE guests when virtio
> > 1.0 is used on both sides. The latter renders virtio-blk unusable with
> > DASD backing, because things simply don't work with the default.  
> 
> Sounds good to me.

Will add it to the end.

> 
> >  
> >> 
> >> Also mention that we use this workaround for modern on BE only?  
> >
> > We have that already, don't we. The sentence that starts with "In
> > particular". The regression description should reinforce that
> > sufficiently IMHO.  
> 
> No strong opinion here. Anyone else?
> 
> >  
> >>   
> >> >
> >> > Signed-off-by: Halil Pasic <[e-mail]>
> >> > Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config space")
> >> > Fixes: fe36cbe0671e ("virtio_net: clear MTU when out of range")
> >> > Reported-by: [e-mail]
> >> > ---
> >> >  drivers/virtio/virtio.c | 10 ++++++++++
> >> >  1 file changed, 10 insertions(+)
> >> >
> >> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> >> > index 0a5b54034d4b..494cfecd3376 100644
> >> > --- a/drivers/virtio/virtio.c
> >> > +++ b/drivers/virtio/virtio.c
> >> > @@ -239,6 +239,16 @@ 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 for these when needed.    
> >> 
> >> "...to force LE config space accesses before FEATURES_OK for these when
> >> needed (BE)."
> >> 
> >> ?  
> >
> > Can do, but I would rather omit the (BE) at the end. All the conditions
> > are necessary:
> > * have validate callback
> > * device offered VERSION_1
> > * virtio legacy is be
> >  
> 
> Ok, let's use that without the trailing BE.
>

Nod.
 
> >>   
> >> > +	 */
> >> > +	if (drv->validate && !virtio_legacy_is_little_endian()
> >> > +			  && BIT_ULL(VIRTIO_F_VERSION_1) & device_features) {    
> >> 
> >> Nit: putting device_features first would read more naturally to me.
> >>   
> >
> > Can do.
> >  
> >> > +		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    
> >> 
> >> Patch LGTM.
> >> 
> >>   
> >
> > Thanks for having a look. If you are fine with the proposed solution
> > please tell me, so I can send out a v2.  
> 
> No further comments other than what I wrote above, but maybe others have
> comments as well?
> 
> 

I will wait then till end of day before sending out a v2.

Thank you very much!

Regards,
Halil

Cornelia Huck Oct. 7, 2021, 4:16 p.m. UTC | #5

On Thu, Oct 07 2021, Halil Pasic <[e-mail]> wrote:

> On Thu, 07 Oct 2021 17:25:52 +0200
> Cornelia Huck <[e-mail]> wrote:
>
>> On Thu, Oct 07 2021, Halil Pasic <[e-mail]> wrote:
>> 
>> > On Thu, 07 Oct 2021 13:52:24 +0200
>> > Cornelia Huck <[e-mail]> wrote:
>> >  
>> >> On Wed, Oct 06 2021, Halil Pasic <[e-mail]> wrote:
>> >>   
>> >> > The virtio specification virtio-v1.1-cs01 states: "Transitional devices
>> >> > MUST detect Legacy drivers by detecting that VIRTIO_F_VERSION_1 has not
>> >> > been acknowledged by the driver."  This is exactly what QEMU as of 6.1
>> >> > has done relying solely on VIRTIO_F_VERSION_1 for detecting that.
>> >> >
>> >> > However, the specification also says: "... driver MAY read (but MUST NOT
>> >> > write) the device-specific configuration fields to check that it can
>> >> > support the device ..." before setting FEATURES_OK.    
>> >> 
>> >> Suggest to put the citations from the spec into quotes, so that they are
>> >> distinguishable from the rest of the text.  
>> >
>> > For the record: I basically took Michael's description, the one which you
>> > said you prefer, with some minor changes.  
>> 
>> Well I did look at what the text said, not the details in the formatting...
>> 
>> >
>> > This is one of the changes, which renders this a paraphrase and not a
>> > quote. Michael didn't use quotation marks so I was not sure it is was
>> > a word by word quote anyway. It was. But the spec depends on "During this
>> > step" which does not make any sense without the context. That is why I made
>> > the end of step explicit.  
>> 
>> I still think that would be nicer while using some quotation marks, even
>> if you are just doing a partial quote.
>> 
>> In the first paragraph, however, we really should mark the quote
>> properly. It gave me a stop when I first read it.
>
> I've added in some quotation marks and ellipsis marks. Does that look
> good for you?

Yep, works for me.

Patch

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 0a5b54034d4b..494cfecd3376 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -239,6 +239,16 @@  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 for these when needed.
+	 */
+	if (drv->validate && !virtio_legacy_is_little_endian()
+			  && BIT_ULL(VIRTIO_F_VERSION_1) & device_features) {
+		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