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