Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_PACKET_IN_DATA_OFFSET calculation conflicts with ASSERT for OF1.3 only #207

Open
samwhitlock opened this issue Feb 20, 2014 · 7 comments
Open

Comments

@samwhitlock
Copy link

I'm working with a recent version of Indigo and the loxigen-artifacts submodule (merged the changes from today).

how to trigger

Here are the important steps in my code to trigger this bug:

  1. Create an OpenFlow 1.3 packet in object
of_packet_in_t *packet_in = (of_packet_in_t*) of_packet_in_new(OF_VERSION_1_3);

\2. Set the data in it to send to the controller (i.e., don't buffer the packet locally)

of_packet_in_data_set(packet_in, &octets);

\3. the _PACKET_IN_DATA_OFFSET macro sets the offset to 26, and the call to _END_LEN returns 8. Packet length is 34 when I observe this behavior.

\4. Because cur_len != 0 and cur_len != value->bytes in of_wire_buffer_octets_data_set, the assertion fails.


I don't observe this behavior when I use OpenFlow 1.2 packets.

@rlane
Copy link
Contributor

rlane commented Mar 13, 2014

Here's a minimal failing testcase:

    uint8_t data[] = { 1, 2, 3, 4 };
    of_octets_t octets = { .bytes = 4, .data = data };
    of_packet_in_t *packet_in = (of_packet_in_t*) of_packet_in_new(OF_VERSION_1_3);
    of_packet_in_data_set(packet_in, &octets);

This works:

    of_match_t match;
    uint8_t data[] = { 1, 2, 3, 4 };
    memset(&match, 0, sizeof(match));
    match.version = OF_VERSION_1_3;
    of_octets_t octets = { .bytes = 4, .data = data };
    of_packet_in_t *packet_in = (of_packet_in_t*) of_packet_in_new(OF_VERSION_1_3);
    of_packet_in_match_set(packet_in, &match);
    of_packet_in_data_set(packet_in, &octets);

I don't know what's causing this bug yet, but setting an empty match is an easy workaround. We've never seen this because we always set the match in packet-ins.

@bjlee72
Copy link
Contributor

bjlee72 commented Jun 2, 2014

Is this issue resolved?

@capveg
Copy link

capveg commented Aug 2, 2014

This looks like a resolved issue so unless I hear otherwise, I'm going to close it.

@andi-bigswitch
Copy link
Contributor

@rlane is this related to cd6c1d6?

@rlane
Copy link
Contributor

rlane commented Oct 20, 2014

I don't think so, given that the padding also exists in OF 1.2.

@rlane
Copy link
Contributor

rlane commented Oct 24, 2014

I found the issue. There's a hack in gen_new_fn_body that manually initializes the length field (but not the type) of embedded matches. It uses the offset from OF 1.2, but OF 1.3 added a cookie field, changing the offset of the match. So for OF 1.3, the initial length is written into the cookie field instead of the match length field.

I'll fix this after the OF 1.4 changes are merged.

@andi-bigswitch
Copy link
Contributor

@rlane - did we end up fixing this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants