Looks good, thanks.
Ethan
On Fri, Jul 20, 2012 at 4:25 PM, Ben Pfaff <b...@nicira.com> wrote:
> This allows port numbers in actions and elsewhere in OpenFlow messages to
> be checked at a higher level.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> 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 b95179a..f1d2ae0 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -588,7 +588,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. */
> @@ -734,6 +733,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;
>
> @@ -743,7 +743,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);
> @@ -4593,7 +4595,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;
> }
> @@ -4703,7 +4705,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;
> @@ -6359,11 +6361,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 6eef106..2f62473 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 467dfda..cbcf0d2 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -389,6 +389,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);
> @@ -421,6 +422,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)
> {
> @@ -436,6 +440,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)
> {
> @@ -2100,6 +2122,10 @@ handle_packet_out(struct ofconn *ofconn, const struct
> ofp_packet_out *opo)
> 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) {
> --
> 1.7.2.5
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
