Re: [Qemu-devel] [PATCH 1/2] kvm: zero-initialize KVM_SET_GSI_ROUTING input

[Michael S. Tsirkin]

On Wed, Jun 05, 2013 at 04:00:20PM +0300, Gleb Natapov wrote:
> On Tue, Jun 04, 2013 at 02:52:32PM +0300, Michael S. Tsirkin wrote:
> > kvm_add_routing_entry makes an attempt to
> > zero-initialize any new routing entry.
> > However, it fails to initialize padding
> > within the u field of the structure
> > kvm_irq_routing_entry.
> > 
> > Other functions like kvm_irqchip_update_msi_route
> > also fail to initialize the padding field in
> > kvm_irq_routing_entry.
> > 
> > While mostly harmless, this would prevent us from
> > reusing these fields for something useful in
> > the future.
> > 
> The fact that kernel does not check them for zero value is what will
> prevents us from doing so.

Well we can not change kernel now (it would break userspace)
but we can start zeroing everything in userspace.

Also, checkers like coverity might get confused by this.

Finally, simpler assignment and comparison make it worth it,
don't they?

> > It's better to just make sure all input is initialized.
> > 
> > Once it is, we can also drop complex field by field assignment and just
> > do the simple *a = *b to update a route entry.
> > 
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > ---
> >  kvm-all.c | 19 +++++++------------
> >  1 file changed, 7 insertions(+), 12 deletions(-)
> > 
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 405480e..f119ce1 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -1006,11 +1006,8 @@ static void kvm_add_routing_entry(KVMState *s,
> >      }
> >      n = s->irq_routes->nr++;
> >      new = &s->irq_routes->entries[n];
> > -    memset(new, 0, sizeof(*new));
> > -    new->gsi = entry->gsi;
> > -    new->type = entry->type;
> > -    new->flags = entry->flags;
> > -    new->u = entry->u;
> > +
> > +    *new = *entry;
> >  
> >      set_gsi(s, entry->gsi);
> >  
> > @@ -1029,9 +1026,7 @@ static int kvm_update_routing_entry(KVMState *s,
> >              continue;
> >          }
> >  
> > -        entry->type = new_entry->type;
> > -        entry->flags = new_entry->flags;
> > -        entry->u = new_entry->u;
> > +        *entry = *new_entry;
> >  
> >          kvm_irqchip_commit_routes(s);
> >  
> > @@ -1043,7 +1038,7 @@ static int kvm_update_routing_entry(KVMState *s,
> >  
> >  void kvm_irqchip_add_irq_route(KVMState *s, int irq, int irqchip, int pin)
> >  {
> > -    struct kvm_irq_routing_entry e;
> > +    struct kvm_irq_routing_entry e = {};
> >  
> >      assert(pin < s->gsi_count);
> >  
> > @@ -1156,7 +1151,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
> >              return virq;
> >          }
> >  
> > -        route = g_malloc(sizeof(KVMMSIRoute));
> > +        route = g_malloc0(sizeof(KVMMSIRoute));
> >          route->kroute.gsi = virq;
> >          route->kroute.type = KVM_IRQ_ROUTING_MSI;
> >          route->kroute.flags = 0;
> > @@ -1177,7 +1172,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
> >  
> >  int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
> >  {
> > -    struct kvm_irq_routing_entry kroute;
> > +    struct kvm_irq_routing_entry kroute = {};
> >      int virq;
> >  
> >      if (!kvm_gsi_routing_enabled()) {
> > @@ -1203,7 +1198,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage 
> > msg)
> >  
> >  int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg)
> >  {
> > -    struct kvm_irq_routing_entry kroute;
> > +    struct kvm_irq_routing_entry kroute = {};
> >  
> >      if (!kvm_irqchip_in_kernel()) {
> >          return -ENOSYS;
> > -- 
> > MST
> 
> --
>                       Gleb.