diff mbox

[ovs-dev] ovn: Set critical bit in Geneve option.

Message ID 1471381125-77810-1-git-send-email-jesse@kernel.org
State Accepted
Headers show

Commit Message

Jesse Gross Aug. 16, 2016, 8:58 p.m. UTC
Currently the Geneve option type that OVN uses is 0, which in
Geneve marks this as non-critical. Non-critical means that if a
receiver does not recognize this option, it is free to ignore it
and continue processing the packet.

OVN uses its option to transmit things like input and output port
which are used to enforce security policies and direct packets to
their correct location. If the recipicient of a packet ignored this
information then it would likely be a security hole. This would seem
to qualify the option as critical.

There's no issue in an instance of OVN as currently written - the
receiver will always match on the option data. However, if a
theoretical future version that did not use this option was connected
or a third-party component was introduced then it's possible that this
might be accidentally ignored.

This patch changes the option type used by OVN to include the
critical bit to properly mark the intention. Obviously, this will
cause interoperability issues with any existing deployments but
it should be fine while OVN is still labeled as experimental.

Signed-off-by: Jesse Gross <jesse@kernel.org>
---
 ovn/controller/physical.h  | 2 +-
 ovn/ovn-architecture.7.xml | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Russell Bryant Aug. 16, 2016, 9:04 p.m. UTC | #1
On Tue, Aug 16, 2016 at 4:58 PM, Jesse Gross <jesse@kernel.org> wrote:

> Currently the Geneve option type that OVN uses is 0, which in
> Geneve marks this as non-critical. Non-critical means that if a
> receiver does not recognize this option, it is free to ignore it
> and continue processing the packet.
>
> OVN uses its option to transmit things like input and output port
> which are used to enforce security policies and direct packets to
> their correct location. If the recipicient of a packet ignored this
> information then it would likely be a security hole. This would seem
> to qualify the option as critical.
>
> There's no issue in an instance of OVN as currently written - the
> receiver will always match on the option data. However, if a
> theoretical future version that did not use this option was connected
> or a third-party component was introduced then it's possible that this
> might be accidentally ignored.
>
> This patch changes the option type used by OVN to include the
> critical bit to properly mark the intention. Obviously, this will
> cause interoperability issues with any existing deployments but
> it should be fine while OVN is still labeled as experimental.
>
> Signed-off-by: Jesse Gross <jesse@kernel.org>
>

Thanks for the detailed explanation.  That makes sense to me.  For master
and 2.6:

Acked-by: Russell Bryant <russell@ovn.org>
Jesse Gross Aug. 17, 2016, 1:54 a.m. UTC | #2
On Tue, Aug 16, 2016 at 2:04 PM, Russell Bryant <russell@ovn.org> wrote:
>
> On Tue, Aug 16, 2016 at 4:58 PM, Jesse Gross <jesse@kernel.org> wrote:
>>
>> Currently the Geneve option type that OVN uses is 0, which in
>> Geneve marks this as non-critical. Non-critical means that if a
>> receiver does not recognize this option, it is free to ignore it
>> and continue processing the packet.
>>
>> OVN uses its option to transmit things like input and output port
>> which are used to enforce security policies and direct packets to
>> their correct location. If the recipicient of a packet ignored this
>> information then it would likely be a security hole. This would seem
>> to qualify the option as critical.
>>
>> There's no issue in an instance of OVN as currently written - the
>> receiver will always match on the option data. However, if a
>> theoretical future version that did not use this option was connected
>> or a third-party component was introduced then it's possible that this
>> might be accidentally ignored.
>>
>> This patch changes the option type used by OVN to include the
>> critical bit to properly mark the intention. Obviously, this will
>> cause interoperability issues with any existing deployments but
>> it should be fine while OVN is still labeled as experimental.
>>
>> Signed-off-by: Jesse Gross <jesse@kernel.org>
>
>
> Thanks for the detailed explanation.  That makes sense to me.  For master
> and 2.6:
>
> Acked-by: Russell Bryant <russell@ovn.org>

Thanks - I applied this to master and branch-2.6.
diff mbox

Patch

diff --git a/ovn/controller/physical.h b/ovn/controller/physical.h
index 92680dc..28845b2 100644
--- a/ovn/controller/physical.h
+++ b/ovn/controller/physical.h
@@ -37,7 +37,7 @@  struct simap;
  *
  * Keep these in sync with the documentation in ovn-architecture(7). */
 #define OVN_GENEVE_CLASS 0x0102  /* Assigned Geneve class for OVN. */
-#define OVN_GENEVE_TYPE 0
+#define OVN_GENEVE_TYPE 0x80     /* Critical option. */
 #define OVN_GENEVE_LEN 4
 
 void physical_register_ovs_idl(struct ovsdb_idl *);
diff --git a/ovn/ovn-architecture.7.xml b/ovn/ovn-architecture.7.xml
index 1a1bd39..de2a376 100644
--- a/ovn/ovn-architecture.7.xml
+++ b/ovn/ovn-architecture.7.xml
@@ -1180,7 +1180,7 @@ 
 
     <!-- Keep the following in sync with ovn/controller/physical.h. -->
     OVN transmits the logical ingress and logical egress ports in a TLV with
-    class 0x0102, type 0, and a 32-bit value encoded as follows, from MSB to
+    class 0x0102, type 0x80, and a 32-bit value encoded as follows, from MSB to
     LSB:
   </p>