Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1607597 > unrolled thread

[PATCH] virtio_balloon: prevent uninitialized variable use

Started byArnd Bergmann <arnd@arndb.de>
First post2017-03-23 16:20 +0100
Last post2017-03-27 12:10 +0200
Articles 6 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] virtio_balloon: prevent uninitialized variable use Arnd Bergmann <arnd@arndb.de> - 2017-03-23 16:20 +0100
    Re: [PATCH] virtio_balloon: prevent uninitialized variable use David Hildenbrand <david@redhat.com> - 2017-03-24 19:40 +0100
      Re: [PATCH] virtio_balloon: prevent uninitialized variable use Ladi Prosek <lprosek@redhat.com> - 2017-03-24 21:20 +0100
        Re: [PATCH] virtio_balloon: prevent uninitialized variable use Arnd Bergmann <arnd@arndb.de> - 2017-03-24 21:50 +0100
          Re: [PATCH] virtio_balloon: prevent uninitialized variable use "Michael S. Tsirkin" <mst@redhat.com> - 2017-03-24 22:00 +0100
            Re: [PATCH] virtio_balloon: prevent uninitialized variable use Ladi Prosek <lprosek@redhat.com> - 2017-03-27 12:10 +0200

#1607597 — [PATCH] virtio_balloon: prevent uninitialized variable use

FromArnd Bergmann <arnd@arndb.de>
Date2017-03-23 16:20 +0100
Subject[PATCH] virtio_balloon: prevent uninitialized variable use
Message-ID<tocGS-3Wl-5@gated-at.bofh.it>
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 <arnd@arndb.de>
---
 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

[toc] | [next] | [standalone]

#1608854

FromDavid Hildenbrand <david@redhat.com>
Date2017-03-24 19:40 +0100
Message-ID<toChY-5vt-1@gated-at.bofh.it>
In reply to#1607597
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 <arnd@arndb.de>
> ---
>  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

[toc] | [prev] | [next] | [standalone]

#1608950

FromLadi Prosek <lprosek@redhat.com>
Date2017-03-24 21:20 +0100
Message-ID<toDQK-6Kj-21@gated-at.bofh.it>
In reply to#1608854
On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <david@redhat.com> 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 <arnd@arndb.de>
>> ---
>>  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

[toc] | [prev] | [next] | [standalone]

#1608969

FromArnd Bergmann <arnd@arndb.de>
Date2017-03-24 21:50 +0100
Message-ID<toEjM-6Xy-19@gated-at.bofh.it>
In reply to#1608950
On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek <lprosek@redhat.com> wrote:
> On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <david@redhat.com> 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 <arnd@arndb.de>
>>> ---
>>>  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

[toc] | [prev] | [next] | [standalone]

#1608970

From"Michael S. Tsirkin" <mst@redhat.com>
Date2017-03-24 22:00 +0100
Message-ID<toEts-71u-7@gated-at.bofh.it>
In reply to#1608969
On Fri, Mar 24, 2017 at 09:40:07PM +0100, Arnd Bergmann wrote:
> On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek <lprosek@redhat.com> wrote:
> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <david@redhat.com> 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 <arnd@arndb.de>
> >>> ---
> >>>  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

[toc] | [prev] | [next] | [standalone]

#1609687

FromLadi Prosek <lprosek@redhat.com>
Date2017-03-27 12:10 +0200
Message-ID<tpzL5-6pP-45@gated-at.bofh.it>
In reply to#1608970
On Fri, Mar 24, 2017 at 9:59 PM, Michael S. Tsirkin <mst@redhat.com> 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 <lprosek@redhat.com> wrote:
>> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <david@redhat.com> 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 <arnd@arndb.de>
>> >>> ---
>> >>>  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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web