| Message ID | 20220314110928.471986-1-mheib@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] controller/pinctrl: avoid accessing invalid memory | 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 Mon, Mar 14, 2022 at 7:10 AM Mohammad Heib <mheib@redhat.com> wrote: > > currently pinctrl main thread uses some shash lists > that were supplied by ovn-controller main thread to prepare > and send IPv6 RAs, those lists are not updated properly when > LRP is deleted and can cause some invalid memory access > in the pinctrl module. > > This patch handles such changes and update those lists > to avoid invalid memory access. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2052945 > Signed-off-by: Mohammad Heib <mheib@redhat.com> > --- > controller/binding.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 4d62b0858..ebbaae9ac 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -489,12 +489,14 @@ update_active_pb_ras_pd(const struct sbrec_port_binding *pb, > bool ras_pd_conf = smap_get_bool(&pb->options, conf, false); > struct shash_node *iter = shash_find(map, pb->logical_port); > struct pb_ld_binding *ras_pd = iter ? iter->data : NULL; > + bool deleted = sbrec_port_binding_is_deleted(pb); > > - if (iter && !ras_pd_conf) { > + if (iter && (!ras_pd_conf || deleted)) { > shash_delete(map, iter); > free(ras_pd); > return; > } > + > if (ras_pd_conf) { > if (!ras_pd) { > ras_pd = xzalloc(sizeof *ras_pd); > @@ -2338,11 +2340,9 @@ delete_done: > > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, > b_ctx_in->port_binding_table) { > - /* Loop to handle create and update changes only. */ > - if (sbrec_port_binding_is_deleted(pb)) { > - continue; > - } > - > + /* handle port changes/deletion and update the local active ports ipv6 > + * releated lists. > + */ Thanks Mohammad for fixing this issue. The loop in which you've modified the code handles the port binding create/updates. I think its a bit odd to handle port binding deletions here. And we have a loop just above to handle the port binding deletions. I'd suggest to delete the port binding related entry from the shash's - b_ctx_out->local_active_ports_ipv6_pd and b_ctx_out->local_active_ports_ras in the first 'SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED()' loop. You could probably add a new function delete_active_pb_ras_pd() (or with a better name) to delete the entry from these shash. > update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, > b_ctx_out->local_active_ports_ipv6_pd, > "ipv6_prefix_delegation"); > @@ -2351,6 +2351,11 @@ delete_done: > b_ctx_out->local_active_ports_ras, > "ipv6_ra_send_periodic"); > > + /* Loop to handle create and update changes only. */ > + if (sbrec_port_binding_is_deleted(pb)) { > + continue; > + } > + > enum en_lport_type lport_type = get_lport_type(pb); > > struct binding_lport *b_lport = > -- > 2.34.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Hi Mohammad, great job finding this problem! I have a couple of suggestions below. On 3/14/22 07:09, Mohammad Heib wrote: > currently pinctrl main thread uses some shash lists > that were supplied by ovn-controller main thread to prepare > and send IPv6 RAs, those lists are not updated properly when I would change the wording here to not mention threads at all. In this case, all of the cited code runs in the same thread, so mentioning threads makes it sound like there are two threads competing. If that were the case, then synchronization would need to be added between the competing threads. The actual problem is that in the incremental case, it's possible for deleted SB port_bindings to remain in a hash table where they should be deleted. That's what you are addressing with your changes below. > LRP is deleted and can cause some invalid memory access > in the pinctrl module. > > This patch handles such changes and update those lists > to avoid invalid memory access. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2052945 > Signed-off-by: Mohammad Heib <mheib@redhat.com> > --- > controller/binding.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 4d62b0858..ebbaae9ac 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -489,12 +489,14 @@ update_active_pb_ras_pd(const struct sbrec_port_binding *pb, > bool ras_pd_conf = smap_get_bool(&pb->options, conf, false); > struct shash_node *iter = shash_find(map, pb->logical_port); > struct pb_ld_binding *ras_pd = iter ? iter->data : NULL; > + bool deleted = sbrec_port_binding_is_deleted(pb); update_active_pb_ras_pd() is called both in the I-P recompute function (binding_run()) and the I-P change handler function (binding_handle_port_binding_changes()). In the context of binding_run(), the pb parameter comes from a loop of SBREC_PORT_BINDING_FOR_EACH(). It may be benign to call sbrec_port_binding_is_deleted() in this situation, but I don't know if there are potential downsides to calling this outside a *_FOR_EACH_TRACKED() loop (maybe someone more knowledgeable about IDL could chime in). Also SBREC_PORT_BINDING_FOR_EACH() will never return a deleted port_binding, so even if it is safe to call sbrec_port_binding_is_deleted() here, there's no benefit to doing so. I think what may work better here is to add a new parameter to update_active_pb_ras_pd(). This would be a boolean that indicates if the pb parameter represents a deleted port binding. This way, from binding_run(), this parameter could always be set "false" and from binding_handle_port_binding_changes(), this parameter could be set to the result of sbrec_port_binding_is_deleted(). > > - if (iter && !ras_pd_conf) { > + if (iter && (!ras_pd_conf || deleted)) { > shash_delete(map, iter); > free(ras_pd); > return; > } > + > if (ras_pd_conf) { > if (!ras_pd) { > ras_pd = xzalloc(sizeof *ras_pd); > @@ -2338,11 +2340,9 @@ delete_done: > > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, > b_ctx_in->port_binding_table) { > - /* Loop to handle create and update changes only. */ > - if (sbrec_port_binding_is_deleted(pb)) { > - continue; > - } > - > + /* handle port changes/deletion and update the local active ports ipv6 > + * releated lists. > + */ > update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, > b_ctx_out->local_active_ports_ipv6_pd, > "ipv6_prefix_delegation"); > @@ -2351,6 +2351,11 @@ delete_done: > b_ctx_out->local_active_ports_ras, > "ipv6_ra_send_periodic"); > > + /* Loop to handle create and update changes only. */ > + if (sbrec_port_binding_is_deleted(pb)) { > + continue; > + } > + > enum en_lport_type lport_type = get_lport_type(pb); > > struct binding_lport *b_lport = >
Hi Numan, On 3/21/22 20:00, Numan Siddique wrote: > On Mon, Mar 14, 2022 at 7:10 AM Mohammad Heib <mheib@redhat.com> wrote: >> currently pinctrl main thread uses some shash lists >> that were supplied by ovn-controller main thread to prepare >> and send IPv6 RAs, those lists are not updated properly when >> LRP is deleted and can cause some invalid memory access >> in the pinctrl module. >> >> This patch handles such changes and update those lists >> to avoid invalid memory access. >> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2052945 >> Signed-off-by: Mohammad Heib <mheib@redhat.com> > >> --- >> controller/binding.c | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/controller/binding.c b/controller/binding.c >> index 4d62b0858..ebbaae9ac 100644 >> --- a/controller/binding.c >> +++ b/controller/binding.c >> @@ -489,12 +489,14 @@ update_active_pb_ras_pd(const struct sbrec_port_binding *pb, >> bool ras_pd_conf = smap_get_bool(&pb->options, conf, false); >> struct shash_node *iter = shash_find(map, pb->logical_port); >> struct pb_ld_binding *ras_pd = iter ? iter->data : NULL; >> + bool deleted = sbrec_port_binding_is_deleted(pb); >> >> - if (iter && !ras_pd_conf) { >> + if (iter && (!ras_pd_conf || deleted)) { >> shash_delete(map, iter); >> free(ras_pd); >> return; >> } >> + >> if (ras_pd_conf) { >> if (!ras_pd) { >> ras_pd = xzalloc(sizeof *ras_pd); >> @@ -2338,11 +2340,9 @@ delete_done: >> >> SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, >> b_ctx_in->port_binding_table) { >> - /* Loop to handle create and update changes only. */ >> - if (sbrec_port_binding_is_deleted(pb)) { >> - continue; >> - } >> - >> + /* handle port changes/deletion and update the local active ports ipv6 >> + * releated lists. >> + */ > Thanks Mohammad for fixing this issue. > > The loop in which you've modified the code handles the port binding > create/updates. > I think its a bit odd to handle port binding deletions here. > And we have a loop just above to handle the port binding deletions. > > I'd suggest to delete the port binding related entry from the shash's > - b_ctx_out->local_active_ports_ipv6_pd > and b_ctx_out->local_active_ports_ras in the first > 'SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED()' loop. > > You could probably add a new function delete_active_pb_ras_pd() (or > with a better name) to delete > the entry from these shash. thank you for your review:) i submitted v2 with your suggested changes. https://patchwork.ozlabs.org/project/ovn/patch/20220403112637.3207942-1-mheib@redhat.com/ thanks, > > >> update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, >> b_ctx_out->local_active_ports_ipv6_pd, >> "ipv6_prefix_delegation"); >> @@ -2351,6 +2351,11 @@ delete_done: >> b_ctx_out->local_active_ports_ras, >> "ipv6_ra_send_periodic"); >> >> + /* Loop to handle create and update changes only. */ >> + if (sbrec_port_binding_is_deleted(pb)) { >> + continue; >> + } >> + >> enum en_lport_type lport_type = get_lport_type(pb); >> >> struct binding_lport *b_lport = >> -- >> 2.34.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
Hi Mark, thank you for reviewing my patch:) On 3/21/22 20:30, Mark Michelson wrote: > Hi Mohammad, great job finding this problem! I have a couple of > suggestions below. > > On 3/14/22 07:09, Mohammad Heib wrote: >> currently pinctrl main thread uses some shash lists >> that were supplied by ovn-controller main thread to prepare >> and send IPv6 RAs, those lists are not updated properly when > > I would change the wording here to not mention threads at all. In this > case, all of the cited code runs in the same thread, so mentioning > threads makes it sound like there are two threads competing. If that > were the case, then synchronization would need to be added between the > competing threads. > > The actual problem is that in the incremental case, it's possible for > deleted SB port_bindings to remain in a hash table where they should > be deleted. That's what you are addressing with your changes below. yes you are right it's a bit confusing when mentioning thread and yes it's not a threads issues, i submitted v2 with an update commit message with your suggested changes. > >> LRP is deleted and can cause some invalid memory access >> in the pinctrl module. >> >> This patch handles such changes and update those lists >> to avoid invalid memory access. >> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2052945 >> Signed-off-by: Mohammad Heib <mheib@redhat.com> >> --- >> controller/binding.c | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/controller/binding.c b/controller/binding.c >> index 4d62b0858..ebbaae9ac 100644 >> --- a/controller/binding.c >> +++ b/controller/binding.c >> @@ -489,12 +489,14 @@ update_active_pb_ras_pd(const struct >> sbrec_port_binding *pb, >> bool ras_pd_conf = smap_get_bool(&pb->options, conf, false); >> struct shash_node *iter = shash_find(map, pb->logical_port); >> struct pb_ld_binding *ras_pd = iter ? iter->data : NULL; >> + bool deleted = sbrec_port_binding_is_deleted(pb); > > update_active_pb_ras_pd() is called both in the I-P recompute function > (binding_run()) and the I-P change handler function > (binding_handle_port_binding_changes()). In the context of > binding_run(), the pb parameter comes from a loop of > SBREC_PORT_BINDING_FOR_EACH(). It may be benign to call > sbrec_port_binding_is_deleted() in this situation, but I don't know if > there are potential downsides to calling this outside a > *_FOR_EACH_TRACKED() loop (maybe someone more knowledgeable about IDL > could chime in). Also SBREC_PORT_BINDING_FOR_EACH() will never return > a deleted port_binding, so even if it is safe to call > sbrec_port_binding_is_deleted() here, there's no benefit to doing so. > > I think what may work better here is to add a new parameter to > update_active_pb_ras_pd(). This would be a boolean that indicates if > the pb parameter represents a deleted port binding. This way, from > binding_run(), this parameter could always be set "false" and from > binding_handle_port_binding_changes(), this parameter could be set to > the result of sbrec_port_binding_is_deleted(). in v2 i added a new function delete_active_pb_ras_pd which will remove the pb record from the needed hash tables. this function will only be called in the incremental case only. https://patchwork.ozlabs.org/project/ovn/patch/20220403112637.3207942-1-mheib@redhat.com/ >> - if (iter && !ras_pd_conf) { >> + if (iter && (!ras_pd_conf || deleted)) { >> shash_delete(map, iter); >> free(ras_pd); >> return; >> } >> + >> if (ras_pd_conf) { >> if (!ras_pd) { >> ras_pd = xzalloc(sizeof *ras_pd); >> @@ -2338,11 +2340,9 @@ delete_done: >> SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, >> b_ctx_in->port_binding_table) { >> - /* Loop to handle create and update changes only. */ >> - if (sbrec_port_binding_is_deleted(pb)) { >> - continue; >> - } >> - >> + /* handle port changes/deletion and update the local active >> ports ipv6 >> + * releated lists. >> + */ >> update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, >> b_ctx_out->local_active_ports_ipv6_pd, >> "ipv6_prefix_delegation"); >> @@ -2351,6 +2351,11 @@ delete_done: >> b_ctx_out->local_active_ports_ras, >> "ipv6_ra_send_periodic"); >> + /* Loop to handle create and update changes only. */ >> + if (sbrec_port_binding_is_deleted(pb)) { >> + continue; >> + } >> + >> enum en_lport_type lport_type = get_lport_type(pb); >> struct binding_lport *b_lport = >> > Thanks.
diff --git a/controller/binding.c b/controller/binding.c index 4d62b0858..ebbaae9ac 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -489,12 +489,14 @@ update_active_pb_ras_pd(const struct sbrec_port_binding *pb, bool ras_pd_conf = smap_get_bool(&pb->options, conf, false); struct shash_node *iter = shash_find(map, pb->logical_port); struct pb_ld_binding *ras_pd = iter ? iter->data : NULL; + bool deleted = sbrec_port_binding_is_deleted(pb); - if (iter && !ras_pd_conf) { + if (iter && (!ras_pd_conf || deleted)) { shash_delete(map, iter); free(ras_pd); return; } + if (ras_pd_conf) { if (!ras_pd) { ras_pd = xzalloc(sizeof *ras_pd); @@ -2338,11 +2340,9 @@ delete_done: SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, b_ctx_in->port_binding_table) { - /* Loop to handle create and update changes only. */ - if (sbrec_port_binding_is_deleted(pb)) { - continue; - } - + /* handle port changes/deletion and update the local active ports ipv6 + * releated lists. + */ update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, b_ctx_out->local_active_ports_ipv6_pd, "ipv6_prefix_delegation"); @@ -2351,6 +2351,11 @@ delete_done: b_ctx_out->local_active_ports_ras, "ipv6_ra_send_periodic"); + /* Loop to handle create and update changes only. */ + if (sbrec_port_binding_is_deleted(pb)) { + continue; + } + enum en_lport_type lport_type = get_lport_type(pb); struct binding_lport *b_lport =
currently pinctrl main thread uses some shash lists that were supplied by ovn-controller main thread to prepare and send IPv6 RAs, those lists are not updated properly when LRP is deleted and can cause some invalid memory access in the pinctrl module. This patch handles such changes and update those lists to avoid invalid memory access. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2052945 Signed-off-by: Mohammad Heib <mheib@redhat.com> --- controller/binding.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)