| Message ID | 20240819161848.1416381-1-numans@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] northd: Fix potential crash when creating chassisredirect port. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On Mon, Aug 19, 2024 at 9:18 AM <numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org> > > Commit 8d13579bf5b3 creates a chassisredirect port for a logical switch > port (of type 'router') in certain scenarios and 'op->nbsp' can be NULL. > The following crash is reported by Sanitizer. > > ==84927==ERROR: AddressSanitizer: SEGV on unknown address > ==84927==The signal is caused by a READ memory access. > ==84927==Hint: address points to the zero page. > #0 0x57ab3854f04a in hmap_first_with_hash ovn/ovs/./include/openvswitch/hmap.h:360:38 > #1 0x57ab3854fedf in smap_find__ ovn/ovs/lib/smap.c:421:5 > #2 0x57ab3854f7b8 in smap_get_node ovn/ovs/lib/smap.c:217:12 > #3 0x57ab3854f74f in smap_get_def ovn/ovs/lib/smap.c:208:30 > #4 0x57ab3854f726 in smap_get ovn/ovs/lib/smap.c:200:12 > #5 0x57ab3854f862 in smap_get_int ovn/ovs/lib/smap.c:240:25 > #6 0x57ab383222eb in ovn_port_assign_requested_tnl_id ovn/northd/northd.c:4372:27 > #7 0x57ab383072fa in build_ports ovn/northd/northd.c:4454:9 > #8 0x57ab38301457 in ovnnb_db_run ovn/northd/northd.c:18023:5 > #9 0x57ab3841d99e in en_northd_run ovn/northd/en-northd.c:137:5 > #10 0x57ab384599b2 in engine_recompute ovn/lib/inc-proc-eng.c:411:5 > #11 0x57ab38459d6e in engine_run_node ovn/lib/inc-proc-eng.c:473:9 > #12 0x57ab38459ec3 in engine_run ovn/lib/inc-proc-eng.c:524:9 > #13 0x57ab38430c5d in inc_proc_northd_run ovn/northd/inc-proc-northd.c:420:5 > #14 0x57ab3841bb2f in main ovn/northd/ovn-northd.c:970:32 > > This patch fixes this issue. It also corrects some typo introduced by > the commit - changes the reference from "chassisresident" to > "chassisredirect". > > Fixes: 8d13579bf5b3 ("Add support for centralize routing for distributed gw ports.") > Reported-by: Felix Huettner <felix.huettner@mail.schwarz> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2024-August/416264.html > CC: Han Zhou <hzhou@ovn.org> > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > northd/northd.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 7ceed63ddb..73367b9104 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -1126,7 +1126,7 @@ is_l3dgw_port(const struct ovn_port *op) > /* This function returns true if 'op' is a chassis resident > * derived port. False otherwise. > * There are 2 ways to check if 'op' is chassis resident port. > - * 1. op->sb->type is "chassisresident" > + * 1. op->sb->type is "chassisredirect" > * 2. op->primary_port is not NULL. If op->primary_port is set, > * it means 'op' is derived from the ovn_port op->primary_port. > * > @@ -2136,7 +2136,7 @@ create_cr_port(struct ovn_port *op, struct hmap *ports, > > struct ovn_port *crp = ovn_port_find(ports, redirect_name); > if (crp && crp->sb && crp->sb->datapath == op->od->sb) { > - ovn_port_set_nb(crp, NULL, op->nbrp); > + ovn_port_set_nb(crp, op->nbsp, op->nbrp); > ovs_list_remove(&crp->list); > ovs_list_push_back(both_dbs, &crp->list); > } else { > @@ -2466,7 +2466,7 @@ join_logical_ports(const struct sbrec_port_binding_table *sbrec_pb_table, > } > > > - /* Create chassisresident port for the distributed gateway port's (DGP) > + /* Create chassisredirect port for the distributed gateway port's (DGP) > * peer if > * - DGP's router has only one DGP and > * - Its peer is a logical switch port and > @@ -12622,7 +12622,7 @@ build_lrouter_port_nat_arp_nd_flow(struct ovn_port *op, > > if (op->peer && op->peer->cr_port) { > /* We don't add the below flows if the router port's peer has > - * a chassisresident port. That's because routing is centralized on > + * a chassisredirect port. That's because routing is centralized on > * the gateway chassis for the router port networks/subnets. > */ > return; > @@ -16349,7 +16349,7 @@ lrouter_check_nat_entry(const struct ovn_datapath *od, > *distributed = false; > > /* NAT cannnot be distributed if the DGP's peer > - * has a chassisresident port (as the routing is centralized > + * has a chassisredirect port (as the routing is centralized > * on the gateway chassis for the DGP's networks/subnets.) > */ > struct ovn_port *l3dgw_port = *nat_l3dgw_port; > -- > 2.45.2 > Thanks Numan. Acked-by: Han Zhou <hzhou@ovn.org>
On Mon, Aug 19, 2024 at 7:11 PM Han Zhou <hzhou@ovn.org> wrote: > > On Mon, Aug 19, 2024 at 9:18 AM <numans@ovn.org> wrote: > > > > From: Numan Siddique <numans@ovn.org> > > > > Commit 8d13579bf5b3 creates a chassisredirect port for a logical switch > > port (of type 'router') in certain scenarios and 'op->nbsp' can be NULL. > > The following crash is reported by Sanitizer. > > > > ==84927==ERROR: AddressSanitizer: SEGV on unknown address > > ==84927==The signal is caused by a READ memory access. > > ==84927==Hint: address points to the zero page. > > #0 0x57ab3854f04a in hmap_first_with_hash ovn/ovs/./include/openvswitch/hmap.h:360:38 > > #1 0x57ab3854fedf in smap_find__ ovn/ovs/lib/smap.c:421:5 > > #2 0x57ab3854f7b8 in smap_get_node ovn/ovs/lib/smap.c:217:12 > > #3 0x57ab3854f74f in smap_get_def ovn/ovs/lib/smap.c:208:30 > > #4 0x57ab3854f726 in smap_get ovn/ovs/lib/smap.c:200:12 > > #5 0x57ab3854f862 in smap_get_int ovn/ovs/lib/smap.c:240:25 > > #6 0x57ab383222eb in ovn_port_assign_requested_tnl_id ovn/northd/northd.c:4372:27 > > #7 0x57ab383072fa in build_ports ovn/northd/northd.c:4454:9 > > #8 0x57ab38301457 in ovnnb_db_run ovn/northd/northd.c:18023:5 > > #9 0x57ab3841d99e in en_northd_run ovn/northd/en-northd.c:137:5 > > #10 0x57ab384599b2 in engine_recompute ovn/lib/inc-proc-eng.c:411:5 > > #11 0x57ab38459d6e in engine_run_node ovn/lib/inc-proc-eng.c:473:9 > > #12 0x57ab38459ec3 in engine_run ovn/lib/inc-proc-eng.c:524:9 > > #13 0x57ab38430c5d in inc_proc_northd_run ovn/northd/inc-proc-northd.c:420:5 > > #14 0x57ab3841bb2f in main ovn/northd/ovn-northd.c:970:32 > > > > This patch fixes this issue. It also corrects some typo introduced by > > the commit - changes the reference from "chassisresident" to > > "chassisredirect". > > > > Fixes: 8d13579bf5b3 ("Add support for centralize routing for distributed gw ports.") > > Reported-by: Felix Huettner <felix.huettner@mail.schwarz> > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2024-August/416264.html > > CC: Han Zhou <hzhou@ovn.org> > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > northd/northd.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 7ceed63ddb..73367b9104 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -1126,7 +1126,7 @@ is_l3dgw_port(const struct ovn_port *op) > > /* This function returns true if 'op' is a chassis resident > > * derived port. False otherwise. > > * There are 2 ways to check if 'op' is chassis resident port. > > - * 1. op->sb->type is "chassisresident" > > + * 1. op->sb->type is "chassisredirect" > > * 2. op->primary_port is not NULL. If op->primary_port is set, > > * it means 'op' is derived from the ovn_port op->primary_port. > > * > > @@ -2136,7 +2136,7 @@ create_cr_port(struct ovn_port *op, struct hmap *ports, > > > > struct ovn_port *crp = ovn_port_find(ports, redirect_name); > > if (crp && crp->sb && crp->sb->datapath == op->od->sb) { > > - ovn_port_set_nb(crp, NULL, op->nbrp); > > + ovn_port_set_nb(crp, op->nbsp, op->nbrp); > > ovs_list_remove(&crp->list); > > ovs_list_push_back(both_dbs, &crp->list); > > } else { > > @@ -2466,7 +2466,7 @@ join_logical_ports(const struct sbrec_port_binding_table *sbrec_pb_table, > > } > > > > > > - /* Create chassisresident port for the distributed gateway port's (DGP) > > + /* Create chassisredirect port for the distributed gateway port's (DGP) > > * peer if > > * - DGP's router has only one DGP and > > * - Its peer is a logical switch port and > > @@ -12622,7 +12622,7 @@ build_lrouter_port_nat_arp_nd_flow(struct ovn_port *op, > > > > if (op->peer && op->peer->cr_port) { > > /* We don't add the below flows if the router port's peer has > > - * a chassisresident port. That's because routing is centralized on > > + * a chassisredirect port. That's because routing is centralized on > > * the gateway chassis for the router port networks/subnets. > > */ > > return; > > @@ -16349,7 +16349,7 @@ lrouter_check_nat_entry(const struct ovn_datapath *od, > > *distributed = false; > > > > /* NAT cannnot be distributed if the DGP's peer > > - * has a chassisresident port (as the routing is centralized > > + * has a chassisredirect port (as the routing is centralized > > * on the gateway chassis for the DGP's networks/subnets.) > > */ > > struct ovn_port *l3dgw_port = *nat_l3dgw_port; > > -- > > 2.45.2 > > > > Thanks Numan. > Acked-by: Han Zhou <hzhou@ovn.org> Thanks. Applied to main and branch-24.09. Numan > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/northd/northd.c b/northd/northd.c index 7ceed63ddb..73367b9104 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -1126,7 +1126,7 @@ is_l3dgw_port(const struct ovn_port *op) /* This function returns true if 'op' is a chassis resident * derived port. False otherwise. * There are 2 ways to check if 'op' is chassis resident port. - * 1. op->sb->type is "chassisresident" + * 1. op->sb->type is "chassisredirect" * 2. op->primary_port is not NULL. If op->primary_port is set, * it means 'op' is derived from the ovn_port op->primary_port. * @@ -2136,7 +2136,7 @@ create_cr_port(struct ovn_port *op, struct hmap *ports, struct ovn_port *crp = ovn_port_find(ports, redirect_name); if (crp && crp->sb && crp->sb->datapath == op->od->sb) { - ovn_port_set_nb(crp, NULL, op->nbrp); + ovn_port_set_nb(crp, op->nbsp, op->nbrp); ovs_list_remove(&crp->list); ovs_list_push_back(both_dbs, &crp->list); } else { @@ -2466,7 +2466,7 @@ join_logical_ports(const struct sbrec_port_binding_table *sbrec_pb_table, } - /* Create chassisresident port for the distributed gateway port's (DGP) + /* Create chassisredirect port for the distributed gateway port's (DGP) * peer if * - DGP's router has only one DGP and * - Its peer is a logical switch port and @@ -12622,7 +12622,7 @@ build_lrouter_port_nat_arp_nd_flow(struct ovn_port *op, if (op->peer && op->peer->cr_port) { /* We don't add the below flows if the router port's peer has - * a chassisresident port. That's because routing is centralized on + * a chassisredirect port. That's because routing is centralized on * the gateway chassis for the router port networks/subnets. */ return; @@ -16349,7 +16349,7 @@ lrouter_check_nat_entry(const struct ovn_datapath *od, *distributed = false; /* NAT cannnot be distributed if the DGP's peer - * has a chassisresident port (as the routing is centralized + * has a chassisredirect port (as the routing is centralized * on the gateway chassis for the DGP's networks/subnets.) */ struct ovn_port *l3dgw_port = *nat_l3dgw_port;