Linux Virtualization - Dec 2019 - [PATCH 2/2] virtio-pci: check name when counting MSI-X vectors

Ensure that elements of the array that correspond to unavailable
features are set to NULL; previously, they would be left uninitialized.

Since the corresponding names array elements were explicitly set to
NULL, the uninitialized callback pointers would not actually be
dereferenced; however, the uninitialized callbacks elements would still
be read in vp_find_vqs_msix() and used to calculate the number of MSI-X
vectors required.

Fixes: 86a559787e6f ("virtio-balloon:
VIRTIO_BALLOON_F_FREE_PAGE_HINT")
Signed-off-by: Daniel Verkamp <dverkamp at chromium.org>
---
 drivers/virtio/virtio_balloon.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 93f995f6cf36..8e400ece9273 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -475,7 +475,9 @@ static int init_vqs(struct virtio_balloon *vb)
 	names[VIRTIO_BALLOON_VQ_INFLATE] = "inflate";
 	callbacks[VIRTIO_BALLOON_VQ_DEFLATE] = balloon_ack;
 	names[VIRTIO_BALLOON_VQ_DEFLATE] = "deflate";
+	callbacks[VIRTIO_BALLOON_VQ_STATS] = NULL;
 	names[VIRTIO_BALLOON_VQ_STATS] = NULL;
+	callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
 	names[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
 
 	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
-- 
2.24.1.735.g03f4e72817-goog

VQs without a name specified are not valid; they are skipped in the
later loop that assigns MSI-X vectors to queues, but the per_vq_vectors
loop above that counts the required number of vectors previously still
counted any queue with a non-NULL callback as needing a vector.

Add a check to the per_vq_vectors loop so that vectors with no name are
not counted to make the two loops consistent.  This prevents
over-counting unnecessary vectors (e.g. for features which were not
negotiated with the device).

Signed-off-by: Daniel Verkamp <dverkamp at chromium.org>
---
 drivers/virtio/virtio_pci_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
index f2862f66c2ac..222d630c41fc 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -294,7 +294,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev,
unsigned nvqs,
 		/* Best option: one for change interrupt, one per vq. */
 		nvectors = 1;
 		for (i = 0; i < nvqs; ++i)
-			if (callbacks[i])
+			if (names[i] && callbacks[i])
 				++nvectors;
 	} else {
 		/* Second best: one for change, shared for all vqs. */
-- 
2.24.1.735.g03f4e72817-goog

On Tue, Dec 17, 2019 at 11:06:10AM -0800, Daniel Verkamp
wrote:
> VQs without a name specified are not valid; they are skipped in the
> later loop that assigns MSI-X vectors to queues, but the per_vq_vectors
> loop above that counts the required number of vectors previously still
> counted any queue with a non-NULL callback as needing a vector.
> 
> Add a check to the per_vq_vectors loop so that vectors with no name are
> not counted to make the two loops consistent.  This prevents
> over-counting unnecessary vectors (e.g. for features which were not
> negotiated with the device).
> 
> Signed-off-by: Daniel Verkamp <dverkamp at chromium.org>
And I'll add:
Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the
balloon driver (V4)")

here too.
> ---
>  drivers/virtio/virtio_pci_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
> index f2862f66c2ac..222d630c41fc 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -294,7 +294,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev,
unsigned nvqs,
>  		/* Best option: one for change interrupt, one per vq. */
>  		nvectors = 1;
>  		for (i = 0; i < nvqs; ++i)
> -			if (callbacks[i])
> +			if (names[i] && callbacks[i])
>  				++nvectors;
>  	} else {
>  		/* Second best: one for change, shared for all vqs. */
> -- 
> 2.24.1.735.g03f4e72817-goog

On Tue, Dec 17, 2019 at 11:06:09AM -0800, Daniel Verkamp
wrote:
> Ensure that elements of the array that correspond to unavailable
> features are set to NULL; previously, they would be left uninitialized.
> 
> Since the corresponding names array elements were explicitly set to
> NULL, the uninitialized callback pointers would not actually be
> dereferenced; however, the uninitialized callbacks elements would still
> be read in vp_find_vqs_msix() and used to calculate the number of MSI-X
> vectors required.
> 
> Fixes: 86a559787e6f ("virtio-balloon:
VIRTIO_BALLOON_F_FREE_PAGE_HINT")
> Signed-off-by: Daniel Verkamp <dverkamp at chromium.org>
Actually, we already have the issue with the stats VQ, no?

So I think this one is more appropriate:
Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the
balloon driver (V4)")

> ---
>  drivers/virtio/virtio_balloon.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_balloon.c
b/drivers/virtio/virtio_balloon.c
> index 93f995f6cf36..8e400ece9273 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -475,7 +475,9 @@ static int init_vqs(struct virtio_balloon *vb)
>  	names[VIRTIO_BALLOON_VQ_INFLATE] = "inflate";
>  	callbacks[VIRTIO_BALLOON_VQ_DEFLATE] = balloon_ack;
>  	names[VIRTIO_BALLOON_VQ_DEFLATE] = "deflate";
> +	callbacks[VIRTIO_BALLOON_VQ_STATS] = NULL;
>  	names[VIRTIO_BALLOON_VQ_STATS] = NULL;
> +	callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
>  	names[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
>  
>  	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> -- 
> 2.24.1.735.g03f4e72817-goog

On Tue, Dec 17, 2019 at 12:05 PM Michael S. Tsirkin <mst at redhat.com>
wrote:
>
> On Tue, Dec 17, 2019 at 11:06:09AM -0800, Daniel Verkamp wrote:
> > Ensure that elements of the array that correspond to unavailable
> > features are set to NULL; previously, they would be left
uninitialized.
> >
> > Since the corresponding names array elements were explicitly set to
> > NULL, the uninitialized callback pointers would not actually be
> > dereferenced; however, the uninitialized callbacks elements would
still
> > be read in vp_find_vqs_msix() and used to calculate the number of
MSI-X
> > vectors required.
> >
> > Fixes: 86a559787e6f ("virtio-balloon:
VIRTIO_BALLOON_F_FREE_PAGE_HINT")
> > Signed-off-by: Daniel Verkamp <dverkamp at chromium.org>
>
> Actually, we already have the issue with the stats VQ, no?
>
> So I think this one is more appropriate:
> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the
balloon driver (V4)")
I think things were OK in 9564e138b1f6 because nvqs was calculated
based on the available features, so the later elements of the array
would not have been inspected by find_vqs.  86a559787e6f introduced
the uninitialized array elements as well as the removal of dynamic
nvqs based on features.

On 12/18/2019 03:06 AM, Daniel Verkamp wrote:
> Ensure that elements of the array that correspond to unavailable
> features are set to NULL; previously, they would be left uninitialized.
>
> Since the corresponding names array elements were explicitly set to
> NULL, the uninitialized callback pointers would not actually be
> dereferenced; however, the uninitialized callbacks elements would still
> be read in vp_find_vqs_msix() and used to calculate the number of MSI-X
> vectors required.
With your 2nd patch:
if (names[i] && callbacks[i])
     ++nvectors;

It seems this patch isn't necessary as names[i] is already NULL, isn't
it?

Best,
Wei

On Tue, Dec 17, 2019 at 11:06:10AM -0800, Daniel Verkamp
wrote:
> VQs without a name specified are not valid; they are skipped in the
> later loop that assigns MSI-X vectors to queues, but the per_vq_vectors
> loop above that counts the required number of vectors previously still
> counted any queue with a non-NULL callback as needing a vector.
> 
> Add a check to the per_vq_vectors loop so that vectors with no name are
> not counted to make the two loops consistent.  This prevents
> over-counting unnecessary vectors (e.g. for features which were not
> negotiated with the device).
> 
> Signed-off-by: Daniel Verkamp <dverkamp at chromium.org>
OK so I will add
Fixes: 86a559787e6f ("virtio-balloon:
VIRTIO_BALLOON_F_FREE_PAGE_HINT")

here too?
> ---
>  drivers/virtio/virtio_pci_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
> index f2862f66c2ac..222d630c41fc 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -294,7 +294,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev,
unsigned nvqs,
>  		/* Best option: one for change interrupt, one per vq. */
>  		nvectors = 1;
>  		for (i = 0; i < nvqs; ++i)
> -			if (callbacks[i])
> +			if (names[i] && callbacks[i])
>  				++nvectors;
>  	} else {
>  		/* Second best: one for change, shared for all vqs. */
> -- 
> 2.24.1.735.g03f4e72817-goog

On Wed, Dec 18, 2019 at 11:18:45AM +0800, Wei Wang
wrote:
> On 12/18/2019 03:06 AM, Daniel Verkamp wrote:
> > Ensure that elements of the array that correspond to unavailable
> > features are set to NULL; previously, they would be left
uninitialized.
> > 
> > Since the corresponding names array elements were explicitly set to
> > NULL, the uninitialized callback pointers would not actually be
> > dereferenced; however, the uninitialized callbacks elements would
still
> > be read in vp_find_vqs_msix() and used to calculate the number of
MSI-X
> > vectors required.
> 
> With your 2nd patch:
> if (names[i] && callbacks[i])
>     ++nvectors;
> 
> It seems this patch isn't necessary as names[i] is already NULL,
isn't it?
> 
> Best,
> Wei
Right but passing uninitialized values isn't nice even if
the function called happens to ignore them.

On Tue, 17 Dec 2019 11:06:09 -0800
Daniel Verkamp <dverkamp at chromium.org> wrote:
> Ensure that elements of the array that correspond to unavailable
s/array/callbacks array/ ?
> features are set to NULL; previously, they would be left uninitialized.
> 
> Since the corresponding names array elements were explicitly set to
> NULL, the uninitialized callback pointers would not actually be
> dereferenced; however, the uninitialized callbacks elements would still
> be read in vp_find_vqs_msix() and used to calculate the number of MSI-X
> vectors required.
> 
> Fixes: 86a559787e6f ("virtio-balloon:
VIRTIO_BALLOON_F_FREE_PAGE_HINT")
> Signed-off-by: Daniel Verkamp <dverkamp at chromium.org>
> ---
>  drivers/virtio/virtio_balloon.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_balloon.c
b/drivers/virtio/virtio_balloon.c
> index 93f995f6cf36..8e400ece9273 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -475,7 +475,9 @@ static int init_vqs(struct virtio_balloon *vb)
>  	names[VIRTIO_BALLOON_VQ_INFLATE] = "inflate";
>  	callbacks[VIRTIO_BALLOON_VQ_DEFLATE] = balloon_ack;
>  	names[VIRTIO_BALLOON_VQ_DEFLATE] = "deflate";
> +	callbacks[VIRTIO_BALLOON_VQ_STATS] = NULL;
>  	names[VIRTIO_BALLOON_VQ_STATS] = NULL;
> +	callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
>  	names[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
>  
>  	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
Alternatively, it might make sense to zero the whole array and drop the
explicit NULL assignments, which would also be a bit more future
proof... but this one fixes things as well, of course.

Reviewed-by: Cornelia Huck <cohuck at redhat.com>

On Tue, 17 Dec 2019 11:06:10 -0800
Daniel Verkamp <dverkamp at chromium.org> wrote:
> VQs without a name specified are not valid; they are skipped in the
> later loop that assigns MSI-X vectors to queues, but the per_vq_vectors
> loop above that counts the required number of vectors previously still
> counted any queue with a non-NULL callback as needing a vector.
> 
> Add a check to the per_vq_vectors loop so that vectors with no name are
> not counted to make the two loops consistent.  This prevents
> over-counting unnecessary vectors (e.g. for features which were not
> negotiated with the device).
> 
> Signed-off-by: Daniel Verkamp <dverkamp at chromium.org>
> ---
>  drivers/virtio/virtio_pci_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
> index f2862f66c2ac..222d630c41fc 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -294,7 +294,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev,
unsigned nvqs,
>  		/* Best option: one for change interrupt, one per vq. */
>  		nvectors = 1;
>  		for (i = 0; i < nvqs; ++i)
> -			if (callbacks[i])
> +			if (names[i] && callbacks[i])
>  				++nvectors;
>  	} else {
>  		/* Second best: one for change, shared for all vqs. */
Reviewed-by: Cornelia Huck <cohuck at redhat.com>