| Message ID | 20240510105733.36153-1-amusil@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [ovs-dev,v2] ovn-controller: Initialize bitmap to zero. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On 5/10/24 12:57, Ales Musil wrote: > The bitmap used in the update_ct_zones was uninitialized, and it could > contain any value besides 0. Use the bitmap_allocate() function instead, > to allocate the bitmap in heap rather than stack, the allocate makes sure > that the memory is properly zeroed. > This was caught by valgrind: > > Conditional jump or move depends on uninitialised value(s) > at 0x44074B: update_ct_zones (ovn-controller.c:812) > by 0x440DC9: en_ct_zones_run (ovn-controller.c:2579) > by 0x468BB7: engine_recompute (inc-proc-eng.c:415) > by 0x46954C: engine_compute (inc-proc-eng.c:454) > by 0x46954C: engine_run_node (inc-proc-eng.c:503) > by 0x46954C: engine_run (inc-proc-eng.c:528) > by 0x40AE9D: main (ovn-controller.c:5776) > Uninitialised value was created by a stack allocation > at 0x440313: update_ct_zones (ovn-controller.c:747) > > Fixes: f9cab11d5fab ("Allow explicit setting of the SNAT zone on a gateway router.") > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > v2: Use bitmap_allocate() instead of array on stack. > --- > controller/ovn-controller.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 453dc62fd..8ee2da2fd 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -732,7 +732,7 @@ update_ct_zones(const struct sset *local_lports, > const char *user; > struct sset all_users = SSET_INITIALIZER(&all_users); > struct simap req_snat_zones = SIMAP_INITIALIZER(&req_snat_zones); > - unsigned long unreq_snat_zones_map[BITMAP_N_LONGS(MAX_CT_ZONES)]; > + unsigned long *unreq_snat_zones_map = bitmap_allocate(MAX_CT_ZONES); > struct simap unreq_snat_zones = SIMAP_INITIALIZER(&unreq_snat_zones); > > const char *local_lport; > @@ -843,6 +843,7 @@ update_ct_zones(const struct sset *local_lports, > simap_destroy(&req_snat_zones); > simap_destroy(&unreq_snat_zones); > sset_destroy(&all_users); > + free(unreq_snat_zones_map); > } > > static void Thanks, Ales. This change LGTM. Though I'm a bit surprised asan didn't catch this. Is this code not covered by tests? Best regards, Ilya Maximets.
On Fri, May 10, 2024 at 1:34 PM Ilya Maximets <i.maximets@ovn.org> wrote: > On 5/10/24 12:57, Ales Musil wrote: > > The bitmap used in the update_ct_zones was uninitialized, and it could > > contain any value besides 0. Use the bitmap_allocate() function instead, > > to allocate the bitmap in heap rather than stack, the allocate makes sure > > that the memory is properly zeroed. > > This was caught by valgrind: > > > > Conditional jump or move depends on uninitialised value(s) > > at 0x44074B: update_ct_zones (ovn-controller.c:812) > > by 0x440DC9: en_ct_zones_run (ovn-controller.c:2579) > > by 0x468BB7: engine_recompute (inc-proc-eng.c:415) > > by 0x46954C: engine_compute (inc-proc-eng.c:454) > > by 0x46954C: engine_run_node (inc-proc-eng.c:503) > > by 0x46954C: engine_run (inc-proc-eng.c:528) > > by 0x40AE9D: main (ovn-controller.c:5776) > > Uninitialised value was created by a stack allocation > > at 0x440313: update_ct_zones (ovn-controller.c:747) > > > > Fixes: f9cab11d5fab ("Allow explicit setting of the SNAT zone on a > gateway router.") > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > v2: Use bitmap_allocate() instead of array on stack. > > --- > > controller/ovn-controller.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 453dc62fd..8ee2da2fd 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -732,7 +732,7 @@ update_ct_zones(const struct sset *local_lports, > > const char *user; > > struct sset all_users = SSET_INITIALIZER(&all_users); > > struct simap req_snat_zones = SIMAP_INITIALIZER(&req_snat_zones); > > - unsigned long unreq_snat_zones_map[BITMAP_N_LONGS(MAX_CT_ZONES)]; > > + unsigned long *unreq_snat_zones_map = bitmap_allocate(MAX_CT_ZONES); > > struct simap unreq_snat_zones = > SIMAP_INITIALIZER(&unreq_snat_zones); > > > > const char *local_lport; > > @@ -843,6 +843,7 @@ update_ct_zones(const struct sset *local_lports, > > simap_destroy(&req_snat_zones); > > simap_destroy(&unreq_snat_zones); > > sset_destroy(&all_users); > > + free(unreq_snat_zones_map); > > } > > > > static void > > Thanks, Ales. This change LGTM. > > Though I'm a bit surprised asan didn't catch this. Is this code not > covered by tests? > It should be covered indirectly by a lot of tests, but there are some that target this code specifically e.g. "resolve CT zone conflicts from ovsdb". > > Best regards, Ilya Maximets. > > Thanks, Ales
On 5/10/24 13:44, Ales Musil wrote: > > > On Fri, May 10, 2024 at 1:34 PM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> wrote: > > On 5/10/24 12:57, Ales Musil wrote: > > The bitmap used in the update_ct_zones was uninitialized, and it could > > contain any value besides 0. Use the bitmap_allocate() function instead, > > to allocate the bitmap in heap rather than stack, the allocate makes sure > > that the memory is properly zeroed. > > This was caught by valgrind: > > > > Conditional jump or move depends on uninitialised value(s) > > at 0x44074B: update_ct_zones (ovn-controller.c:812) > > by 0x440DC9: en_ct_zones_run (ovn-controller.c:2579) > > by 0x468BB7: engine_recompute (inc-proc-eng.c:415) > > by 0x46954C: engine_compute (inc-proc-eng.c:454) > > by 0x46954C: engine_run_node (inc-proc-eng.c:503) > > by 0x46954C: engine_run (inc-proc-eng.c:528) > > by 0x40AE9D: main (ovn-controller.c:5776) > > Uninitialised value was created by a stack allocation > > at 0x440313: update_ct_zones (ovn-controller.c:747) > > > > Fixes: f9cab11d5fab ("Allow explicit setting of the SNAT zone on a gateway router.") > > Signed-off-by: Ales Musil <amusil@redhat.com <mailto:amusil@redhat.com>> > > --- > > v2: Use bitmap_allocate() instead of array on stack. > > --- > > controller/ovn-controller.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 453dc62fd..8ee2da2fd 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -732,7 +732,7 @@ update_ct_zones(const struct sset *local_lports, > > const char *user; > > struct sset all_users = SSET_INITIALIZER(&all_users); > > struct simap req_snat_zones = SIMAP_INITIALIZER(&req_snat_zones); > > - unsigned long unreq_snat_zones_map[BITMAP_N_LONGS(MAX_CT_ZONES)]; > > + unsigned long *unreq_snat_zones_map = bitmap_allocate(MAX_CT_ZONES); > > struct simap unreq_snat_zones = SIMAP_INITIALIZER(&unreq_snat_zones); > > > > const char *local_lport; > > @@ -843,6 +843,7 @@ update_ct_zones(const struct sset *local_lports, > > simap_destroy(&req_snat_zones); > > simap_destroy(&unreq_snat_zones); > > sset_destroy(&all_users); > > + free(unreq_snat_zones_map); > > } > > > > static void > > Thanks, Ales. This change LGTM. > > Though I'm a bit surprised asan didn't catch this. Is this code not > covered by tests? > > > It should be covered indirectly by a lot of tests, but there are some that target > this code specifically e.g. "resolve CT zone conflicts from ovsdb". I'd expect asan to catch use of uninitialized stack memory. Weird. Best regards, Ilya Maximets.
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 453dc62fd..8ee2da2fd 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -732,7 +732,7 @@ update_ct_zones(const struct sset *local_lports, const char *user; struct sset all_users = SSET_INITIALIZER(&all_users); struct simap req_snat_zones = SIMAP_INITIALIZER(&req_snat_zones); - unsigned long unreq_snat_zones_map[BITMAP_N_LONGS(MAX_CT_ZONES)]; + unsigned long *unreq_snat_zones_map = bitmap_allocate(MAX_CT_ZONES); struct simap unreq_snat_zones = SIMAP_INITIALIZER(&unreq_snat_zones); const char *local_lport; @@ -843,6 +843,7 @@ update_ct_zones(const struct sset *local_lports, simap_destroy(&req_snat_zones); simap_destroy(&unreq_snat_zones); sset_destroy(&all_users); + free(unreq_snat_zones_map); } static void
The bitmap used in the update_ct_zones was uninitialized, and it could contain any value besides 0. Use the bitmap_allocate() function instead, to allocate the bitmap in heap rather than stack, the allocate makes sure that the memory is properly zeroed. This was caught by valgrind: Conditional jump or move depends on uninitialised value(s) at 0x44074B: update_ct_zones (ovn-controller.c:812) by 0x440DC9: en_ct_zones_run (ovn-controller.c:2579) by 0x468BB7: engine_recompute (inc-proc-eng.c:415) by 0x46954C: engine_compute (inc-proc-eng.c:454) by 0x46954C: engine_run_node (inc-proc-eng.c:503) by 0x46954C: engine_run (inc-proc-eng.c:528) by 0x40AE9D: main (ovn-controller.c:5776) Uninitialised value was created by a stack allocation at 0x440313: update_ct_zones (ovn-controller.c:747) Fixes: f9cab11d5fab ("Allow explicit setting of the SNAT zone on a gateway router.") Signed-off-by: Ales Musil <amusil@redhat.com> --- v2: Use bitmap_allocate() instead of array on stack. --- controller/ovn-controller.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)