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 <mh...@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 =
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
