| Message ID | 20240510084430.21717-1-amusil@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] 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 | success | github build: passed |
On 5/10/24 10:44, Ales Musil wrote: > The bitmap used in the update_ct_zones was uninitialized and it could > contain any value besides 0, make sure we initialize it to 0 instead. > 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> > --- > controller/ovn-controller.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 453dc62fd..2388a1c15 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_N_LONGS(MAX_CT_ZONES)] = {0}; > struct simap unreq_snat_zones = SIMAP_INITIALIZER(&unreq_snat_zones); > > const char *local_lport; Hi, Ales. Thanks for the fix! The issue is caused by not using a proper bitmap API. Can we just use the bitmap_allocate() here instead? With the amount of dynamic memory allocations this function does with all the hash maps adding one more allocation will not make any difference, but may protect from potential issues of not using the API / providing a bad example. Allocating 8KB on stack is not a particularly good thing anyway. What do you think? Best regards, Ilya Maximets.
On Fri, May 10, 2024 at 11:23 AM Ilya Maximets <i.maximets@ovn.org> wrote: > On 5/10/24 10:44, Ales Musil wrote: > > The bitmap used in the update_ct_zones was uninitialized and it could > > contain any value besides 0, make sure we initialize it to 0 instead. > > 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> > > --- > > controller/ovn-controller.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 453dc62fd..2388a1c15 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_N_LONGS(MAX_CT_ZONES)] = > {0}; > > struct simap unreq_snat_zones = > SIMAP_INITIALIZER(&unreq_snat_zones); > > > > const char *local_lport; > > Hi, Ales. Thanks for the fix! > > The issue is caused by not using a proper bitmap API. Can we just use > the bitmap_allocate() here instead? With the amount of dynamic memory > allocations this function does with all the hash maps adding one more > allocation will not make any difference, but may protect from potential > issues of not using the API / providing a bad example. Allocating 8KB > on stack is not a particularly good thing anyway. > > What do you think? > > Best regards, Ilya Maximets. > > Hi Ilya, it is reasonable and I don't have a hard preference. I'll send v2 with bitmap_allocate() instead. Thanks, Ales
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 453dc62fd..2388a1c15 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_N_LONGS(MAX_CT_ZONES)] = {0}; struct simap unreq_snat_zones = SIMAP_INITIALIZER(&unreq_snat_zones); const char *local_lport;
The bitmap used in the update_ct_zones was uninitialized and it could contain any value besides 0, make sure we initialize it to 0 instead. 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> --- controller/ovn-controller.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)