search

frenetic-lang / ocaml-openflow

Serialization library for OpenFlow
Other
13 stars 4 forks source link

Fix bug in IP wildcarding. #172

Closed jnfoster closed 10 years ago

jnfoster commented 10 years ago

This pull requests fixes a bug in the calculation of IP masks in OpenFlow 0x01.

seliopou commented 10 years ago

Whitespace.

seliopou commented 10 years ago

The line-wrapping introduced more whitespace errors. There are also tabs:

$ git diff --check master..HEAD
lib/SDN_OpenFlow0x01.ml:54: tab in indent.
+        if m = 32l then
lib/SDN_OpenFlow0x01.ml:55: tab in indent.
+          None
lib/SDN_OpenFlow0x01.ml:56: tab in indent.
+        else
lib/SDN_OpenFlow0x01.ml:57: tab in indent.
+          Some (Int32.sub 32l m) in
lib/SDN_OpenFlow0x01.ml:63: tab in indent.
+        if m = 32l then
lib/SDN_OpenFlow0x01.ml:64: tab in indent.
+          None
lib/SDN_OpenFlow0x01.ml:65: tab in indent.
+        else
lib/SDN_OpenFlow0x01.ml:66: tab in indent.
+          Some (Int32.sub 32l m) in
lib/SDN_OpenFlow0x01.ml:132: trailing whitespace.
+      | AL.Modify (AL.SetEthTyp _) -> 
lib/SDN_OpenFlow0x01.ml:133: tab in indent.
+        raise (Invalid_argument "cannot set Ethernet type")
lib/SDN_OpenFlow0x01.ml:134: trailing whitespace.
+      | AL.Modify (AL.SetIPProto _) -> 
lib/SDN_OpenFlow0x01.ml:135: tab in indent.
+        raise (Invalid_argument "cannot set IP protocol")
lib/SDN_OpenFlow0x01.ml:147: trailing whitespace.
+  : Core.action list = 
lib/SDN_OpenFlow0x01.ml:151: trailing whitespace.
+  | _ ->

Adding the pre-commit hook will notify you of stuff like this before you commit:

$ cp .git/hooks/pre-commit{.sample,}
jnfoster commented 10 years ago

By the way: @mcanini @fugitifduck I only fixed 1.0 since it's the code I'm most familiar with. Do you know if 1.3 and 1.4 might also have the same bug?

-N

fugitifduck commented 10 years ago

@jnfoster I think, line 20 of OpenFlow0x0(4|5)_Core.ml is code like in 0x01.

But, starting from 0x02, nwsrc is include in Oxm, and the mask is a CIDR mask (where /32 is exact and /0 is wildcard). It can also be arbitrary masking... (currently not implemented i think)

jnfoster commented 10 years ago

Cool. So, just to make sure I understand, they switched to CIDR conventions so there shouldn't be a bug in our 1.3/1.4 code?

fugitifduck commented 10 years ago

I think (p52 of OF1.3.3 spec)

jnfoster commented 10 years ago

Excellent. Thanks!

mcanini commented 10 years ago

@jnfoster

Actually I am not sure that this won't be a bug in 1.3/1.4 too.

Quoting the specs (1.3.3): "Each 1-bit in oxm_mask constrains the OXM TLV to match only packets in which the corresponding bit of the field equals the the corresponding bit in oxm_value. Each 0-bit in oxm_mask places no constraint on the corresponding bit in the field."

So, in my interpretation, something link 192.168.1.0/24 means that the mask has to be 24 MSB set to 1 followed by 8 LSB set to 0.

mcanini commented 10 years ago

So, after a bit of investigation, indeed it seems that the bug affects OF1.3/1.4 too. @fugitifduck is implementing the fix. Thanks!

jnfoster commented 10 years ago

Fantastic! Great to hear.

-N

On Fri, Sep 5, 2014 at 10:58 AM, Marco Canini notifications@github.com wrote:

So, after a bit of investigation, indeed it seems that the bug affects OF1.3/1.4 too. @fugitifduck https://github.com/fugitifduck is implementing the fix. Thanks!

— Reply to this email directly or view it on GitHub https://github.com/frenetic-lang/ocaml-openflow/pull/172#issuecomment-54635831 .