From 9185896026dc384f39e9bce79a05b736e4cc4ba4 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Thu, 19 Jul 2012 18:30:12 -0700 Subject: [PATCH] ofproto: Move 'max_ports' from ofproto-dpif.c to ofproto.c. This allows port numbers in actions and elsewhere in OpenFlow messages to be checked at a higher level. Signed-off-by: Ben Pfaff --- ofproto/ofproto-dpif.c | 16 +++++++--------- ofproto/ofproto-provider.h | 30 +++++++++++++++++++----------- ofproto/ofproto.c | 26 ++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 20 deletions(-) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 78cb186a..beb12f0a 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -598,7 +598,6 @@ struct ofproto_dpif { struct hmap_node all_ofproto_dpifs_node; /* In 'all_ofproto_dpifs'. */ struct ofproto up; struct dpif *dpif; - int max_ports; /* Special OpenFlow rules. */ struct rule_dpif *miss_rule; /* Sends flow table misses to controller. */ @@ -744,6 +743,7 @@ construct(struct ofproto *ofproto_) { struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); const char *name = ofproto->up.name; + int max_ports; int error; int i; @@ -753,7 +753,9 @@ construct(struct ofproto *ofproto_) return error; } - ofproto->max_ports = dpif_get_max_ports(ofproto->dpif); + max_ports = dpif_get_max_ports(ofproto->dpif); + ofproto_init_max_ports(ofproto_, MIN(max_ports, OFPP_MAX)); + ofproto->n_matches = 0; dpif_flow_flush(ofproto->dpif); @@ -4639,7 +4641,7 @@ rule_construct(struct rule *rule_) enum ofperr error; error = ofpacts_check(rule->up.ofpacts, rule->up.ofpacts_len, - &rule->up.cr.flow, ofproto->max_ports); + &rule->up.cr.flow, ofproto->up.max_ports); if (error) { return error; } @@ -4749,7 +4751,7 @@ rule_modify_actions(struct rule *rule_) enum ofperr error; error = ofpacts_check(rule->up.ofpacts, rule->up.ofpacts_len, - &rule->up.cr.flow, ofproto->max_ports); + &rule->up.cr.flow, ofproto->up.max_ports); if (error) { ofoperation_complete(rule->up.pending, error); return; @@ -6443,11 +6445,7 @@ packet_out(struct ofproto *ofproto_, struct ofpbuf *packet, struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); enum ofperr error; - if (flow->in_port >= ofproto->max_ports && flow->in_port < OFPP_MAX) { - return OFPERR_NXBRC_BAD_IN_PORT; - } - - error = ofpacts_check(ofpacts, ofpacts_len, flow, ofproto->max_ports); + error = ofpacts_check(ofpacts, ofpacts_len, flow, ofproto->up.max_ports); if (!error) { struct odputil_keybuf keybuf; struct dpif_flow_stats stats; diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 15dc3472..72efb141 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -61,6 +61,7 @@ struct ofproto { /* Datapath. */ struct hmap ports; /* Contains "struct ofport"s. */ struct shash port_by_name; + uint16_t max_ports; /* Max possible OpenFlow port num, plus one. */ /* Flow tables. */ struct oftable *tables; @@ -93,6 +94,7 @@ struct ofproto { }; void ofproto_init_tables(struct ofproto *, int n_tables); +void ofproto_init_max_ports(struct ofproto *, uint16_t max_ports); struct ofproto *ofproto_lookup(const char *name); struct ofport *ofproto_get_port(const struct ofproto *, uint16_t ofp_port); @@ -365,6 +367,11 @@ struct ofproto_class { * ->construct() should delete flows from the underlying datapath, if * necessary, rather than populating the tables. * + * If the ofproto knows the maximum port number that the datapath can have, + * then it can call ofproto_init_max_ports(). If it does so, then the + * client will ensure that the actions it allows to be used through + * OpenFlow do not refer to ports above that maximum number. + * * Only one ofproto instance needs to be supported for any given datapath. * If a datapath is already open as part of one "ofproto", then another * attempt to "construct" the same datapath as part of another ofproto is @@ -946,17 +953,18 @@ struct ofproto_class { * * flow->in_port comes from the OpenFlow OFPT_PACKET_OUT message. The * implementation should reject invalid flow->in_port values by returning - * OFPERR_NXBRC_BAD_IN_PORT. For consistency, the implementation should - * consider valid for flow->in_port any value that could possibly be seen - * in a packet that it passes to connmgr_send_packet_in(). Ideally, even - * an implementation that never generates packet-ins (e.g. due to hardware - * limitations) should still allow flow->in_port values for every possible - * physical port and OFPP_LOCAL. The only virtual ports (those above - * OFPP_MAX) that the caller will ever pass in as flow->in_port, other than - * OFPP_LOCAL, are OFPP_NONE and OFPP_CONTROLLER. The implementation - * should allow both of these, treating each of them as packets generated - * by the controller as opposed to packets originating from some switch - * port. + * OFPERR_NXBRC_BAD_IN_PORT. (If the implementation called + * ofproto_init_max_ports(), then the client will reject these ports + * itself.) For consistency, the implementation should consider valid for + * flow->in_port any value that could possibly be seen in a packet that it + * passes to connmgr_send_packet_in(). Ideally, even an implementation + * that never generates packet-ins (e.g. due to hardware limitations) + * should still allow flow->in_port values for every possible physical port + * and OFPP_LOCAL. The only virtual ports (those above OFPP_MAX) that the + * caller will ever pass in as flow->in_port, other than OFPP_LOCAL, are + * OFPP_NONE and OFPP_CONTROLLER. The implementation should allow both of + * these, treating each of them as packets generated by the controller as + * opposed to packets originating from some switch port. * * (Ordinarily the only effect of flow->in_port is on output actions that * involve the input port, such as actions that output to OFPP_IN_PORT, diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 5c9ab9d3..ae24fd68 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -390,6 +390,7 @@ ofproto_create(const char *datapath_name, const char *datapath_type, ofproto->frag_handling = OFPC_FRAG_NORMAL; hmap_init(&ofproto->ports); shash_init(&ofproto->port_by_name); + ofproto->max_ports = OFPP_MAX; ofproto->tables = NULL; ofproto->n_tables = 0; ofproto->connmgr = connmgr_create(ofproto, datapath_name, datapath_name); @@ -422,6 +423,9 @@ ofproto_create(const char *datapath_name, const char *datapath_type, return 0; } +/* Must be called (only) by an ofproto implementation in its constructor + * function. See the large comment on 'construct' in struct ofproto_class for + * details. */ void ofproto_init_tables(struct ofproto *ofproto, int n_tables) { @@ -437,6 +441,24 @@ ofproto_init_tables(struct ofproto *ofproto, int n_tables) } } +/* To be optionally called (only) by an ofproto implementation in its + * constructor function. See the large comment on 'construct' in struct + * ofproto_class for details. + * + * Sets the maximum number of ports to 'max_ports'. The ofproto generic layer + * will then ensure that actions passed into the ofproto implementation will + * not refer to OpenFlow ports numbered 'max_ports' or higher. If this + * function is not called, there will be no such restriction. + * + * Reserved ports numbered OFPP_MAX and higher are special and not subject to + * the 'max_ports' restriction. */ +void +ofproto_init_max_ports(struct ofproto *ofproto, uint16_t max_ports) +{ + assert(max_ports <= OFPP_MAX); + ofproto->max_ports = max_ports; +} + uint64_t ofproto_get_datapath_id(const struct ofproto *ofproto) { @@ -2103,6 +2125,10 @@ handle_packet_out(struct ofconn *ofconn, const struct ofp_header *oh) if (error) { goto exit_free_ofpacts; } + if (po.in_port >= p->max_ports && po.in_port < OFPP_MAX) { + error = OFPERR_NXBRC_BAD_IN_PORT; + goto exit_free_ofpacts; + } /* Get payload. */ if (po.buffer_id != UINT32_MAX) { -- 2.30.2