[ovs-dev] [PATCH v5 ovn 0/4] Refactor I-P engine and fix use after free.

[Mark Michelson]

Hi Dumitru,

I reviewed this changeset as a whole rather than trying to review each 
individual patch.

Why does runtime_data_sb_port_binding_handler() in ovn-controller.c not 
update node state? Other change handlers update the node state based on 
whether data has changed. Is there some special reason why this 
particular change handler does not or cannot update node state? I'm 
guessing this is done on purpose, but because there is no comment it 
seems like an omission.

On 11/18/19 9:06 AM, Dumitru Ceara wrote:
> 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 |  418 ++++++++++++++++++++++++-------------------
>   lib/inc-proc-eng.c          |  314 +++++++++++++++++++++++++-------
>   lib/inc-proc-eng.h          |  104 +++++++++--
>   3 files changed, 564 insertions(+), 272 deletions(-)
> 
> ---
> 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.
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>