Re: [PATCH] virtio_balloon: prevent uninitialized variable use
On Tue, Mar 28, 2017 at 6:18 PM, Michael S. Tsirkin wrote:
> On Mon, Mar 27, 2017 at 12:02:33PM +0200, Ladi Prosek wrote:
>> On Fri, Mar 24, 2017 at 9:59 PM, Michael S. Tsirkin wrote:
>> > On Fri, Mar 24, 2017 at 09:40:07PM +0100, Arnd Bergmann wrote:
>> >> On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek wrote:
>> >> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand
>> >> > wrote:
>> >> >> On 23.03.2017 16:17, Arnd Bergmann wrote:
>> >> >>> The latest gcc-7.0.1 snapshot reports a new warning:
>> >> >>>
>> >> >>> virtio/virtio_balloon.c: In function 'update_balloon_stats':
>> >> >>> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used
>> >> >>> uninitialized in this function [-Werror=uninitialized]
>> >> >>> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used
>> >> >>> uninitialized in this function [-Werror=uninitialized]
>> >> >>> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used
>> >> >>> uninitialized in this function [-Werror=uninitialized]
>> >> >>> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used
>> >> >>> uninitialized in this function [-Werror=uninitialized]
>> >> >>>
>> >> >>> This seems absolutely right, so we should add an extra check to
>> >> >>> prevent copying uninitialized stack data into the statistics.
>> >> >>> From all I can tell, this has been broken since the statistics code
>> >> >>> was originally added in 2.6.34.
>> >> >>>
>> >> >>> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the
>> >> >>> balloon driver (V4)")
>> >> >>> Signed-off-by: Arnd Bergmann
>> >> >>> ---
>> >> >>> 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 4e1191508228..cd5c54e2003d 100644
>> >> >>> --- a/drivers/virtio/virtio_balloon.c
>> >> >>> +++ b/drivers/virtio/virtio_balloon.c
>> >> >>> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct
>> >> >>> virtio_balloon *vb)
>> >> >>>
>> >> >>> available = si_mem_available();
>> >> >>>
>> >> >>> +#ifdef CONFIG_VM_EVENT_COUNTERS
>> >> >>> update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
>> >> >>> pages_to_bytes(events[PSWPIN]));
>> >> >>> update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
>> >> >>> pages_to_bytes(events[PSWPOUT]));
>> >> >>> update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT,
>> >> >>> events[PGMAJFAULT]);
>> >> >>> update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT,
>> >> >>> events[PGFAULT]);
>> >> >>> +#endif
>> >> >>> update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>> >> >>> pages_to_bytes(i.freeram));
>> >> >>> update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
>> >> >
>> >> > This will leave four uninitialized slots in vb->stats if
>> >> > CONFIG_VM_EVENT_COUNTERS is not defined. update_balloon_stats should
>> >> > have
>> >> >
>> >> > BUG_ON(idx < VIRTIO_BALLOON_S_NR);
>> >> >
>> >> > at the end.
>> >> >
>> >> > You need to make sure that vb->stats is smaller, either by using
>> >> > something else than VIRTIO_BALLOON_S_NR for its size or something else
>> >> > than sizeof(vb->stats) as the last argument to sg_init_one in this
>> >> > file.
>> >>
>> >> Ah, got it. Would one of you create a fixed patch for this, or should I?
>>
>> I will do it. If we go the route of returning 'idx' from
>> update_balloon_stats, my patch
>>
>>virtio_balloon: don't push uninitialized buffers to stats virt
[PATCH v3 3/3] virtio_balloon: prevent uninitialized variable use
From: Arnd Bergmann
The latest gcc-7.0.1 snapshot reports a new warning:
virtio/virtio_balloon.c: In function 'update_balloon_stats':
virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in
this function [-Werror=uninitialized]
virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in
this function [-Werror=uninitialized]
virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in
this function [-Werror=uninitialized]
virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in
this function [-Werror=uninitialized]
This seems absolutely right, so we should add an extra check to
prevent copying uninitialized stack data into the statistics.
>From all I can tell, this has been broken since the statistics code
was originally added in 2.6.34.
Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon
driver (V4)")
Signed-off-by: Arnd Bergmann
Signed-off-by: Ladi Prosek
---
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 d9544db..34adf9b 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -254,12 +254,14 @@ static unsigned int update_balloon_stats(struct
virtio_balloon *vb)
available = si_mem_available();
+#ifdef CONFIG_VM_EVENT_COUNTERS
update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
pages_to_bytes(events[PSWPIN]));
update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
pages_to_bytes(events[PSWPOUT]));
update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
+#endif
update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
pages_to_bytes(i.freeram));
update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
--
2.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 2/3] virtio-balloon: use actual number of stats for stats queue buffers
The virtio balloon driver contained a not-so-obvious invariant that
update_balloon_stats has to update exactly VIRTIO_BALLOON_S_NR counters
in order to send valid stats to the host. This commit fixes it by having
update_balloon_stats return the actual number of counters, and its
callers use it when pushing buffers to the stats virtqueue.
Note that it is still out of spec to change the number of counters
at run-time. "Driver MUST supply the same subset of statistics in all
buffers submitted to the statsq."
Suggested-by: Arnd Bergmann
Signed-off-by: Ladi Prosek
---
drivers/virtio/virtio_balloon.c | 17 ++---
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index fcd06e1..d9544db 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -242,11 +242,11 @@ static inline void update_stat(struct virtio_balloon *vb,
int idx,
#define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
-static void update_balloon_stats(struct virtio_balloon *vb)
+static unsigned int update_balloon_stats(struct virtio_balloon *vb)
{
unsigned long events[NR_VM_EVENT_ITEMS];
struct sysinfo i;
- int idx = 0;
+ unsigned int idx = 0;
long available;
all_vm_events(events);
@@ -266,6 +266,8 @@ static void update_balloon_stats(struct virtio_balloon *vb)
pages_to_bytes(i.totalram));
update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
pages_to_bytes(available));
+
+ return idx;
}
/*
@@ -291,14 +293,14 @@ static void stats_handle_request(struct virtio_balloon
*vb)
{
struct virtqueue *vq;
struct scatterlist sg;
- unsigned int len;
+ unsigned int len, num_stats;
- update_balloon_stats(vb);
+ num_stats = update_balloon_stats(vb);
vq = vb->stats_vq;
if (!virtqueue_get_buf(vq, &len))
return;
- sg_init_one(&sg, vb->stats, sizeof(vb->stats));
+ sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
virtqueue_kick(vq);
}
@@ -423,15 +425,16 @@ static int init_vqs(struct virtio_balloon *vb)
vb->deflate_vq = vqs[1];
if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
struct scatterlist sg;
+ unsigned int num_stats;
vb->stats_vq = vqs[2];
/*
* Prime this virtqueue with one buffer so the hypervisor can
* use it to signal us later (it can't be broken yet!).
*/
- update_balloon_stats(vb);
+ num_stats = update_balloon_stats(vb);
- sg_init_one(&sg, vb->stats, sizeof vb->stats);
+ sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
< 0)
BUG();
--
2.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 1/3] virtio_balloon: don't push uninitialized buffers to stats virtqueue
When init_vqs runs, virtio_balloon.stats is either uninitialized or contains stale values. The host updates its state with garbage data because it has no way of knowing that this is just a marker buffer used for signaling. This patch updates the stats before pushing the initial buffer. Signed-off-by: Ladi Prosek --- 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 4e11915..fcd06e1 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -429,6 +429,8 @@ static int init_vqs(struct virtio_balloon *vb) * Prime this virtqueue with one buffer so the hypervisor can * use it to signal us later (it can't be broken yet!). */ + update_balloon_stats(vb); + sg_init_one(&sg, vb->stats, sizeof vb->stats); if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL) < 0) -- 2.9.3 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 0/3] virtio_balloon: don't push uninitialized buffers to stats virtqueue
This series fixes issues with variable initialization in the virtio balloon driver which manifest as the driver sending invalid memory stats to the host. v1->v2: * Call update_balloon_stats instead of filling the buffer with invalid tags. * Add BUG_ON to update_balloon_stats to formalize the invariant that all slots in the buffer must be initialized. v2->v3: * Remove BUG_ON and return the actual number of counters from update_balloon_stats instead. * Added Arnd's patch to omit VM stats if CONFIG_VM_EVENT_COUNTERS is not defined. Arnd Bergmann (1): virtio_balloon: prevent uninitialized variable use Ladi Prosek (2): virtio_balloon: don't push uninitialized buffers to stats virtqueue virtio-balloon: use actual number of stats for stats queue buffers drivers/virtio/virtio_balloon.c | 19 +-- 1 file changed, 13 insertions(+), 6 deletions(-) Thanks! Ladi ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_balloon: prevent uninitialized variable use
On Fri, Mar 24, 2017 at 9:59 PM, Michael S. Tsirkin wrote:
> On Fri, Mar 24, 2017 at 09:40:07PM +0100, Arnd Bergmann wrote:
>> On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek wrote:
>> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand
>> > wrote:
>> >> On 23.03.2017 16:17, Arnd Bergmann wrote:
>> >>> The latest gcc-7.0.1 snapshot reports a new warning:
>> >>>
>> >>> virtio/virtio_balloon.c: In function 'update_balloon_stats':
>> >>> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized
>> >>> in this function [-Werror=uninitialized]
>> >>> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized
>> >>> in this function [-Werror=uninitialized]
>> >>> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used
>> >>> uninitialized in this function [-Werror=uninitialized]
>> >>> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used
>> >>> uninitialized in this function [-Werror=uninitialized]
>> >>>
>> >>> This seems absolutely right, so we should add an extra check to
>> >>> prevent copying uninitialized stack data into the statistics.
>> >>> From all I can tell, this has been broken since the statistics code
>> >>> was originally added in 2.6.34.
>> >>>
>> >>> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the
>> >>> balloon driver (V4)")
>> >>> Signed-off-by: Arnd Bergmann
>> >>> ---
>> >>> 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 4e1191508228..cd5c54e2003d 100644
>> >>> --- a/drivers/virtio/virtio_balloon.c
>> >>> +++ b/drivers/virtio/virtio_balloon.c
>> >>> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct
>> >>> virtio_balloon *vb)
>> >>>
>> >>> available = si_mem_available();
>> >>>
>> >>> +#ifdef CONFIG_VM_EVENT_COUNTERS
>> >>> update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
>> >>> pages_to_bytes(events[PSWPIN]));
>> >>> update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
>> >>> pages_to_bytes(events[PSWPOUT]));
>> >>> update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT,
>> >>> events[PGMAJFAULT]);
>> >>> update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
>> >>> +#endif
>> >>> update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>> >>> pages_to_bytes(i.freeram));
>> >>> update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
>> >
>> > This will leave four uninitialized slots in vb->stats if
>> > CONFIG_VM_EVENT_COUNTERS is not defined. update_balloon_stats should
>> > have
>> >
>> > BUG_ON(idx < VIRTIO_BALLOON_S_NR);
>> >
>> > at the end.
>> >
>> > You need to make sure that vb->stats is smaller, either by using
>> > something else than VIRTIO_BALLOON_S_NR for its size or something else
>> > than sizeof(vb->stats) as the last argument to sg_init_one in this
>> > file.
>>
>> Ah, got it. Would one of you create a fixed patch for this, or should I?
I will do it. If we go the route of returning 'idx' from
update_balloon_stats, my patch
virtio_balloon: don't push uninitialized buffers to stats virtqueue
needs to be redone anyway. Thanks!
>> An easy way to solve it would be to preinitialize the events array
>> and return zeroes to the host, but I don't know if that's the right
>> solution.
>
>
> I think that's out of spec.
>
>> Another option would be to return 'idx' from update_balloon_stats,
>> and use that for the size:
>>
>> diff --git a/drivers/virtio/virtio_balloon.c
>> b/drivers/virtio/virtio_balloon.c
>> index cd5c54e2003d..bea3cfb88e53 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -242,7 +242,7 @@ static inline void update_stat(struct
>> virtio_balloon *vb, int idx,
>>
>> #define pages_to_bytes(x) ((u64)(x) << P
Re: [PATCH] virtio_balloon: prevent uninitialized variable use
On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand wrote:
> On 23.03.2017 16:17, Arnd Bergmann wrote:
>> The latest gcc-7.0.1 snapshot reports a new warning:
>>
>> virtio/virtio_balloon.c: In function 'update_balloon_stats':
>> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in
>> this function [-Werror=uninitialized]
>> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in
>> this function [-Werror=uninitialized]
>> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in
>> this function [-Werror=uninitialized]
>> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in
>> this function [-Werror=uninitialized]
>>
>> This seems absolutely right, so we should add an extra check to
>> prevent copying uninitialized stack data into the statistics.
>> From all I can tell, this has been broken since the statistics code
>> was originally added in 2.6.34.
>>
>> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon
>> driver (V4)")
>> Signed-off-by: Arnd Bergmann
>> ---
>> 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 4e1191508228..cd5c54e2003d 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon
>> *vb)
>>
>> available = si_mem_available();
>>
>> +#ifdef CONFIG_VM_EVENT_COUNTERS
>> update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
>> pages_to_bytes(events[PSWPIN]));
>> update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
>> pages_to_bytes(events[PSWPOUT]));
>> update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
>> update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
>> +#endif
>> update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>> pages_to_bytes(i.freeram));
>> update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
This will leave four uninitialized slots in vb->stats if
CONFIG_VM_EVENT_COUNTERS is not defined. update_balloon_stats should
have
BUG_ON(idx < VIRTIO_BALLOON_S_NR);
at the end.
You need to make sure that vb->stats is smaller, either by using
something else than VIRTIO_BALLOON_S_NR for its size or something else
than sizeof(vb->stats) as the last argument to sg_init_one in this
file.
> CC'ing Ladi
Thanks!
> --
>
> Thanks,
>
> David
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2] virtio_balloon: don't push uninitialized buffers to stats virtqueue
When init_vqs runs, virtio_balloon.stats is either uninitialized or contains stale values. The host updates its state with garbage data because it has no way of knowing that this is just a marker buffer used for signaling. This patch updates the stats before pushing the initial buffer. An assertion is also added to update_balloon_stats to make sure that all stats fields are initialized. Alternative fixes: * Push an empty buffer in init_vqs. Not easily done with the current virtio implementation and violates the spec "Driver MUST supply the same subset of statistics in all buffers submitted to the statsq". * Push a buffer with invalid tags in init_vqs. Violates the same spec clause, plus "invalid tag" is not really defined. Signed-off-by: Ladi Prosek --- v1->v2: * Call update_balloon_stats instead of filling the buffer with invalid tags. * Add BUG_ON to update_balloon_stats to formalize the invariant that all slots in the buffer must be initialized. Also, I just noticed this paragraph in the spec: "When using the legacy interface, the device SHOULD ignore all values in the first buffer in the statsq supplied by the driver after device initialization. Note: Historically, drivers supplied an uninitialized buffer in the first buffer." So someone knew about this bug but didn't fix the Linux driver and also didn't implement the SHOULD in QEMU. Makes me wonder if I'm missing something here. Thanks! drivers/virtio/virtio_balloon.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 4e11915..42dc35f 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -266,6 +266,8 @@ static void update_balloon_stats(struct virtio_balloon *vb) pages_to_bytes(i.totalram)); update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL, pages_to_bytes(available)); + + BUG_ON(idx < VIRTIO_BALLOON_S_NR); } /* @@ -429,6 +431,8 @@ static int init_vqs(struct virtio_balloon *vb) * Prime this virtqueue with one buffer so the hypervisor can * use it to signal us later (it can't be broken yet!). */ + update_balloon_stats(vb); + sg_init_one(&sg, vb->stats, sizeof vb->stats); if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL) < 0) -- 2.7.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_balloon: don't push uninitialized buffers to stats virtqueue
On Wed, Mar 22, 2017 at 6:15 PM, Michael S. Tsirkin wrote:
> On Wed, Mar 22, 2017 at 06:05:39PM +0100, Ladi Prosek wrote:
>> On Wed, Mar 22, 2017 at 5:14 PM, Michael S. Tsirkin wrote:
>> > On Wed, Mar 22, 2017 at 04:10:27PM +0100, Ladi Prosek wrote:
>> >> When init_vqs runs, virtio_balloon.stats is either uninitialized or
>> >> contains stale values. The host updates its state with garbage data
>> >> because it has no way of knowing that this is just a marker buffer
>> >> used for signaling.
>> >>
>> >> This patch initializes all tags with U16_MAX which is guaranteed to
>> >> be ignored by the host.
>> >>
>> >> The alternative fix would be to push an empty buffer in init_vqs but
>> >> that's not easily done with the current virtio implementation and
>> >> would still not eliminate the invariant that update_balloon_stats
>> >> has to update all VIRTIO_BALLOON_S_NR fields.
>> >>
>> >> Signed-off-by: Ladi Prosek
>> >> ---
>> >> drivers/virtio/virtio_balloon.c | 4
>> >> 1 file changed, 4 insertions(+)
>> >>
>> >> diff --git a/drivers/virtio/virtio_balloon.c
>> >> b/drivers/virtio/virtio_balloon.c
>> >> index 4e11915..d34f56f 100644
>> >> --- a/drivers/virtio/virtio_balloon.c
>> >> +++ b/drivers/virtio/virtio_balloon.c
>> >> @@ -423,12 +423,16 @@ static int init_vqs(struct virtio_balloon *vb)
>> >> vb->deflate_vq = vqs[1];
>> >> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
>> >> struct scatterlist sg;
>> >> + int idx;
>> >> vb->stats_vq = vqs[2];
>> >>
>> >> /*
>> >>* Prime this virtqueue with one buffer so the hypervisor
>> >> can
>> >>* use it to signal us later (it can't be broken yet!).
>> >>*/
>> >> + for (idx = 0; idx < VIRTIO_BALLOON_S_NR; idx++)
>> >> + update_stat(vb, idx, U16_MAX, 0);
>> >> +
>> >
>> > Can't we fill in actual values?
>>
>> We can if we're fine with the unsolicited update.
>>
>> In QEMU, this would mean that you'll see valid stats in
>> /machine/peripheral/balloon0/guest-stats without setting the
>> guest-stats-polling-interval. And they would be updated on driver
>> (re-)load, so kind of arbitrarily from host pov.
>
> IUC the only issue would be that someone might come to
> depend on this. Doesn't look like a big deal to me.
Ok, I'll post v2 of the patch to use actual values when sending the
initial buffer.
Btw, I checked the Windows driver and it's been sending all FF's in
the first stats buffer since about 2011. Just for completeness,
probably won't change your mind :)
Thanks!
Ladi
>> On the other hand, this patch does not really comply with (5.5.6.3.1
>> Driver Requirements: Memory Statistics):
>>
>> "Driver MUST supply the same subset of statistics in all buffers
>> submitted to the statsq."
>>
>> So going by the letter of the law, we should fill in actual values.
>> Going by the (the purpose of the initial buffer is to wait for the
>> host to request stats), we should push an empty buffer or invalid
>> values.
>> >> sg_init_one(&sg, vb->stats, sizeof vb->stats);
>> >> if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb,
>> >> GFP_KERNEL)
>> >> < 0)
>> >> --
>> >> 2.7.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_balloon: don't push uninitialized buffers to stats virtqueue
On Wed, Mar 22, 2017 at 5:14 PM, Michael S. Tsirkin wrote:
> On Wed, Mar 22, 2017 at 04:10:27PM +0100, Ladi Prosek wrote:
>> When init_vqs runs, virtio_balloon.stats is either uninitialized or
>> contains stale values. The host updates its state with garbage data
>> because it has no way of knowing that this is just a marker buffer
>> used for signaling.
>>
>> This patch initializes all tags with U16_MAX which is guaranteed to
>> be ignored by the host.
>>
>> The alternative fix would be to push an empty buffer in init_vqs but
>> that's not easily done with the current virtio implementation and
>> would still not eliminate the invariant that update_balloon_stats
>> has to update all VIRTIO_BALLOON_S_NR fields.
>>
>> Signed-off-by: Ladi Prosek
>> ---
>> drivers/virtio/virtio_balloon.c | 4
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/virtio/virtio_balloon.c
>> b/drivers/virtio/virtio_balloon.c
>> index 4e11915..d34f56f 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -423,12 +423,16 @@ static int init_vqs(struct virtio_balloon *vb)
>> vb->deflate_vq = vqs[1];
>> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
>> struct scatterlist sg;
>> + int idx;
>> vb->stats_vq = vqs[2];
>>
>> /*
>>* Prime this virtqueue with one buffer so the hypervisor can
>>* use it to signal us later (it can't be broken yet!).
>>*/
>> + for (idx = 0; idx < VIRTIO_BALLOON_S_NR; idx++)
>> + update_stat(vb, idx, U16_MAX, 0);
>> +
>
> Can't we fill in actual values?
We can if we're fine with the unsolicited update.
In QEMU, this would mean that you'll see valid stats in
/machine/peripheral/balloon0/guest-stats without setting the
guest-stats-polling-interval. And they would be updated on driver
(re-)load, so kind of arbitrarily from host pov.
On the other hand, this patch does not really comply with (5.5.6.3.1
Driver Requirements: Memory Statistics):
"Driver MUST supply the same subset of statistics in all buffers
submitted to the statsq."
So going by the letter of the law, we should fill in actual values.
Going by the (the purpose of the initial buffer is to wait for the
host to request stats), we should push an empty buffer or invalid
values.
>> sg_init_one(&sg, vb->stats, sizeof vb->stats);
>> if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
>> < 0)
>> --
>> 2.7.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio_balloon: don't push uninitialized buffers to stats virtqueue
When init_vqs runs, virtio_balloon.stats is either uninitialized or
contains stale values. The host updates its state with garbage data
because it has no way of knowing that this is just a marker buffer
used for signaling.
This patch initializes all tags with U16_MAX which is guaranteed to
be ignored by the host.
The alternative fix would be to push an empty buffer in init_vqs but
that's not easily done with the current virtio implementation and
would still not eliminate the invariant that update_balloon_stats
has to update all VIRTIO_BALLOON_S_NR fields.
Signed-off-by: Ladi Prosek
---
drivers/virtio/virtio_balloon.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 4e11915..d34f56f 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -423,12 +423,16 @@ static int init_vqs(struct virtio_balloon *vb)
vb->deflate_vq = vqs[1];
if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
struct scatterlist sg;
+ int idx;
vb->stats_vq = vqs[2];
/*
* Prime this virtqueue with one buffer so the hypervisor can
* use it to signal us later (it can't be broken yet!).
*/
+ for (idx = 0; idx < VIRTIO_BALLOON_S_NR; idx++)
+ update_stat(vb, idx, U16_MAX, 0);
+
sg_init_one(&sg, vb->stats, sizeof vb->stats);
if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
< 0)
--
2.7.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2] virtio_ring: Make interrupt suppression spec compliant
According to the spec, if the VIRTIO_RING_F_EVENT_IDX feature bit is
negotiated the driver MUST set flags to 0. Not dirtying the available
ring in virtqueue_disable_cb also has a minor positive performance
impact, improving L1 dcache load missed by ~0.5% in vring_bench.
Writes to the used event field (vring_used_event) are still unconditional.
Cc: Michael S. Tsirkin
Cc: # f277ec4 virtio_ring: shadow available
Cc:
Signed-off-by: Ladi Prosek
---
v1->v2:
* fixed coding style
* perf measurement results added to commit message
* patch sent to virtualization@lists.linux-foundation.org (was
k...@vger.kernel.org)
drivers/virtio/virtio_ring.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index e383ecd..926ecb7 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -732,7 +732,8 @@ void virtqueue_disable_cb(struct virtqueue *_vq)
if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
- vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev,
vq->avail_flags_shadow);
+ if (!vq->event)
+ vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev,
vq->avail_flags_shadow);
}
}
@@ -764,7 +765,8 @@ unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq)
* entry. Always do both to keep code simple. */
if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
- vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev,
vq->avail_flags_shadow);
+ if (!vq->event)
+ vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev,
vq->avail_flags_shadow);
}
vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, last_used_idx
= vq->last_used_idx);
END_USE(vq);
@@ -832,10 +834,11 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
* more to do. */
/* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
* either clear the flags bit or point the event index at the next
-* entry. Always do both to keep code simple. */
+* entry. Always update the event index to keep code simple. */
if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
- vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev,
vq->avail_flags_shadow);
+ if (!vq->event)
+ vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev,
vq->avail_flags_shadow);
}
/* TODO: tune this threshold */
bufs = (u16)(vq->avail_idx_shadow - vq->last_used_idx) * 3 / 4;
@@ -953,7 +956,8 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
/* No callback? Tell other side not to bother us. */
if (!callback) {
vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
- vq->vring.avail->flags = cpu_to_virtio16(vdev,
vq->avail_flags_shadow);
+ if (!vq->event)
+ vq->vring.avail->flags = cpu_to_virtio16(vdev,
vq->avail_flags_shadow);
}
/* Put everything in free lists. */
--
2.5.5
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
