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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jun 05, 2013 at 05:12:32PM +0300, Gleb Natapov wrote:
> On Wed, Jun 05, 2013 at 05:11:44PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 05, 2013 at 05:04:55PM +0300, Gleb Natapov wrote:
> > > On Wed, Jun 05, 2013 at 05:02:07PM +0300, Michael S. Tsirkin wrote:
> > > > 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?
> > > > 
> > > I am not at all against the patch! Just pointing out a mistake in the
> > > commit message.
> > 
> > I think we can agree both userspace not initializing them and kernel not
> > checking it are mistakes?
> > 
> Mistake that cannot be fixed at this point.
> 
> > Anyway ... could you commit this tweaking the commit
> > message in a way that you consider appropriate?
> > Or want me to repost?
> > 
> No need to report.

Thanks.

> > 
> > > > > > 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 <mst@redhat.com>
> > > > > > ---
> > > > > >  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.
> > > 
> > > --
> > > 			Gleb.
> 
> --
> 			Gleb.