| Message ID | 1370346682-22970-1-git-send-email-mst@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > 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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > > > 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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? Anyway ... could you commit this tweaking the commit message in a way that you consider appropriate? Or want me to repost? > > > > 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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > > > > > > 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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > > 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> Applied, thanks. > --- > 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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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. 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(-)