> -----Original Message----- > From: dev <[email protected]> On Behalf Of Ilya Maximets > Sent: Friday 14 January 2022 11:44 > To: ovs-dev <[email protected]> > Cc: [email protected] > Subject: [ovs-dev] [BUG] dpcls_subtable_lookup_reprobe is not thread-safe. > > The function currently looks like this: > > lib/dpif-netdev.c: > > static uint32_t > dpcls_subtable_lookup_reprobe(struct dpcls *cls) > { > struct pvector *pvec = &cls->subtables; > uint32_t subtables_changed = 0; > struct dpcls_subtable *subtable = NULL; > > PVECTOR_FOR_EACH (subtable, pvec) { > uint32_t u0_bits = subtable->mf_bits_set_unit0; > uint32_t u1_bits = subtable->mf_bits_set_unit1; > void *old_func = subtable->lookup_func; > subtable->lookup_func = dpcls_subtable_get_best_impl(u0_bits, > u1_bits); > subtables_changed += (old_func != subtable->lookup_func); > } > pvector_publish(pvec); > > return subtables_changed; > } > > The problem is that it only pretends to be thread-safe: > > 1. PVECTOR_FOR_EACH() iterates over the *current* implementation of a priority > vector. Hence, the function is changing the pointers that are currently in > use by a PMD thread. A few possible consequences: > > a. Pointer store may be not atomic. In that case PMD thread may read > garbage and use it as a function pointer leading to a crash on some > platforms. > > b. If PMD thread is currently re-sorting subtables, it already cloned the > current pvector implementation and will replace it with a new version. > Therefore modifications done here will be lost. > > 2. At best, pvector_publish(pvec) is useless here, because there were no > pvector > modifications. At worst, it will publish changes that the other thread is > working on at the same time. That will likely break the vector. > > 3. Even if there were actual pvector modifications and correct use of > pvector_publish(), these calls should be done under the pmd->flow_mutex > in order to be safe. > > Best regards, Ilya Maximets.
Hi Ilya, Thanks for finding and reporting this. I'm looking into the details and your suggestions. Thanks, Cian _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
