OFFlowMod (at least) allows conflicting OFVersions in fields

[rizard]

@rlane
@Sovietaced
@capveg

I apologize in advance -- this is quite long for completeness. My proposed solutions are at the bottom.

I'm not sure why anyone would ever want to do this intentionally, but Loxi's OpenFlowJ allows you to build OFFlowMods (and possibly other OFMessages -- have not looked into them all yet) with elements of different OFVersions. This isn't a bug in Loxi per-se, but it allowed a bug to occur in Floodlight that was difficult to track down ...but was obvious in hindsight, of course :-) IMHO, this can be addressed in Loxi up front to eliminate the possibility of this bug for others using the Java library at least.

As an example, an OFFlowAdd.Builder allows you to set a Match that was constructed with a different OFVersion (i.e. from a different OFFactory version).

Match m = my13Factory.buildMatch() /* OpenFlow 1.3 Match */
  /*
   * set MatchFields here
   */
  .build();

OFFlowAdd fa = my10Factory.buildFlowAdd() /* OpenFlow 1.0 OFFlowAdd */
  /*
   * set other items here
   */
  .setMatch(m) /* set the 1.3 Match in the 1.0 flow */
  .build();

The build succeeds and you can actually write the message to the switch, which is unfortunate.

The following is an example of an OpenFlow 1.0 flow mod that is sent by Floodlight to the switch. As you can see, it has an OpenFlow 1.3 OXM list of matches where the rigid match structure of an OF1.0 flow mod should be instead. Wireshark has trouble parsing this. (The same mismatch happens for an OF1.3 flow mod and an OF1.0 match, and likely any other version of OF.)

In the screenshot of the Wireshark capture above, take a look at bytes 0x64-0x69 of the OpenFlow data -- those are what should be the source MAC address. Now, take a look at bytes 0x60-0x63 -- the four bytes immediately preceding the MAC address. They are 0x80000806.

We all know what an OXM is, but for completeness, an OXM contains the class as bits 31-16, the TLV ID as bits 15-9, the has-mask flag as bit 8, and the length as bits 7-0:

[31--class--16] + [15--TLV-ID--9] + [8--has-mask--8] + [7--length--0]

Bits 31-16, 0x8000 is the OpenFlowBasic OXM class, which contains all standard OpenFlow matches. The trailing 0x06, bits 7-0, is the length of a MAC address (6 bytes), and the 0x08 in the middle, bits 15-8, is actually 0b00001000. Remove the LSB (the has-mask flag) and the TLV ID is: 0b0000100X, which leaves 0b0000100, which is 4 in decimal. 4 is the TLV ID for a source MAC address.

This pattern of 0x8000 followed by the TLV ID and the length can be found in front of the other fields being matched in the bad flow. It's clearly a list of OXMs that we want to match on as opposed to the rigid match structure that should be in an OF1.0 flow mod header.

Under the hood, in OpenFlowJ-Loxi, the Match is stored as a generic Match but the version is not checked to make sure it's the same as the flow mod being composed.

So, how do we fix this? We can certainly explicitly check the OpenFlow version of any and all applicable fields set in an OFMessage builder to make sure the version field is the same as the OFMessage builder itself. We could also force setters, e.g. setMatch(Match m), to only accept same-version types of the builder they're being set in. For example, setMatch(OFMatchV1 m) could be used to accept OF1.0 matches only and not any Match as is presently done with setMatch(Match m). For any OF version, version-specific matches at least are already easily built via e.g. my10Factory.buildMatchV1(). I can't say for sure about other version-specific types we might need to consider for other OFMessages.

Is there a better or preferred way? I wanted to start the conversation before I went hacking at it myself.

[Sovietaced]

Hi @rizard. Sorry for the delay and thanks for pointing this out. I don't see any circumstances under which someone would want to add message components from different OF versions so I naively think it would make sense to add these checks automatically with Loxi.

I'm sure @andi-bigswitch has an opinion.

[andi-bigswitch]

I agree -- I can't see any point in composing an OFMessages out of parts with different versions. It's an oversight that OpenFlowJ/Loxi allows that. We should fix it.

One place where we could add that check is the constructor of the OFMessages; somewhere around https://github.com/floodlight/loxigen/blob/master/java_gen/templates/of_class.java#L76

Basically, in that loop that checks for null values, we would also check if the child object type is a Loxigen object (an OFObject). This is the parent of all OF-related objects generated by loxigen, and allows to query the version. If the child is an OFObject, put code in the constructor that validates the version against the parent version.

The same should be done for Collections of OFObject (the, validating each member).

I think you should be able to retrieve that information from the member object (class JavaMember).

Would either of you want to take a stab at fixing this?

[Sovietaced]

Sure.