Re: [PATCH 1/2] virtio-balloon: initialize all vq callbacks
On Tue, 17 Dec 2019 11:06:09 -0800
Daniel Verkamp 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
> ---
> 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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] virtio-balloon: initialize all vq callbacks
On 12/18/2019 01:19 PM, Michael S. Tsirkin wrote:
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.
Hmm.. then please make one more change:
if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
names[VIRTIO_BALLOON_VQ_FREE_PAGE] = "free_page_vq";
-callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
}
And I think all the commit description isn't accurate then, it seems to
be a coding style improvement,
instead of affecting "calculate the number of MSI-X vectors required".
Best,
Wei
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] virtio-balloon: initialize all vq callbacks
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. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] virtio-balloon: initialize all vq callbacks
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] virtio-balloon: initialize all vq callbacks
On Tue, Dec 17, 2019 at 12:05 PM Michael S. Tsirkin 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
>
> 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.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] virtio-balloon: initialize all vq callbacks
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
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 1/2] virtio-balloon: initialize all vq callbacks
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
---
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
