[ovs-dev] [PATCH v6 ovn 0/4] Refactor I-P engine and fix use after free.
Dumitru Ceara
dceara at redhat.com
Fri Nov 22 16:13:06 UTC 2019
The incremental processing engine might stop a run before the
en_runtime_data node is processed. In such cases the ed_runtime_data
fields might contain pointers to already deleted SB records. For
example, if a port binding corresponding to a patch port is removed from
the SB database and the incremental processing engine aborts before the
en_runtime_data node is processed then the corresponding local_datapath
hashtable entry in ed_runtime_data is stale and will store a pointer to
the already freed sbrec_port_binding record.
This will cause invalid memory accesses in various places (e.g.,
pinctrl_run() -> prepare_ipv6_ras()).
This series fixes the issue (patch4) but to make the fix generic and
easier to debug it first refactors the incremental processing engine in
the following way:
- patch1: split engine_run() in smaller functional parts and simplify the
logic of calling engine_run and engine_need_run in the main loop.
- patch2: remove recursion from the I-P engine code. Introduce node states
to track validity of node data.
- patch3: move ct-zones to its own engine node in order to remove dependencies
on other runtime data.
CC: Han Zhou <hzhou at ovn.org>
Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine - quiet mode.")
Signed-off-by: Dumitru Ceara <dceara at redhat.com>
Dumitru Ceara (4):
ovn-controller: Refactor I-P engine_run() tracking.
ovn-controller: Add per node states to I-P engine.
ovn-controller: Add separate I-P engine node for processing ct-zones.
ovn-controller: Fix use of dangling pointers in I-P runtime_data.
controller/ovn-controller.c | 414 ++++++++++++++++++++++++-------------------
lib/inc-proc-eng.c | 338 +++++++++++++++++++++++++++--------
lib/inc-proc-eng.h | 103 ++++++++---
3 files changed, 570 insertions(+), 285 deletions(-)
---
v6:
- Address Han's comments:
- Call engine_recompute only once for a node if at least one of its input
nodes' change handler returns false.
- Simplify the incremental engine API and internally store the
topologically sorted engine nodes.
- Change 'engine_abort_recompute' from global variable to argument to be
passed to engine_run(). It's only relevant in one run context anyway
as we used to reset it before every call to engine_run().
- engine_init_run() and engine_has_run() now check all the nodes in the
engine instead of a single one.
- Change 'engine_node_valid()' to call the node's 'is_valid' method only
if the node is not in state EN_UPDATED or EN_VALID.
v5:
- Rebase.
v4:
- Address Numan's comments:
- Fix engine_need_run().
v3:
- split the change in series.
- Address Han's comments:
- fix the data encapsulation issue.
- add is_valid method to nodes.
- add internal_data/data fields to nodes as it makes it easier to write
the code instead of adding an "engine_get_data()" API.
v2: Address Han's comments:
- call engine_node_valid() in all the places where node local data is
used.
- move out "global" data outside the engine nodes. Make a clear
separation between data that can be safely used at any time and data
that can be used only when the engine run was successful.
- add a debug log for iterations when the engine didn't run.
- refactor a bit more the incremental engine code.
More information about the dev
mailing list