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

Ben Pfaff blp at nicira.com
Fri May 25 16:17:40 UTC 2012


I think the handling should be uniform, if we need any at all.

On Fri, May 25, 2012 at 01:16:44PM +0000, Prabina Pattnaik wrote:
> 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