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 <[email protected]>
---
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/[email protected]/
- 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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
