Message ID | 20240510105733.36153-1-amusil@redhat.com |
---|---|
State | Accepted |
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.
Like Ilya, I'm surprised that ASAN did not catch this. Having said that, Acked-by: Mark Michelson <mmichels@redhat.com> On 5/10/24 06: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
On 6/3/24 22:56, Mark Michelson wrote: > Like Ilya, I'm surprised that ASAN did not catch this. > > Having said that, > Acked-by: Mark Michelson <mmichels@redhat.com> > Thanks, Ales, Ilya, Mark! I applied this to main with a small change, see below. I also backported it to all supported branches. > On 5/10/24 06: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); bitmap_free(unreq_snat_zones_map); >> } >> static void > Regards, Dumitru
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(-)