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 <[email protected]>
---
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,
--
2.9.0
On 03/23/2017 06:17 PM, 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 <[email protected]>
Reviewed-by: Denis V. Lunev <[email protected]>
> ---
> 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,
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 <[email protected]>
> ---
> 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,
>
CC'ing Ladi
--
Thanks,
David
On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <[email protected]> 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 <[email protected]>
>> ---
>> 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
On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek <[email protected]> wrote:
> On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <[email protected]> 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 <[email protected]>
>>> ---
>>> 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?
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.
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) << 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;
@@ -268,6 +268,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;
}
/*
@@ -294,13 +296,14 @@ static void stats_handle_request(struct
virtio_balloon *vb)
struct virtqueue *vq;
struct scatterlist sg;
unsigned int len;
+ unsigned int 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);
}
Arnd
On Fri, Mar 24, 2017 at 09:40:07PM +0100, Arnd Bergmann wrote:
> On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek <[email protected]> wrote:
> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <[email protected]> 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 <[email protected]>
> >>> ---
> >>> 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?
>
> 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) << 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;
> @@ -268,6 +268,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;
> }
>
> /*
> @@ -294,13 +296,14 @@ static void stats_handle_request(struct
> virtio_balloon *vb)
> struct virtqueue *vq;
> struct scatterlist sg;
> unsigned int len;
> + unsigned int 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);
> }
>
> Arnd
Sounds reasonable.
--
MST
On Fri, Mar 24, 2017 at 9:59 PM, Michael S. Tsirkin <[email protected]> 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 <[email protected]> wrote:
>> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <[email protected]> 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 <[email protected]>
>> >>> ---
>> >>> 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) << 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;
>> @@ -268,6 +268,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;
>> }
>>
>> /*
>> @@ -294,13 +296,14 @@ static void stats_handle_request(struct
>> virtio_balloon *vb)
>> struct virtqueue *vq;
>> struct scatterlist sg;
>> unsigned int len;
>> + unsigned int 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);
>> }
>>
>> Arnd
>
> Sounds reasonable.
>
> --
> MST
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 <[email protected]> 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 <[email protected]> wrote:
> >> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <[email protected]> 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 <[email protected]>
> >> >>> ---
> >> >>> 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!
Can you do it soon pls? I'd like to do a pull request.
> >> 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) << 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;
> >> @@ -268,6 +268,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;
> >> }
> >>
> >> /*
> >> @@ -294,13 +296,14 @@ static void stats_handle_request(struct
> >> virtio_balloon *vb)
> >> struct virtqueue *vq;
> >> struct scatterlist sg;
> >> unsigned int len;
> >> + unsigned int 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);
> >> }
> >>
> >> Arnd
> >
> > Sounds reasonable.
> >
> > --
> > MST
On Tue, Mar 28, 2017 at 6:18 PM, Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
>> >> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <[email protected]> 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 <[email protected]>
>> >> >>> ---
>> >> >>> 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!
>
> Can you do it soon pls? I'd like to do a pull request.
Done.
[PATCH v3 0/3] virtio_balloon: don't push uninitialized buffers to
stats virtqueue
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) << 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;
>> >> @@ -268,6 +268,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;
>> >> }
>> >>
>> >> /*
>> >> @@ -294,13 +296,14 @@ static void stats_handle_request(struct
>> >> virtio_balloon *vb)
>> >> struct virtqueue *vq;
>> >> struct scatterlist sg;
>> >> unsigned int len;
>> >> + unsigned int 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);
>> >> }
>> >>
>> >> Arnd
>> >
>> > Sounds reasonable.
>> >
>> > --
>> > MST