[ovs-discuss] [PATCH] ofproto/ofproto.c, lib/vconn.c:: OFPBRC_BAD_VERSION generated from switch when there is version mismatch.

Prabina Pattnaik Prabina.Pattnaik at nechclst.in
Fri May 25 13:16:44 UTC 2012


Hi Ben,

This is grey out area of openflow 1.0 spec that with which request packet "OFPBRC_BAD_VERSION" should come.
If we go with remaining request packet like ECHO_REQUEST, BARRIER_REQUEST and GET_CONFIG_REQUEST, this error is not coming and connection is resetting in case of BARRIER_REQUEST and GET_CONFIG_REQUEST.

We haven't handled for above packets because controller is not supposed to get any useful data in a data field from switch.
It can be supported for OFPBRC_BAD_VERSION error type to handle all request packets to prevent the reset connection when there is version mismatch between request and reply packet.

Regards,
Prabina

-----Original Message-----
From: Ben Pfaff [mailto:blp at nicira.com] 
Sent: Wednesday, May 23, 2012 10:11 PM
To: Prabina Pattnaik
Cc: bugs at openvswitch.org; Varun Gupta; discuss at openvswitch.org
Subject: Re: [ovs-discuss] [PATCH] ofproto/ofproto.c, lib/vconn.c:: OFPBRC_BAD_VERSION generated from switch when there is version mismatch.

You made changes to a few randomly selected OpenFlow messages.  What
is special about these messages?  Why change them and not others?

On Wed, May 23, 2012 at 11:59:15AM +0000, Prabina Pattnaik wrote:
> Hi Ben,
> 
> Design description and rationale -
> ---------------------------------------
> We are adding switch configuration message (OFPT_FEATURE_REQUEST) and 
> statistics message (OFPT_STATS_REQUEST) as a condition in vconn.c file 
> to prevent the reset of connection on receiving incorrect version in 
> feature request and stat request packets from controller.
> Checking the version mismatch between stat request / feature request 
> packets and version supported by switch in ofproto.c file. If versions 
> are different then return the error packet to controller.  
> 
> Testing after merging patch in OVS 1.4.1
> -------------------------------------------------
> 
> 1.	Verify OFPET_BAD_REQUEST  OFPBRC_BAD_VERSION code message from 
> switch to controller for  switch description statistics,  aggregate statistics, 
> table statistics,  port statistics and queue statistics when there is mismatch version.
> 2.	Verify stats request  and reply message from controller to switch 
> for switch description statistics,  aggregate statistics, table statistics,  
> port statistics and queue statistics when version match.
> 3.	Verify OFPET_BAD_REQUEST  OFPBRC_BAD_VERSION code message from switch 
> to controller for  feature request message when there is mismatch version.
> 4.	Verify feature request and feature reply messages from controller to 
> switch for switch description statistics,  aggregate statistics, table statistics,  
> flow statistics and queue statistics when version match.
> 
> 
> Patch
> ---------
>  ofproto/ofproto.c               |   21 +++++++++++++++++++++
>  lib/vconn.c |    4 +++-
>  2 files changed, 24 insertions(+), 1 deletions(-)
> 
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 9057215..0a7fbcc 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -2894,6 +2894,9 @@ handle_openflow__(struct ofconn *ofconn, const struct ofpbuf *msg)
>          return handle_echo_request(ofconn, oh);
>  
>      case OFPUTIL_OFPT_FEATURES_REQUEST:
> +        if (oh->version != OFP_VERSION) {
> +            return ofp_mkerr(OFPET_BAD_REQUEST,OFPBRC_BAD_VERSION); 
> +        }
>          return handle_features_request(ofconn, oh);
>  
>      case OFPUTIL_OFPT_GET_CONFIG_REQUEST:
> @@ -2933,23 +2936,41 @@ handle_openflow__(struct ofconn *ofconn, const struct ofpbuf *msg)
>  
>          /* Statistics requests. */
>      case OFPUTIL_OFPST_DESC_REQUEST:
> +        if (oh->version != OFP_VERSION) {
> +            return ofp_mkerr(OFPET_BAD_REQUEST,OFPBRC_BAD_VERSION);
> +        }
>          return handle_desc_stats_request(ofconn, msg->data);
>  
>      case OFPUTIL_OFPST_FLOW_REQUEST:
>      case OFPUTIL_NXST_FLOW_REQUEST:
> +        if (oh->version != OFP_VERSION) {
> +            return ofp_mkerr(OFPET_BAD_REQUEST,OFPBRC_BAD_VERSION); 
> +        }
>          return handle_flow_stats_request(ofconn, msg->data);
>  
>      case OFPUTIL_OFPST_AGGREGATE_REQUEST:
>      case OFPUTIL_NXST_AGGREGATE_REQUEST:
> +        if (oh->version != OFP_VERSION) {
> +            return ofp_mkerr(OFPET_BAD_REQUEST,OFPBRC_BAD_VERSION); 
> +        }
>          return handle_aggregate_stats_request(ofconn, msg->data);
>  
>      case OFPUTIL_OFPST_TABLE_REQUEST:
> +        if (oh->version != OFP_VERSION) {
> +            return ofp_mkerr(OFPET_BAD_REQUEST,OFPBRC_BAD_VERSION);
> +        }
>          return handle_table_stats_request(ofconn, msg->data);
>  
>      case OFPUTIL_OFPST_PORT_REQUEST:
> +        if (oh->version != OFP_VERSION) {
> +            return ofp_mkerr(OFPET_BAD_REQUEST,OFPBRC_BAD_VERSION); 
> +        }
>          return handle_port_stats_request(ofconn, msg->data);
>  
>      case OFPUTIL_OFPST_QUEUE_REQUEST:
> +        if (oh->version != OFP_VERSION) {
> +            return ofp_mkerr(OFPET_BAD_REQUEST,OFPBRC_BAD_VERSION);
> +         }
>          return handle_queue_stats_request(ofconn, msg->data);
>  
>      case OFPUTIL_MSG_INVALID:
> 
> diff --git a/lib/vconn.c b/lib/vconn.c
> index 6ea9366..c499623 100644
> --- a/lib/vconn.c
> +++ b/lib/vconn.c
> @@ -546,7 +546,9 @@ do_recv(struct vconn *vconn, struct ofpbuf **msgp)
>              && oh->type != OFPT_ERROR
>              && oh->type != OFPT_ECHO_REQUEST
>              && oh->type != OFPT_ECHO_REPLY
> -            && oh->type != OFPT_VENDOR)
> +            && oh->type != OFPT_VENDOR
> +            && oh->type != OFPT_FEATURES_REQUEST
> +            && oh->type != OFPT_STATS_REQUEST)
>          {
>              if (vconn->version < 0) {
>                  VLOG_ERR_RL(&bad_ofmsg_rl,
> 
> Regards,
> Prabina
> 
> -----Original Message-----
> From: discuss-bounces at openvswitch.org [mailto:discuss-bounces at openvswitch.org] On Behalf Of Ben Pfaff
> Sent: Tuesday, May 22, 2012 11:40 PM
> To: Varun Gupta
> Cc: bugs at openvswitch.org
> Subject: Re: [ovs-discuss] OpenVSwitch - "OFPBRC_BAD_VERSION" error packet not received when there is version mismatch between stats request and stats reply
> 
> Can you provide a patch?  I'll apply it, if it's reasonable.
> 
> On Tue, May 22, 2012 at 07:40:49AM +0000, Varun Gupta wrote:
> > The two issues I raised (stats request/reply and feature request/reply) were found when we were evaluating the switch's response in case the controller behave indifferently. 
> > Our objective is to make OVS more robust and respond with an error (OFPBRC_BAD_VERSION)  in the cases of 'OFPET_BAD_REQUEST' error type as mentioned in spec (1.0):
> > 
> > -------------------------------------------
> > 
> > For the OFPET_BAD_REQUEST error type, the following codes are currently de-
> > fined:
> > /* ofp_error_msg 'code' values for OFPET_BAD_REQUEST. 'data' contains at least
> > * the first 64 bytes of the failed request. */
> > enum ofp_bad_request_code {
> > OFPBRC_BAD_VERSION, /* ofp_header.version not supported. */
> > ----------
> > ----------
> > };
> > 
> > ------------------------------------------
> > 
> > Regards,
> > Varun
> > 
> > -----Original Message-----
> > From: Ben Pfaff [mailto:blp at nicira.com] 
> > Sent: Monday, May 21, 2012 10:04 PM
> > To: Varun Gupta
> > Cc: bugs at openvswitch.org
> > Subject: Re: [ovs-discuss] OpenVSwitch - "OFPBRC_BAD_VERSION" error packet not received when there is version mismatch between stats request and stats reply
> > 
> > On Mon, May 21, 2012 at 10:50:19AM +0000, Varun Gupta wrote:
> > > Switch is not notifying the controller for OFPBRC_BAD_VERSION, /*
> > > ofp_header.version not supported. */ when there is version mismatch
> > > between stats request and stats reply. The switch was resetting the
> > > connection and only a warning message was generated on switch in log
> > > (/var/log/messages) before resetting connection.
> > 
> > It's hard for me to consider this a serious bug, because it's a bug
> > in the controller to send an OpenFlow message of a version that was
> > not negotiated in the OpenFlow version negotiation.  I don't know why
> > you'd do that.
> > 
> > 
> > 
> > DISCLAIMER:
> > 
> > -----------------------------------------------------------------------------------------------------------------------
> > 
> > The contents of this e-mail and any attachment(s) are confidential and
> > intended
> > 
> > for the named recipient(s) only. 
> > 
> > It shall not attach any liability on the originator or NECHCL or its
> > 
> > affiliates. Any views or opinions presented in 
> > 
> > this email are solely those of the author and may not necessarily reflect the
> > 
> > opinions of NECHCL or its affiliates. 
> > 
> > Any form of reproduction, dissemination, copying, disclosure, modification,
> > 
> > distribution and / or publication of 
> > 
> > this message without the prior written consent of the author of this e-mail is
> > 
> > strictly prohibited. If you have 
> > 
> > received this email in error please delete it and notify the sender
> > 
> > immediately. .
> > 
> > -----------------------------------------------------------------------------------------------------------------------
> _______________________________________________
> discuss mailing list
> discuss at openvswitch.org
> http://openvswitch.org/mailman/listinfo/discuss
> 
> 
> 
> DISCLAIMER:
> 
> -----------------------------------------------------------------------------------------------------------------------
> 
> The contents of this e-mail and any attachment(s) are confidential and
> intended
> 
> for the named recipient(s) only. 
> 
> It shall not attach any liability on the originator or NECHCL or its
> 
> affiliates. Any views or opinions presented in 
> 
> this email are solely those of the author and may not necessarily reflect the
> 
> opinions of NECHCL or its affiliates. 
> 
> Any form of reproduction, dissemination, copying, disclosure, modification,
> 
> distribution and / or publication of 
> 
> this message without the prior written consent of the author of this e-mail is
> 
> strictly prohibited. If you have 
> 
> received this email in error please delete it and notify the sender
> 
> immediately. .
> 
> -----------------------------------------------------------------------------------------------------------------------



DISCLAIMER:

-----------------------------------------------------------------------------------------------------------------------

The contents of this e-mail and any attachment(s) are confidential and
intended

for the named recipient(s) only. 

It shall not attach any liability on the originator or NECHCL or its

affiliates. Any views or opinions presented in 

this email are solely those of the author and may not necessarily reflect the

opinions of NECHCL or its affiliates. 

Any form of reproduction, dissemination, copying, disclosure, modification,

distribution and / or publication of 

this message without the prior written consent of the author of this e-mail is

strictly prohibited. If you have 

received this email in error please delete it and notify the sender

immediately. .

-----------------------------------------------------------------------------------------------------------------------



More information about the discuss mailing list