[PATCH] vhost_net: initialize acked_features to a safe value during ack

[Jason Wang]

commit 2e6d46d77ed328d34a94688da8371bcbe243479b (vhost: add
vhost_get_features and vhost_ack_features) removes the step that
initializes the acked_features to backend_features. This will result an
unexpected value of acked_features which may fail the features setting of
vhost. This patch fixes it by recover this step.

Cc: Nikolay Nikolaev <***@virtualopensystems.com>
Cc: Andrey Korolyov <***@xdel.ru>
Cc: Michael S. Tsirkin <***@redhat.com>
Cc: Michael Roth <***@linux.vnet.ibm.com>
Cc: qemu-***@nongnu.org
Signed-off-by: Jason Wang <***@redhat.com>
---
hw/net/vhost_net.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index f87c798..b1d4b1f 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -115,6 +115,7 @@ unsigned vhost_net_get_features(struct vhost_net *net, unsigned features)

void vhost_net_ack_features(struct vhost_net *net, unsigned features)
{
+ net->dev.acked_features = net->dev.backend_features;
vhost_ack_features(&net->dev, vhost_net_get_feature_bits(net), features);
}

[Michael S. Tsirkin]

Post by Jason Wang
commit 2e6d46d77ed328d34a94688da8371bcbe243479b (vhost: add
vhost_get_features and vhost_ack_features) removes the step that
initializes the acked_features to backend_features.

But acked features are set in vhost_ack_features.
why would we need to initialize to backend_features?
0 is a better default.

Post by Jason Wang
This will result an
unexpected value of acked_features which may fail the features setting of
vhost. This patch fixes it by recover this step.
---
hw/net/vhost_net.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index f87c798..b1d4b1f 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -115,6 +115,7 @@ unsigned vhost_net_get_features(struct vhost_net *net, unsigned features)
void vhost_net_ack_features(struct vhost_net *net, unsigned features)
{
+ net->dev.acked_features = net->dev.backend_features;

I think it's wrong: you don't want to set all features.

Post by Jason Wang
vhost_ack_features(&net->dev, vhost_net_get_feature_bits(net), features);
}
--
1.8.3.1

[Michael S. Tsirkin]

commit 2e6d46d77ed328d34a94688da8371bcbe243479b (vhost: add
vhost_get_features and vhost_ack_features) removes the step that
initializes the acked_features to backend_features.

As this field is now uninitialized, vhost initialization will sometimes
fail.

To fix, initialize field in core vhost code.

As the next step, cleanup vhost scsi code as well.

Reported-by: Jason Wang <***@redhat.com>
Reported-by: Andrey Korolyov <***@xdel.ru>
Cc: Nikolay Nikolaev <***@virtualopensystems.com>
Cc: qemu-***@nongnu.org
Signed-off-by: Jason Wang <***@redhat.com>
Reviewed-by: Michael S. Tsirkin <***@redhat.com>
Signed-off-by: Michael S. Tsirkin <***@redhat.com>
---
hw/net/vhost_net.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 1fe18c7..e258fda 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -115,6 +115,7 @@ unsigned vhost_net_get_features(struct vhost_net *net, unsigned features)

void vhost_net_ack_features(struct vhost_net *net, unsigned features)
{
+ net->dev.acked_features = net->dev.backend_features;
vhost_ack_features(&net->dev, vhost_net_get_feature_bits(net), features);
}

[Michael S. Tsirkin]

Post by Jason Wang
commit 2e6d46d77ed328d34a94688da8371bcbe243479b (vhost: add
vhost_get_features and vhost_ack_features) removes the step that
initializes the acked_features to backend_features.
As this field is now uninitialized, vhost initialization will sometimes
fail.
To fix, initialize field in core vhost code.
As the next step, cleanup vhost scsi code as well.

Oops, it's the wrong patch - original one from jason :(
Sorry, resending.

Post by Jason Wang
---
hw/net/vhost_net.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 1fe18c7..e258fda 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -115,6 +115,7 @@ unsigned vhost_net_get_features(struct vhost_net *net, unsigned features)
void vhost_net_ack_features(struct vhost_net *net, unsigned features)
{
+ net->dev.acked_features = net->dev.backend_features;
vhost_ack_features(&net->dev, vhost_net_get_feature_bits(net), features);
}
--
MST

[Michael S. Tsirkin]

Post by Jason Wang
commit 2e6d46d77ed328d34a94688da8371bcbe243479b (vhost: add
vhost_get_features and vhost_ack_features) removes the step that
initializes the acked_features to backend_features. This will result an
unexpected value of acked_features which may fail the features setting of
vhost. This patch fixes it by recover this step.

OK I get it and it's correct, but I think it's better to
put the initialization in core vhost code.
Patch sent, could you confirm that it works for you please?

Post by Jason Wang
---
hw/net/vhost_net.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index f87c798..b1d4b1f 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -115,6 +115,7 @@ unsigned vhost_net_get_features(struct vhost_net *net, unsigned features)
void vhost_net_ack_features(struct vhost_net *net, unsigned features)
{
+ net->dev.acked_features = net->dev.backend_features;
vhost_ack_features(&net->dev, vhost_net_get_feature_bits(net), features);
}
--
1.8.3.1

[Andrey Korolyov]

Post by Michael S. Tsirkin

Post by Jason Wang
commit 2e6d46d77ed328d34a94688da8371bcbe243479b (vhost: add
vhost_get_features and vhost_ack_features) removes the step that
initializes the acked_features to backend_features. This will result an
unexpected value of acked_features which may fail the features setting of
vhost. This patch fixes it by recover this step.

OK I get it and it's correct, but I think it's better to
put the initialization in core vhost code.
Patch sent, could you confirm that it works for you please?

Post by Jason Wang
---
hw/net/vhost_net.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index f87c798..b1d4b1f 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -115,6 +115,7 @@ unsigned vhost_net_get_features(struct vhost_net *net, unsigned features)
void vhost_net_ack_features(struct vhost_net *net, unsigned features)
{
+ net->dev.acked_features = net->dev.backend_features;
vhost_ack_features(&net->dev, vhost_net_get_feature_bits(net), features);
}
--
1.8.3.1

Yes, this patch fixes both issues with vhost subsystem for me.

[Michael S. Tsirkin]

Post by Andrey Korolyov

Post by Michael S. Tsirkin

Post by Jason Wang
commit 2e6d46d77ed328d34a94688da8371bcbe243479b (vhost: add
vhost_get_features and vhost_ack_features) removes the step that
initializes the acked_features to backend_features. This will result an
unexpected value of acked_features which may fail the features setting of
vhost. This patch fixes it by recover this step.

OK I get it and it's correct, but I think it's better to
put the initialization in core vhost code.
Patch sent, could you confirm that it works for you please?

Post by Jason Wang
---
hw/net/vhost_net.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index f87c798..b1d4b1f 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -115,6 +115,7 @@ unsigned vhost_net_get_features(struct vhost_net *net, unsigned features)
void vhost_net_ack_features(struct vhost_net *net, unsigned features)
{
+ net->dev.acked_features = net->dev.backend_features;
vhost_ack_features(&net->dev, vhost_net_get_feature_bits(net), features);
}
--
1.8.3.1

Yes, this patch fixes both issues with vhost subsystem for me.

Sorry posted a different one - can you pls try it out?
We still have a bug somewhere in error handling I suspect, so
let's keep debugging.

[Michael S. Tsirkin]

This post might be inappropriate. Click to display it.