| Message ID | 20231023092739.3861183-1-xsimonar@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovn-ic: fix potential segmentation violation when ts is deleted | 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 |
Thanks Xavier, Acked-by: Mark Michelson <mmichels@redhat.com> On 10/23/23 05:27, Xavier Simonart wrote: > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > --- > ic/ovn-ic.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > index e2023c2ba..eec466ec4 100644 > --- a/ic/ovn-ic.c > +++ b/ic/ovn-ic.c > @@ -1630,13 +1630,18 @@ collect_lr_routes(struct ic_context *ctx, > const struct icnbrec_transit_switch *key; > > struct hmap *routes_ad; > + const struct icnbrec_transit_switch *t_sw; > for (int i = 0; i < ic_lr->n_isb_pbs; i++) { > isb_pb = ic_lr->isb_pbs[i]; > key = icnbrec_transit_switch_index_init_row( > ctx->icnbrec_transit_switch_by_name); > icnbrec_transit_switch_index_set_name(key, isb_pb->transit_switch); > - ts_name = icnbrec_transit_switch_index_find( > - ctx->icnbrec_transit_switch_by_name, key)->name; > + t_sw = icnbrec_transit_switch_index_find( > + ctx->icnbrec_transit_switch_by_name, key); > + if (!t_sw) { > + continue; > + } > + ts_name = t_sw->name; > icnbrec_transit_switch_index_destroy_row(key); > routes_ad = shash_find_data(routes_ad_by_ts, ts_name); > if (!routes_ad) {
On Mon, Oct 23, 2023 at 11:28 AM Xavier Simonart <xsimonar@redhat.com> wrote: > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > --- > ic/ovn-ic.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > index e2023c2ba..eec466ec4 100644 > --- a/ic/ovn-ic.c > +++ b/ic/ovn-ic.c > @@ -1630,13 +1630,18 @@ collect_lr_routes(struct ic_context *ctx, > const struct icnbrec_transit_switch *key; > > struct hmap *routes_ad; > + const struct icnbrec_transit_switch *t_sw; > for (int i = 0; i < ic_lr->n_isb_pbs; i++) { > isb_pb = ic_lr->isb_pbs[i]; > key = icnbrec_transit_switch_index_init_row( > ctx->icnbrec_transit_switch_by_name); > icnbrec_transit_switch_index_set_name(key, isb_pb->transit_switch); > - ts_name = icnbrec_transit_switch_index_find( > - ctx->icnbrec_transit_switch_by_name, key)->name; > + t_sw = icnbrec_transit_switch_index_find( > + ctx->icnbrec_transit_switch_by_name, key); > + if (!t_sw) { > + continue; > + } > + ts_name = t_sw->name; > icnbrec_transit_switch_index_destroy_row(key); > routes_ad = shash_find_data(routes_ad_by_ts, ts_name); > if (!routes_ad) { > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
On Fri, Nov 3, 2023 at 5:26 AM Ales Musil <amusil@redhat.com> wrote: > > On Mon, Oct 23, 2023 at 11:28 AM Xavier Simonart <xsimonar@redhat.com> wrote: > > > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > > --- > > ic/ovn-ic.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > > index e2023c2ba..eec466ec4 100644 > > --- a/ic/ovn-ic.c > > +++ b/ic/ovn-ic.c > > @@ -1630,13 +1630,18 @@ collect_lr_routes(struct ic_context *ctx, > > const struct icnbrec_transit_switch *key; > > > > struct hmap *routes_ad; > > + const struct icnbrec_transit_switch *t_sw; > > for (int i = 0; i < ic_lr->n_isb_pbs; i++) { > > isb_pb = ic_lr->isb_pbs[i]; > > key = icnbrec_transit_switch_index_init_row( > > ctx->icnbrec_transit_switch_by_name); > > icnbrec_transit_switch_index_set_name(key, isb_pb->transit_switch); > > - ts_name = icnbrec_transit_switch_index_find( > > - ctx->icnbrec_transit_switch_by_name, key)->name; > > + t_sw = icnbrec_transit_switch_index_find( > > + ctx->icnbrec_transit_switch_by_name, key); > > + if (!t_sw) { > > + continue; > > + } > > + ts_name = t_sw->name; > > icnbrec_transit_switch_index_destroy_row(key); > > routes_ad = shash_find_data(routes_ad_by_ts, ts_name); > > if (!routes_ad) { > > -- > > 2.31.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > Looks good to me, thanks. > > Acked-by: Ales Musil <amusil@redhat.com> Thanks. Applied the patch to main and backported to branch-23.09. And then I realize that the patch is leaking the memory by not destroying the index if index_find() returns NULL. I submitted a patch to fix it - https://patchwork.ozlabs.org/project/ovn/patch/20231103155853.4137224-1-numans@ovn.org/ Request to please take a look. Numan > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA > > amusil@redhat.com > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c index e2023c2ba..eec466ec4 100644 --- a/ic/ovn-ic.c +++ b/ic/ovn-ic.c @@ -1630,13 +1630,18 @@ collect_lr_routes(struct ic_context *ctx, const struct icnbrec_transit_switch *key; struct hmap *routes_ad; + const struct icnbrec_transit_switch *t_sw; for (int i = 0; i < ic_lr->n_isb_pbs; i++) { isb_pb = ic_lr->isb_pbs[i]; key = icnbrec_transit_switch_index_init_row( ctx->icnbrec_transit_switch_by_name); icnbrec_transit_switch_index_set_name(key, isb_pb->transit_switch); - ts_name = icnbrec_transit_switch_index_find( - ctx->icnbrec_transit_switch_by_name, key)->name; + t_sw = icnbrec_transit_switch_index_find( + ctx->icnbrec_transit_switch_by_name, key); + if (!t_sw) { + continue; + } + ts_name = t_sw->name; icnbrec_transit_switch_index_destroy_row(key); routes_ad = shash_find_data(routes_ad_by_ts, ts_name); if (!routes_ad) {
Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- ic/ovn-ic.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)