On 7/1/25 5:06 PM, Ilya Maximets wrote: > On 7/1/25 1:41 PM, Roi Dayan via dev wrote: >> From: Eli Britstein <el...@nvidia.com> >> >> A group has a hash_map that contains the array of its buckets. >> Revalidator threads access it to check if some bucket is alive, while >> the main thread may modify the group, re-allocating this array. This race >> condition may lead to invalid memory access. > > Hi, Eli and Roi. > > Groups are actually never modified, they are re-created. AFAIR, the > group_modify() callback is only used from the ofproto_group_mod_finish() > for the newly created group that cannot be accessed by any other > threads as it's not published yet. > > Could you explain in more detail the code path that leads to invalid > memory access in you case?
Hmm, after a closer look we do indeed publish the groups before the mod_finish. That means my assumption is wrong and we can have other threads accessing this group, in theory. Instead of introducing a per-packet cost of locking/unlocking the group, I'd suggest we move the group_modify() call from the mod_finish to mod_start, since this is where we're actually adding/removing the buckets and so that's the place where we need to notify the underlying implementation about the change. The abstraction is not ideal anyway, we expect both groups to be present at the ofproto-dpif level after the new one is constructed, so we may as well call the modification callback there as well. The group will not be accessible until the table version is bumped prior to the mod_finish. Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
