[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