diff mbox

[ovs-dev,1/3] ofp-actions: Look inside write_actions for output ports and groups.

Message ID OF5086A0D9.F9FB9FE8-ON88257EC4.006CBF7A-87257EC4.006E1BAD@selinc.com
State Not Applicable
Headers show

Commit Message

gavin_remaley@selinc.com Sept. 18, 2015, 8:02 p.m. UTC 
This patch looks good, though it looks like the general issue with 
examining the Actions within a Write Actions instruction has several 
instances beyond what is addressed by this patch.

I have noticed that an invalid Group ID within Write Actions is not caught 
(Flow is programmed with no error).  That appears to be a problem in 
ofproto.c -> ofproto_check_ofpacts.  A similar fix seemed to address that 
issue.

I did a global search for OFPACT_FOR_EACH and examined the action 
processing.  I see several other potential problems with Write Actions.

ofp-actions.c -> ofpacts_check
ofp-actions.c -> ofpacts_get_meter
ofproto.c -> ofproto_check_ofpacts

Thanks for your help.



From:   Ben Pfaff <blp@nicira.com>
To:     dev@openvswitch.org
Cc:     Ben Pfaff <blp@nicira.com>, Gavin Remaley 
<gavin_remaley@selinc.com>
Date:   09/08/2015 04:55 PM
Subject:        [PATCH 1/3] ofp-actions: Look inside write_actions for 
output ports and groups.



The out_port and out_group matches only looked at apply_actions
instructions, but my interpretation of the OpenFlow spec is that they
should also look inside write_actions.

Reported-by: Gavin Remaley <gavin_remaley@selinc.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
---
 AUTHORS           |  1 +
 lib/ofp-actions.c | 21 +++++++++++++++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

     case OFPACT_OUTPUT_REG:
     case OFPACT_BUNDLE:
@@ -6113,7 +6119,6 @@ ofpact_outputs_to_port(const struct ofpact *ofpact, 
ofp_port_t port)
     case OFPACT_POP_MPLS:
     case OFPACT_SAMPLE:
     case OFPACT_CLEAR_ACTIONS:
-    case OFPACT_WRITE_ACTIONS:
     case OFPACT_GOTO_TABLE:
     case OFPACT_METER:
     case OFPACT_GROUP:
@@ -6149,9 +6154,17 @@ ofpacts_output_to_group(const struct ofpact 
*ofpacts, size_t ofpacts_len,
     const struct ofpact *a;
 
     OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
-        if (a->type == OFPACT_GROUP
-            && ofpact_get_GROUP(a)->group_id == group_id) {
-            return true;
+        if (a->type == OFPACT_GROUP) {
+            if (ofpact_get_GROUP(a)->group_id == group_id) {
+                return true;
+            }
+        } else if (a->type == OFPACT_WRITE_ACTIONS) {
+            const struct ofpact_nest *nested = 
ofpact_get_WRITE_ACTIONS(a);
+            if (ofpacts_output_to_group(nested->actions,
+ ofpact_nest_get_action_len(nested),
+                                        group_id)) {
+                return true;
+            }
         }
     }

Comments

Ben Pfaff Sept. 29, 2015, 11:34 p.m. UTC | #1 
On Fri, Sep 18, 2015 at 02:02:40PM -0600, gavin_remaley@selinc.com wrote:
> This patch looks good, though it looks like the general issue with 
> examining the Actions within a Write Actions instruction has several 
> instances beyond what is addressed by this patch.
> 
> I have noticed that an invalid Group ID within Write Actions is not caught 
> (Flow is programmed with no error).  That appears to be a problem in 
> ofproto.c -> ofproto_check_ofpacts.  A similar fix seemed to address that 
> issue.
> 
> I did a global search for OFPACT_FOR_EACH and examined the action 
> processing.  I see several other potential problems with Write Actions.
> 
> ofp-actions.c -> ofpacts_check
> ofp-actions.c -> ofpacts_get_meter
> ofproto.c -> ofproto_check_ofpacts
> 
> Thanks for your help.

You're right, I missed those.

I sent an improved series here:
        http://openvswitch.org/pipermail/dev/2015-September/060624.html
diff mbox

Patch

diff --git a/AUTHORS b/AUTHORS
index a7f40bb..0c4d020 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -259,6 +259,7 @@  Eivind Bulie Haanaes
 Eric Lopez              elopez@nicira.com
 Frido Roose             fr.roose@gmail.com
 Gaetano Catalli         gaetano.catalli@gmail.com
+Gavin Remaley           gavin_remaley@selinc.com
 George Shuklin          amarao@desunote.ru
 Gerald Rogers           gerald.rogers@intel.com
 Ghanem Bahri            bahri.ghanem@gmail.com
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index c46ce97..ee686c1 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -6072,6 +6072,12 @@  ofpact_outputs_to_port(const struct ofpact *ofpact, 
ofp_port_t port)
         return ofpact_get_ENQUEUE(ofpact)->port == port;
     case OFPACT_CONTROLLER:
         return port == OFPP_CONTROLLER;
+    case OFPACT_WRITE_ACTIONS: {
+        const struct ofpact_nest *nested = 
ofpact_get_WRITE_ACTIONS(ofpact);
+        return ofpacts_output_to_port(nested->actions,
+                                      ofpact_nest_get_action_len(nested),
+                                      port);
+    }