Add support for large packet in the packetIn2 message #80

wenyingd · 2025-09-04T07:28:43Z

Refine the inbound function in openflow stream.
Fix byte slice copy issues.
Fix the potential overflow issue when encoding packetOut message

vanytsvetkov · 2025-09-04T07:47:25Z

A few questions, if I may.

If it's not a secret, what problem does this patch solve?
Are you sure that adding a proprietary vendor AntreaLargePacket field is the right way to develop the project? Why I'm asking this question is that when making changes to the openvswitch source code, we encountered a similar problem with the extensibility of the libOpenflow source code, which forced us to make a fork and make our own edits directly to it. I suggest looking towards the possibility of increasing the number of fields from outside the repository, rather than adding a custom field at the first problem.

wenyingd · 2025-09-04T07:59:47Z

A few questions, if I may.

1. If it's not a secret, what problem does this patch solve?

2. Are you sure that adding a proprietary vendor AntreaLargePacket field is the right way to develop the project? Why I'm asking this question is that when making changes to the openvswitch source code, we encountered a similar problem with the extensibility of the source code, which forced us to copy the source code and make our own edits directly to it. I suggest looking towards the possibility of increasing the number of fields from outside the repository, rather than adding a custom field at the first problem.

@vanytsvetkov Thanks for providing the feedback.

It is the issue this change is expected to resolve: antrea-io/antrea#7386

The new value of the property is set/consumed only inside libOpenflow rather than updating OVS source code. The new flag is used only when a large packetIn message is received in which the actual size of message is overflow, so the length directly decoded from the bytes is untrusted. If we search solution from outside repository, it means we need to ask OVS to extend the OpenFlow message format, e.g., switch the type of "length" field from uint16 to uint32, it may introduce more change in most of the OpenFlow users. So I don't think it is acceptable in a short time.

vanytsvetkov · 2025-09-04T08:22:10Z

A few questions, if I may.
1. If it's not a secret, what problem does this patch solve?

2. Are you sure that adding a proprietary vendor AntreaLargePacket field is the right way to develop the project? Why I'm asking this question is that when making changes to the openvswitch source code, we encountered a similar problem with the extensibility of the source code, which forced us to copy the source code and make our own edits directly to it. I suggest looking towards the possibility of increasing the number of fields from outside the repository, rather than adding a custom field at the first problem.
@vanytsvetkov Thanks for providing the feedback.

It is the issue this change is expected to resolve: antrea-io/antrea#7386

The new value of the property is set/consumed only inside libOpenflow rather than updating OVS source code. The new flag is used only when a large packetIn message is received in which the actual size of message is overflow, so the length directly decoded from the bytes is untrusted. If we search solution from outside repository, it means we need to ask OVS to extend the OpenFlow message format, e.g., switch the type of "length" field from uint16 to uint32, it may introduce more change in most of the OpenFlow users. So I don't think it is acceptable in a short time.

Thank you for your reply, now everything is clear.

antoninbas · 2025-09-04T18:10:22Z

@wenyingd Does this PR supersede #66 as well?

antoninbas · 2025-09-04T18:58:05Z

openflow13/bundles.go

 	n += 4
 	p.ExperimenterType = binary.BigEndian.Uint32(data[n:])
 	n += 4
 	if len(data) < int(p.Length) {


I am having a hard time understanding this code. I took a look at the Openflow spec and we should always have len(data) >= int(p.Length), i.e., the length of the message should be at least equal to the property length (which includes header + experimenter data). What am I missing?

Good catch, it should be an issue existing for long. I think here should be if n < int(p.Length)

wenyingd · 2025-09-05T01:53:57Z

@wenyingd Does this PR supersede #66 as well?

I don't think so. Do you want me to do that?

antoninbas · 2025-09-05T04:33:13Z

@wenyingd Does this PR supersede #66 as well?

I don't think so. Do you want me to do that?

Not necessarily. I just noticed that you commented on that PR, so I was wondering if there was any relationship between the 2 changes.

openflow15/nxt_message.go

openflow13/bundles.go

openflow15/nxt_message.go

util/stream.go

openflow15/nx_util.go

util/stream.go

util/stream_test.go

wenyingd · 2025-09-12T02:46:05Z

@antoninbas I removed "AntreaLargePacket" by modifying the message "unmarshal" logic in the latest update, could you take another look?

util/stream.go

openflow15/openflow15.go

antoninbas · 2025-09-15T22:42:07Z

openflow15/nxt_message.go

+	return uint16(p.ActLength())
+}

+func (p *PacketIn2PropPacket) ActLength() int {


Looking at the code I am having a hard time understanding why we need the ActLength method instead of just putting the correct implementation in the Len method for PacketIn2PropPacket? Then we would not need getPacketIn2PropertyLength?

The type of return value for function Len is uint16, it always map to the value of field "Length". And the function Len is defined in the interface Message. If we update the value type from uint16 to int, the impact is huge, we need to update almost all messages.

wenyingd · 2025-09-18T09:03:45Z

@antoninbas Do you have other comments on this PR? Or can we move forward?

antoninbas

While I am fine with the change, as I mentioned to you offline it feels like this is a bug in OVS (they should never generate an invalid Openflow message like this), and we should try to get to the bottom of this

openflow15/nxt_message_test.go

util/stream.go

openflow15/nxt_message.go

antoninbas · 2025-09-19T03:51:52Z

openflow15/nxt_message.go

+}

-	return nil
+func getPacketIn2PropertyLength(prop Property) int {


The getPacketIn2PropertyLength function returns the length of any property, not just PacketIn2PropPacket, so I am not sure why the function is named like this.

Renamed as getPropertyLength.

It doesn't seem that this comment has been addressed?
Can you make sure other resolved comments have been addressed as well?

openflow15/nxt_message.go

wenyingd · 2025-09-19T07:20:39Z

OVS issue is created: openvswitch/ovs-issues#371

wenyingd · 2025-09-23T01:56:55Z

@antoninbas Do you have other comments on this change?

antoninbas · 2025-09-23T20:24:48Z

openflow15/nxt_message.go

+}

-	return nil
+func getPacketIn2PropertyLength(prop Property) int {


It doesn't seem that this comment has been addressed?
Can you make sure other resolved comments have been addressed as well?

antoninbas · 2025-09-23T20:26:30Z

util/stream.go

+		}
+		// Check if the message is Type_PacketIn2 or not.
+		experimenterType := binary.BigEndian.Uint32(venderHeaderMessageBytes[12:])
+		if experimenterType != 30 {


Ok, and anyway this code is not supposed to be specific to openflow15

But maybe we should just redefine local constants in this file?

antoninbas · 2025-09-23T20:27:28Z

util/stream.go

+}
+
 // Handle inbound messages
 func (m *MessageStream) inbound() {


do you have good confidence that the updated function will perform as well, or almost as well (performance-wise), as the previous version?

Not did some performance test yet. In theory, the new logic should perform better.

antoninbas

LGTM, I think @tnqn should take a look too if he has time

antoninbas · 2025-09-24T04:52:19Z

util/stream.go

+		}
+		// Check if the message is Type_PacketIn2 or not.
+		experimenterType := binary.BigEndian.Uint32(venderHeaderMessageBytes[12:])
+		if experimenterType != 30 {


I was just suggesting adding local constants const experimenterTypePacketIn2 = 30 and const messageTypeVendorHeader = 4 in this file, not any major refactoring

tnqn · 2025-09-25T06:42:16Z

openflow15/nxt_message.go

+	return uint16(p.actualLength())
+}

+func (p *PacketIn2PropPacket) actualLength() int {
+	n := int(p.PropHeader.Len()) + int(p.Packet.Len())


If Len() just calls actualLength(), why it needs two functions?

Function Len return type is uint16, which is defined in the interface, if the sum of the fields is exceeds 2^16, the result will overflow. Function actualLength return type is int,

Added comments in the code.

1. Refine the inbound function in openflow stream. 2. Fix byte slice copy issues. 3. Fix the potential overflow issue when encoding packetOut message Signed-off-by: Wenying Dong <wenying.dong@broadcom.com>

tnqn · 2025-09-26T07:27:17Z

util/stream.go

+		length, err := getMessageLength(reader)
+		var smallOFErr *smallMessageError
+		var netErr *net.OpError
+		if errors.Is(err, io.EOF) ||


Doesn't Peek return ErrBufferFull when n is larger than buffer size? Why doesn't it handle that but EOF and other errors?

I don't get your question, "n" is always controlled by our call on Peek, the max value used in our function "getMessageLength" is 20, while reader may set its buf size with 4096 at least, so we shall not hit the issue.

While other errors we explicitly set are to catch the exceptions that we may hit in the runtime like connection issues and the message encoding issues from the sender (e.g. OVS).

got it, thanks for explaining.

Fixes #7386 See antrea-io/libOpenflow#80 Signed-off-by: Wenying Dong <wenying.dong@broadcom.com>

wenyingd requested review from antoninbas and tnqn September 4, 2025 07:28

wenyingd force-pushed the refine_stream branch from b13c44e to 41e5b58 Compare September 4, 2025 07:42

wenyingd mentioned this pull request Sep 4, 2025

Error when installing Endpoints flows for Service: bundle reply is timeout antrea-io/antrea#7386

Closed

antoninbas reviewed Sep 4, 2025

View reviewed changes

wenyingd force-pushed the refine_stream branch 2 times, most recently from a1f3333 to 3bcc3ff Compare September 5, 2025 02:22

antoninbas reviewed Sep 5, 2025

View reviewed changes

util/stream.go Outdated Show resolved Hide resolved

antoninbas reviewed Sep 5, 2025

View reviewed changes

util/stream.go Outdated Show resolved Hide resolved

wenyingd force-pushed the refine_stream branch 2 times, most recently from 319558c to 58ad864 Compare September 8, 2025 10:54

wenyingd requested a review from antoninbas September 9, 2025 08:51

antoninbas reviewed Sep 9, 2025

View reviewed changes

util/stream_test.go Outdated Show resolved Hide resolved

wenyingd requested a review from antoninbas September 10, 2025 05:30

wenyingd force-pushed the refine_stream branch 2 times, most recently from ddba8b4 to e3d225b Compare September 12, 2025 06:33

antoninbas reviewed Sep 15, 2025

View reviewed changes

wenyingd force-pushed the refine_stream branch from e3d225b to b856438 Compare September 16, 2025 02:40

wenyingd requested a review from antoninbas September 16, 2025 02:46

wenyingd force-pushed the refine_stream branch from b856438 to aa26a34 Compare September 16, 2025 06:42

antoninbas reviewed Sep 19, 2025

View reviewed changes

wenyingd force-pushed the refine_stream branch from aa26a34 to 82aa839 Compare September 19, 2025 06:53

wenyingd requested a review from antoninbas September 22, 2025 01:36

antoninbas reviewed Sep 23, 2025

View reviewed changes

wenyingd force-pushed the refine_stream branch from 82aa839 to 3c8cd16 Compare September 24, 2025 02:39

wenyingd requested a review from antoninbas September 24, 2025 03:07

wenyingd force-pushed the refine_stream branch from 3c8cd16 to c5f6902 Compare September 24, 2025 03:10

antoninbas previously approved these changes Sep 24, 2025

View reviewed changes

wenyingd dismissed antoninbas’s stale review via f50ea96 September 24, 2025 05:53

wenyingd force-pushed the refine_stream branch from c5f6902 to f50ea96 Compare September 24, 2025 05:53

tnqn reviewed Sep 25, 2025

View reviewed changes

wenyingd requested a review from tnqn September 26, 2025 06:07

Add support for large packet in the packetIn2 message

2efa1bc

1. Refine the inbound function in openflow stream. 2. Fix byte slice copy issues. 3. Fix the potential overflow issue when encoding packetOut message Signed-off-by: Wenying Dong <wenying.dong@broadcom.com>

wenyingd force-pushed the refine_stream branch from f50ea96 to 2efa1bc Compare September 26, 2025 06:18

tnqn reviewed Sep 26, 2025

View reviewed changes

tnqn approved these changes Sep 26, 2025

View reviewed changes

wenyingd merged commit 4ae5ac9 into antrea-io:main Sep 26, 2025
4 checks passed

antoninbas pushed a commit to antrea-io/antrea that referenced this pull request Sep 26, 2025

Bump up libOpenflow to support OpenFlow message overflow (#7470)

6f8cec9

Fixes #7386 See antrea-io/libOpenflow#80 Signed-off-by: Wenying Dong <wenying.dong@broadcom.com>

Add support for large packet in the packetIn2 message #80

Add support for large packet in the packetIn2 message #80

Uh oh!

Conversation

wenyingd commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vanytsvetkov commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wenyingd commented Sep 4, 2025

Uh oh!

vanytsvetkov commented Sep 4, 2025

Uh oh!

antoninbas commented Sep 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenyingd commented Sep 5, 2025

Uh oh!

antoninbas commented Sep 5, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wenyingd commented Sep 12, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenyingd Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenyingd commented Sep 18, 2025

Uh oh!

antoninbas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wenyingd commented Sep 19, 2025

Uh oh!

wenyingd commented Sep 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenyingd commented Sep 4, 2025 •

edited

Loading

vanytsvetkov commented Sep 4, 2025 •

edited

Loading

wenyingd Sep 16, 2025 •

edited

Loading

wenyingd Sep 26, 2025 •

edited

Loading