[ovs-dev] ovn-northd: ovsdb idl question

[Lance Richardson]

> From: "Lance Richardson" <lrichard at redhat.com>
> To: "ovs dev" <dev at openvswitch.org>
> Sent: Tuesday, December 20, 2016 1:59:02 PM
> Subject: [ovs-dev] ovn-northd: ovsdb idl question
> 
> I've been investigating a crash in ovn-northd. This is very easy to
> reproduce by inserting a row into the NB Logical_Switch_Port table
> without setting the "name" column, e.g.:
> 
> ovn-nbctl ls-add ls0
> ovn-nbctl --id=@foo create logical_switch_port type='localnet' -- add
> logical_switch ls0 ports @foo
> 
> This leads to a null pointer dereference when sb->logical_port is
> dereferenced in update_logical_port_status().
> 
> The reason sb->logical_port is NULL is somewhat interesting.
> In build_ports(), the sb port binding record is created via:
>     
>         op->sb = sbrec_port_binding_insert(ctx->ovnsb_txn);
>         sbrec_port_binding_set_logical_port(op->sb, op->key);
> 
> After which op->sb->logical_port is NULL (since op->key
> is an empty string (""), not NULL, I would have expected
> op->sb->logical_port to also be non-NULL, however ovsdb_idl_txn_write__()
> (called by sbrec_port_binding_set_logical_port()) returns
> without having modified op->sb->logical port because of this optimization:
> 
>     if (write_only && ovsdb_datum_equals(ovsdb_idl_read(row, column),
>                                          datum, &column->type)) {
>         goto discard_datum;
>     }
> 
> In this case, write_only is true (mode for the column is OVSDB_IDL_MONITOR),
> and ovsdb_datum_equals() is true because datum->key is "", which
> is compared against "" because the column is uninitialized
> (ovsdb_atom_default()
> is "" for string type), so ovsdb_idl_txn_write__() returns without
> having updated the local_port field, and it remains set to NULL.
> 
> 
> Hopefully the above makes sense... now for the question I'm struggling
> with: what is the best way to address the ovn-northd crash in this
> scenario.  Some options seem to be:
> 
>    1) Simply document that NB Logical_Switch_Port records have
>       to be created with the "name" field initialized. This seems
>       like a poor option unless there's some way to enforce this
>       requirement.
>    2) Sprinkle checks for sb->local_port == NULL where necessary
>       to avoid the crash.  This also seems like a non-ideal
>       solution, particularly since there may be other tables/columns
>       where this issue can be hit.
>    3) Have ovsdb_idl_txn_write__() distinguish between NULL and empty
>       string where ovsdb_datum_equal() is used.
>    4) Maybe there's another option? (Quite possible, I'm far from
>       being able to claim a good understanding of the IDL).
> 
> Any advice appreciated!
> 
> Thanks,
> 
>     Lance


Thinking about (3) a little more, how about this:

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 8ce228d..784e5cc 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -3242,8 +3242,10 @@ ovsdb_idl_txn_write__(const struct ovsdb_idl_row *row_,
      * transaction only does writes of existing values, without making any real
      * changes, we will drop the whole transaction later in
      * ovsdb_idl_txn_commit().) */
-    if (write_only && ovsdb_datum_equals(ovsdb_idl_read(row, column),
-                                         datum, &column->type)) {
+    if (write_only && ((row->written && bitmap_is_set(row->written,
+                       column_idx)) || row->old) &&
+         ovsdb_datum_equals(ovsdb_idl_read(row, column),
+                            datum, &column->type)) {
         goto discard_datum;
     }