On Fri, May 10, 2024 at 11:23 AM Ilya Maximets <[email protected]> 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 <[email protected]>
> > ---
> > 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
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
[email protected]
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
