[ovs-dev] [PATCH] [RFC] ovn-controller: Experiment with restricting access to columns.

Han Zhou zhouhan at gmail.com
Thu Jun 14 03:29:28 UTC 2018

On Wed, Jun 13, 2018 at 3:37 PM, Ben Pfaff <blp at ovn.org> wrote:
> To make ovn-controller recompute incrementally, we need accurate
> dependencies for each function that reads or writes a table.  It's
> currently difficult to be sure about these dependencies, and certainly
> difficult to maintain them over time, because there's no way to actually
> enforce them.
> This commit experiments with an approach that allows for fairly
> fine-grained access control within ovn-controller to tables and columns.
> It's based on generating a new version of the IDL data structures for each
> case where we want different access control.  All of these data structures
> have the same format, but the columns that a given piece of code is not
> supposed to touch are renamed to discourage programmers from using them,
> e.g. they're given names suffixed with "__accessdenied".  (This means
> that there is no runtime overhead to the access control since it only
> requires a cast to convert between the regular and restricted versions.)
> In addition, when a columns is supposed to be read-only, functions for
> modifying the column are not supplied.
> This commit only tries out this experiment for a single file within
> ovn-controller, the BFD implementation (mostly because that's
> alphabetically first, no other real reason).  It would require a little
> more work to apply it everywhere, but it's probably not a huge deal.
> Comments?
> CC: Han Zhou <zhouhan at gmail.com>
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
>  ovn/controller/automake.mk         |   5 +
>  ovn/controller/bfd-vswitch-idl.def |  21 ++++
>  ovn/controller/bfd.c               |  20 ++--
>  ovn/controller/bfd.h               |   8 +-
>  ovn/controller/ovn-controller.c    |  13 ++-
>  ovsdb/ovsdb-idlc.in                | 223 ++++++++++++++++++++++++++++++
>  6 files changed, 268 insertions(+), 22 deletions(-)
>  create mode 100644 ovn/controller/bfd-vswitch-idl.def

I wanted to have a quick test but it didn't pass the compile:
In file included from ovn/controller/bfd.c:17:0:
ovn/controller/bfd.h:19:44: fatal error: ovn/controller/bfd-vswitch-idl.h:
No such file or directory

I didn't debug, but by looking at the code I got the idea. It ensures the
purpose is declared through the generated data type and violations are
captured at compile time.
I think this is a brilliant way to achieve the check with such a small
change, however, I am not sure how is it supposed to help for generating
the dependency. I think instead of caring about whether it is read-only, we
should care about whether it is write-only.

For example, a engine node A reads on Table T1's column C1, reads & writes
on Table T2's column C2, and writes on T3's C3. In this case A depends on
T1 and T2, but not depends on T3.
So I think what matters to the dependency generation is whether it is
Write-Only. Read-Only is no different from ReadWrite.

If my understanding is correct (that we don't care about the difference
between RO and RW), for WO columns, we can simply just don't monitor its
change. Then we don't even need this feature from dependency generation
point of view.
Did I miss something?

Another thing, however, I think is really important for the dependency
generation is the columns that reference other tables, e.g. bfd_run()
doesn't has table "ovsrec_port" as input, but in fact it depends on this
table by referencing from ovsrec_bridge->ports. We need the table being
referenced appear in the input parameters.
The approach in the patch may not solve that problem directly, but
something similar may work: we could declare the usage for a referenced
column as "direct" or "indirect". "indirect" means the column can be
accessed only with "get functions" instead of pointers. The "get functions"
requires an additional parameter that is the instance of the table being
referenced. To call the "get function" to access the column, the caller
function needs to have the table being referenced passed in as parameter,
so we will be able to catch the dependency. I am not sure if this approach
is too heavy, or is tricky to implement.


More information about the dev mailing list