[PATCH] Initialize ioreq_t in hvmemul_do_io()

[PATCH] Initialize ioreq_t in hvmemul_do_io()

[Paul Durrant]

All relevant fields of the ioreq_t are properly initialized but coverity
complains about vp_eport being uninitialized (ID 1215178) because it is
the subject of a struct copy in hvm_send_assist_req().

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/emulate.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 904c71a..e516194 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -57,7 +57,7 @@ static int hvmemul_do_io(
     int value_is_ptr = (p_data == NULL);
     struct vcpu *curr = current;
     struct hvm_vcpu_io *vio;
-    ioreq_t p;
+    ioreq_t p = { 0 };
     unsigned long ram_gfn = paddr_to_pfn(ram_gpa);
     p2m_type_t p2mt;
     struct page_info *ram_page;
-- 
1.7.10.4

Re: [PATCH] Initialize ioreq_t in hvmemul_do_io()

[Jan Beulich]

>>> On 22.05.14 at 14:48, <paul.durrant@citrix.com> wrote:
> All relevant fields of the ioreq_t are properly initialized but coverity
> complains about vp_eport being uninitialized (ID 1215178) because it is
> the subject of a struct copy in hvm_send_assist_req().
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  xen/arch/x86/hvm/emulate.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 904c71a..e516194 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -57,7 +57,7 @@ static int hvmemul_do_io(
>      int value_is_ptr = (p_data == NULL);
>      struct vcpu *curr = current;
>      struct hvm_vcpu_io *vio;
> -    ioreq_t p;
> +    ioreq_t p = { 0 };

To be honest, I dislike fixes like this to address tool shortcomings. It's
not like this is giaramteed to be just a single instruction that gets
added (maybe the compiler can figure this, but I would want to rely
on it.

If it's known to just be one member, can't that (with a suitable but
brief comment) be added to the set of field initializations further
down in the function? That would still be code there just to please
Coverity, but it would be guaranteed to be a single instruction.

Btw., Andrew, can one identify at build time that a build is done
just for Coverity's inspection? If so, rather than a comment a
suitable preprocessor conditional might be enough documentation
in cases like this.

Jan

Re: [PATCH] Initialize ioreq_t in hvmemul_do_io()

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 22 May 2014 14:14
> To: Andrew Cooper; Paul Durrant
> Cc: xen-devel@lists.xen.org; Keir (Xen.org)
> Subject: Re: [PATCH] Initialize ioreq_t in hvmemul_do_io()
> 
> >>> On 22.05.14 at 14:48, <paul.durrant@citrix.com> wrote:
> > All relevant fields of the ioreq_t are properly initialized but coverity
> > complains about vp_eport being uninitialized (ID 1215178) because it is
> > the subject of a struct copy in hvm_send_assist_req().
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Cc: Keir Fraser <keir@xen.org>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> >  xen/arch/x86/hvm/emulate.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> > index 904c71a..e516194 100644
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -57,7 +57,7 @@ static int hvmemul_do_io(
> >      int value_is_ptr = (p_data == NULL);
> >      struct vcpu *curr = current;
> >      struct hvm_vcpu_io *vio;
> > -    ioreq_t p;
> > +    ioreq_t p = { 0 };
> 
> To be honest, I dislike fixes like this to address tool shortcomings. It's
> not like this is giaramteed to be just a single instruction that gets
> added (maybe the compiler can figure this, but I would want to rely
> on it.
> 

I could always change hvm_send_assist_req() to only copy the relevant fields from the proto structure, as it used to in earlier versions of my patch.

  Paul

> If it's known to just be one member, can't that (with a suitable but
> brief comment) be added to the set of field initializations further
> down in the function? That would still be code there just to please
> Coverity, but it would be guaranteed to be a single instruction.
> 
> Btw., Andrew, can one identify at build time that a build is done
> just for Coverity's inspection? If so, rather than a comment a
> suitable preprocessor conditional might be enough documentation
> in cases like this.
> 
> Jan

Re: [PATCH] Initialize ioreq_t in hvmemul_do_io()

[Jan Beulich]

>>> On 22.05.14 at 15:20, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 22 May 2014 14:14
>> To: Andrew Cooper; Paul Durrant
>> Cc: xen-devel@lists.xen.org; Keir (Xen.org)
>> Subject: Re: [PATCH] Initialize ioreq_t in hvmemul_do_io()
>> 
>> >>> On 22.05.14 at 14:48, <paul.durrant@citrix.com> wrote:
>> > All relevant fields of the ioreq_t are properly initialized but coverity
>> > complains about vp_eport being uninitialized (ID 1215178) because it is
>> > the subject of a struct copy in hvm_send_assist_req().
>> >
>> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>> > Cc: Keir Fraser <keir@xen.org>
>> > Cc: Jan Beulich <jbeulich@suse.com>
>> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> > ---
>> >  xen/arch/x86/hvm/emulate.c |    2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>> > index 904c71a..e516194 100644
>> > --- a/xen/arch/x86/hvm/emulate.c
>> > +++ b/xen/arch/x86/hvm/emulate.c
>> > @@ -57,7 +57,7 @@ static int hvmemul_do_io(
>> >      int value_is_ptr = (p_data == NULL);
>> >      struct vcpu *curr = current;
>> >      struct hvm_vcpu_io *vio;
>> > -    ioreq_t p;
>> > +    ioreq_t p = { 0 };
>> 
>> To be honest, I dislike fixes like this to address tool shortcomings. It's
>> not like this is giaramteed to be just a single instruction that gets
>> added (maybe the compiler can figure this, but I would want to rely
>> on it.
>> 
> 
> I could always change hvm_send_assist_req() to only copy the relevant fields 
> from the proto structure, as it used to in earlier versions of my patch.

But the structure copy is certainly more efficient than the piece-wise
copying.

Jan

Re: [PATCH] Initialize ioreq_t in hvmemul_do_io()

[Andrew Cooper]

On 22/05/14 14:27, Jan Beulich wrote:
>>>> On 22.05.14 at 15:20, <Paul.Durrant@citrix.com> wrote:
>>>  -----Original Message-----
>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>> Sent: 22 May 2014 14:14
>>> To: Andrew Cooper; Paul Durrant
>>> Cc: xen-devel@lists.xen.org; Keir (Xen.org)
>>> Subject: Re: [PATCH] Initialize ioreq_t in hvmemul_do_io()
>>>
>>>>>> On 22.05.14 at 14:48, <paul.durrant@citrix.com> wrote:
>>>> All relevant fields of the ioreq_t are properly initialized but coverity
>>>> complains about vp_eport being uninitialized (ID 1215178) because it is
>>>> the subject of a struct copy in hvm_send_assist_req().
>>>>
>>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>>>> Cc: Keir Fraser <keir@xen.org>
>>>> Cc: Jan Beulich <jbeulich@suse.com>
>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>>  xen/arch/x86/hvm/emulate.c |    2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>>>> index 904c71a..e516194 100644
>>>> --- a/xen/arch/x86/hvm/emulate.c
>>>> +++ b/xen/arch/x86/hvm/emulate.c
>>>> @@ -57,7 +57,7 @@ static int hvmemul_do_io(
>>>>      int value_is_ptr = (p_data == NULL);
>>>>      struct vcpu *curr = current;
>>>>      struct hvm_vcpu_io *vio;
>>>> -    ioreq_t p;
>>>> +    ioreq_t p = { 0 };
>>> To be honest, I dislike fixes like this to address tool shortcomings. It's
>>> not like this is giaramteed to be just a single instruction that gets
>>> added (maybe the compiler can figure this, but I would want to rely
>>> on it.
>>>
>> I could always change hvm_send_assist_req() to only copy the relevant fields 
>> from the proto structure, as it used to in earlier versions of my patch.
> But the structure copy is certainly more efficient than the piece-wise
> copying.
>
> Jan
>

When interpreting the coverity results, I accidentally missed the second
occurrence of this issue.

It is possible to "goto finished_access" and have p.addr and p.dir
uninitialised when tracing the IO.

By far the safest option is just initialise the entire structure to 0.

~Andrew

Re: [PATCH] Initialize ioreq_t in hvmemul_do_io()

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 22 May 2014 14:47
> To: Jan Beulich
> Cc: Paul Durrant; xen-devel@lists.xen.org; Keir (Xen.org)
> Subject: Re: [PATCH] Initialize ioreq_t in hvmemul_do_io()
> 
> On 22/05/14 14:27, Jan Beulich wrote:
> >>>> On 22.05.14 at 15:20, <Paul.Durrant@citrix.com> wrote:
> >>>  -----Original Message-----
> >>> From: Jan Beulich [mailto:JBeulich@suse.com]
> >>> Sent: 22 May 2014 14:14
> >>> To: Andrew Cooper; Paul Durrant
> >>> Cc: xen-devel@lists.xen.org; Keir (Xen.org)
> >>> Subject: Re: [PATCH] Initialize ioreq_t in hvmemul_do_io()
> >>>
> >>>>>> On 22.05.14 at 14:48, <paul.durrant@citrix.com> wrote:
> >>>> All relevant fields of the ioreq_t are properly initialized but coverity
> >>>> complains about vp_eport being uninitialized (ID 1215178) because it is
> >>>> the subject of a struct copy in hvm_send_assist_req().
> >>>>
> >>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> >>>> Cc: Keir Fraser <keir@xen.org>
> >>>> Cc: Jan Beulich <jbeulich@suse.com>
> >>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>> ---
> >>>>  xen/arch/x86/hvm/emulate.c |    2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/xen/arch/x86/hvm/emulate.c
> b/xen/arch/x86/hvm/emulate.c
> >>>> index 904c71a..e516194 100644
> >>>> --- a/xen/arch/x86/hvm/emulate.c
> >>>> +++ b/xen/arch/x86/hvm/emulate.c
> >>>> @@ -57,7 +57,7 @@ static int hvmemul_do_io(
> >>>>      int value_is_ptr = (p_data == NULL);
> >>>>      struct vcpu *curr = current;
> >>>>      struct hvm_vcpu_io *vio;
> >>>> -    ioreq_t p;
> >>>> +    ioreq_t p = { 0 };
> >>> To be honest, I dislike fixes like this to address tool shortcomings. It's
> >>> not like this is giaramteed to be just a single instruction that gets
> >>> added (maybe the compiler can figure this, but I would want to rely
> >>> on it.
> >>>
> >> I could always change hvm_send_assist_req() to only copy the relevant
> fields
> >> from the proto structure, as it used to in earlier versions of my patch.
> > But the structure copy is certainly more efficient than the piece-wise
> > copying.
> >
> > Jan
> >
> 
> When interpreting the coverity results, I accidentally missed the second
> occurrence of this issue.
> 
> It is possible to "goto finished_access" and have p.addr and p.dir
> uninitialised when tracing the IO.
> 

Unfortunately the ioreq passed to hvmtrace_io_assist() is just bogus in this case. I need to fix that too.

  Paul

> By far the safest option is just initialise the entire structure to 0.
> 
> ~Andrew

Re: [PATCH] Initialize ioreq_t in hvmemul_do_io()

[Jan Beulich]

>>> On 22.05.14 at 15:47, <andrew.cooper3@citrix.com> wrote:
> On 22/05/14 14:27, Jan Beulich wrote:
>>>>> On 22.05.14 at 15:20, <Paul.Durrant@citrix.com> wrote:
>>>>  -----Original Message-----
>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>> Sent: 22 May 2014 14:14
>>>> To: Andrew Cooper; Paul Durrant
>>>> Cc: xen-devel@lists.xen.org; Keir (Xen.org)
>>>> Subject: Re: [PATCH] Initialize ioreq_t in hvmemul_do_io()
>>>>
>>>>>>> On 22.05.14 at 14:48, <paul.durrant@citrix.com> wrote:
>>>>> All relevant fields of the ioreq_t are properly initialized but coverity
>>>>> complains about vp_eport being uninitialized (ID 1215178) because it is
>>>>> the subject of a struct copy in hvm_send_assist_req().
>>>>>
>>>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>>>>> Cc: Keir Fraser <keir@xen.org>
>>>>> Cc: Jan Beulich <jbeulich@suse.com>
>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> ---
>>>>>  xen/arch/x86/hvm/emulate.c |    2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>>>>> index 904c71a..e516194 100644
>>>>> --- a/xen/arch/x86/hvm/emulate.c
>>>>> +++ b/xen/arch/x86/hvm/emulate.c
>>>>> @@ -57,7 +57,7 @@ static int hvmemul_do_io(
>>>>>      int value_is_ptr = (p_data == NULL);
>>>>>      struct vcpu *curr = current;
>>>>>      struct hvm_vcpu_io *vio;
>>>>> -    ioreq_t p;
>>>>> +    ioreq_t p = { 0 };
>>>> To be honest, I dislike fixes like this to address tool shortcomings. It's
>>>> not like this is giaramteed to be just a single instruction that gets
>>>> added (maybe the compiler can figure this, but I would want to rely
>>>> on it.
>>>>
>>> I could always change hvm_send_assist_req() to only copy the relevant fields 
>>> from the proto structure, as it used to in earlier versions of my patch.
>> But the structure copy is certainly more efficient than the piece-wise
>> copying.
> 
> When interpreting the coverity results, I accidentally missed the second
> occurrence of this issue.
> 
> It is possible to "goto finished_access" and have p.addr and p.dir
> uninitialised when tracing the IO.
> 
> By far the safest option is just initialise the entire structure to 0.

Then the better choice imo is to move as much as possible of the
initialization into the initializer. That might even allow cleaning up
variables - for example value_is_ptr could clearly be replaced by
p.data_is_ptr - and it would reduce the life range of e.g. df.

Jan