| Message ID | 1580130292-6879-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn] ovn-northd: Fix uninitialized 'prefix' in 'ip46_parse_cidr'. | expand |
On Mon, Jan 27, 2020 at 6:35 PM Dumitru Ceara <dceara@redhat.com> wrote: > > The partially initialized 'prefix' was later used for computing route > hashes (route_hash()) causing the ECMP routes to be incorrectly > installed. To fix the issue we now zero out the prefix before parsing > it. > > CC: Han Zhou <hzhou@ovn.org> > Fixes: 4e53974bdc4e ("ovn-northd: Support ECMP routes.") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> Thanks Dumitru for the fix. I applied this to master. Numan > --- > northd/ovn-northd.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index d094587..1e26098 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -6791,6 +6791,8 @@ add_distributed_nat_routes(struct hmap *lflows, const struct ovn_port *op) > static bool > ip46_parse_cidr(const char *str, struct v46_ip *prefix, unsigned int *plen) > { > + memset(prefix, 0, sizeof *prefix); > + > char *error = ip_parse_cidr(str, &prefix->ipv4, plen); > if (!error) { > prefix->family = AF_INET; > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 1/27/20 3:07 PM, Numan Siddique wrote: > On Mon, Jan 27, 2020 at 6:35 PM Dumitru Ceara <dceara@redhat.com> wrote: >> >> The partially initialized 'prefix' was later used for computing route >> hashes (route_hash()) causing the ECMP routes to be incorrectly >> installed. To fix the issue we now zero out the prefix before parsing >> it. >> >> CC: Han Zhou <hzhou@ovn.org> >> Fixes: 4e53974bdc4e ("ovn-northd: Support ECMP routes.") >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > Thanks Dumitru for the fix. I applied this to master. > > Numan > Thanks Numan!
On Mon, Jan 27, 2020 at 6:32 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 1/27/20 3:07 PM, Numan Siddique wrote: > > On Mon, Jan 27, 2020 at 6:35 PM Dumitru Ceara <dceara@redhat.com> wrote: > >> > >> The partially initialized 'prefix' was later used for computing route > >> hashes (route_hash()) causing the ECMP routes to be incorrectly > >> installed. To fix the issue we now zero out the prefix before parsing > >> it. > >> > >> CC: Han Zhou <hzhou@ovn.org> > >> Fixes: 4e53974bdc4e ("ovn-northd: Support ECMP routes.") > >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > > > Thanks Dumitru for the fix. I applied this to master. > > > > Numan > > > > Thanks Numan! > Thanks for fixing this and sorry for the trouble! Dumitru, I am just curious - did the test case just fail on your computer or how did you find the problem?
On Tue, Jan 28, 2020 at 7:55 AM Han Zhou <hzhou@ovn.org> wrote: > > On Mon, Jan 27, 2020 at 6:32 AM Dumitru Ceara <dceara@redhat.com> wrote: > > > > On 1/27/20 3:07 PM, Numan Siddique wrote: > > > On Mon, Jan 27, 2020 at 6:35 PM Dumitru Ceara <dceara@redhat.com> wrote: > > >> > > >> The partially initialized 'prefix' was later used for computing route > > >> hashes (route_hash()) causing the ECMP routes to be incorrectly > > >> installed. To fix the issue we now zero out the prefix before parsing > > >> it. > > >> > > >> CC: Han Zhou <hzhou@ovn.org> > > >> Fixes: 4e53974bdc4e ("ovn-northd: Support ECMP routes.") > > >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > > > > > Thanks Dumitru for the fix. I applied this to master. > > > > > > Numan > > > > > > > Thanks Numan! > > > Thanks for fixing this and sorry for the trouble! > Dumitru, I am just curious - did the test case just fail on your computer > or how did you find the problem? In my test, test cases never failed. It even passed in travis CI. Thanks Numan > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 1/28/20 7:31 AM, Numan Siddique wrote: > On Tue, Jan 28, 2020 at 7:55 AM Han Zhou <hzhou@ovn.org> wrote: >> >> On Mon, Jan 27, 2020 at 6:32 AM Dumitru Ceara <dceara@redhat.com> wrote: >>> >>> On 1/27/20 3:07 PM, Numan Siddique wrote: >>>> On Mon, Jan 27, 2020 at 6:35 PM Dumitru Ceara <dceara@redhat.com> wrote: >>>>> >>>>> The partially initialized 'prefix' was later used for computing route >>>>> hashes (route_hash()) causing the ECMP routes to be incorrectly >>>>> installed. To fix the issue we now zero out the prefix before parsing >>>>> it. >>>>> >>>>> CC: Han Zhou <hzhou@ovn.org> >>>>> Fixes: 4e53974bdc4e ("ovn-northd: Support ECMP routes.") >>>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>>> >>>> Thanks Dumitru for the fix. I applied this to master. >>>> >>>> Numan >>>> >>> >>> Thanks Numan! >>> >> Thanks for fixing this and sorry for the trouble! >> Dumitru, I am just curious - did the test case just fail on your computer >> or how did you find the problem? > > In my test, test cases never failed. It even passed in travis CI. > > Thanks > Numan > On my machine the ECMP test was failing 100% of the time due to hash mismatches. Regards, Dumitru
On Mon, Jan 27, 2020 at 11:52 PM Dumitru Ceara <dceara@redhat.com> wrote: > > On 1/28/20 7:31 AM, Numan Siddique wrote: > > On Tue, Jan 28, 2020 at 7:55 AM Han Zhou <hzhou@ovn.org> wrote: > >> > >> On Mon, Jan 27, 2020 at 6:32 AM Dumitru Ceara <dceara@redhat.com> wrote: > >>> > >>> On 1/27/20 3:07 PM, Numan Siddique wrote: > >>>> On Mon, Jan 27, 2020 at 6:35 PM Dumitru Ceara <dceara@redhat.com> wrote: > >>>>> > >>>>> The partially initialized 'prefix' was later used for computing route > >>>>> hashes (route_hash()) causing the ECMP routes to be incorrectly > >>>>> installed. To fix the issue we now zero out the prefix before parsing > >>>>> it. > >>>>> > >>>>> CC: Han Zhou <hzhou@ovn.org> > >>>>> Fixes: 4e53974bdc4e ("ovn-northd: Support ECMP routes.") > >>>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> > >>>> > >>>> Thanks Dumitru for the fix. I applied this to master. > >>>> > >>>> Numan > >>>> > >>> > >>> Thanks Numan! > >>> > >> Thanks for fixing this and sorry for the trouble! > >> Dumitru, I am just curious - did the test case just fail on your computer > >> or how did you find the problem? > > > > In my test, test cases never failed. It even passed in travis CI. > > > > Thanks > > Numan > > > > On my machine the ECMP test was failing 100% of the time due to hash > mismatches. > > Regards, > Dumitru > I see, thank you (and your machine :))!
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index d094587..1e26098 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -6791,6 +6791,8 @@ add_distributed_nat_routes(struct hmap *lflows, const struct ovn_port *op) static bool ip46_parse_cidr(const char *str, struct v46_ip *prefix, unsigned int *plen) { + memset(prefix, 0, sizeof *prefix); + char *error = ip_parse_cidr(str, &prefix->ipv4, plen); if (!error) { prefix->family = AF_INET;
The partially initialized 'prefix' was later used for computing route hashes (route_hash()) causing the ECMP routes to be incorrectly installed. To fix the issue we now zero out the prefix before parsing it. CC: Han Zhou <hzhou@ovn.org> Fixes: 4e53974bdc4e ("ovn-northd: Support ECMP routes.") Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- northd/ovn-northd.c | 2 ++ 1 file changed, 2 insertions(+)