openvswitch / ovs-issues

Issue tracker repo for Open vSwitch
10 stars 3 forks source link

max_len option for the controller action is never honored #295

Closed antoninbas closed 11 months ago

antoninbas commented 11 months ago

It seems that the max_len option for the controller action is never honored, at least not with the Linux kernel datapath.

Here is my OF flow:

 cookie=0xb020000000000, duration=3663.175s, table=23, n_packets=6, n_bytes=2652, idle_age=122, hard_age=1292, priority=64991,conj_id=1 actions=controller(reason=no_match,max_len=128,id=20030,userdata=02),resubmit(,26)

Here is the datapath flow:

recirc_id(0x260),tunnel(tun_id=0x0,src=192.168.77.100,dst=192.168.77.101,flags(-df-csum+key)),in_port(1),skb_mark(0/0x80000000),ct_state(+new-est-rel-rpl-inv+trk-snat),ct_mark(0/0x7f),ct_label(0/0xffffffff00000000),eth(src=7a:af:12:8d:2a:fd,dst=aa:bb:cc:dd:ee:ff),eth_type(0x0800),ipv4(src=10.10.0.0/255.255.255.0,dst=10.10.1.11,proto=17,ttl=63,frag=no),udp(src=53), packets:0, bytes:0, used:never, actions:set(eth(src=22:46:9b:32:04:5b,dst=3e:4f:e4:c9:88:f7)),set(ipv4(ttl=62)),userspace(pid=4294967295,controller(reason=0,dont_send=0,continuation=0,recirc_id=609,rule_cookie=0xb020000000000,controller_id=20030,max_len=128)),ct(commit,zone=65520,mark=0x1/0xf),recirc(0x262)

Even though max_len is set to 128, the controller receives the full packet, which is 428B.

I looked at the code. It seems that this was meant to be implemented just before sending the packet to the controller (in connmgr), as the entire packet is always upcalled by the kernel datapath anyway. Originally, the functionality was implemented in ofputil_encode_packet_in_private: https://github.com/openvswitch/ovs/blob/7a0f907b2393626dac1387617355990eab69aef7/lib/ofp-util.c#L3857-L3897

The implementation was then removed in OVS v2.7 by this patch: https://github.com/openvswitch/ovs/commit/c184807ced554c1dc7b69b3cd4f59cd85575fdf1

And as far as I can tell, this field has been unused ever since: https://github.com/openvswitch/ovs/blob/cf11766cbcf162399aafb84ba5634a22bccf9e8b/ofproto/connmgr.h#L49

Is this the expected behavior? Should the documentation for the controller action be updated to indicate that the max_len option may not be honored (or is never honored)?

igsilya commented 11 months ago

Hi. It is expected behavior. According to OpenFlow spec [1]:

Switches that do not support internal buffering, are configured to not buffer
packets for the packet-in event, or have run out of internal buffering, must
send the full packet to controllers as part of the [Packet-in] event.
<...>
If the packet is buffered, the number of bytes of the original packet to include
in the packet-in can be configured. <...>

OVS reports number of supported buffers as zero in the OFPT_FEATURES_REPLY, meaning that controller cannot expect buffering. And under these conditions, the switch that doesn't support buffering (OVS) must send the whole packet in the packet-in.

The ovs-actions man page may use some update, true.

[1] Section 6.1.2 : https://opennetworking.org/wp-content/uploads/2014/10/openflow-switch-v1.5.1.pdf

antoninbas commented 11 months ago

Thanks for the quick confirmation.

I can volunteer to update the documentation if that helps.

igsilya commented 11 months ago

I can volunteer to update the documentation if that helps.

Thanks! Feel free to submit a patch.

antoninbas commented 11 months ago

I submitted one: https://mail.openvswitch.org/pipermail/ovs-dev/2023-August/407311.html

V2: https://mail.openvswitch.org/pipermail/ovs-dev/2023-August/407313.html

antoninbas commented 11 months ago

@igsilya the patch was acked, would you mind applying it: https://mail.openvswitch.org/pipermail/ovs-dev/2023-August/407313.html ?

igsilya commented 11 months ago

Applied. Thanks!

antoninbas commented 11 months ago

Thanks. I confirmed that no change is required for the documentation of the output action, given that the following is not allowed:

$ ovs-ofctl mod-flows br-int 'table=0,priority=100,in_port=veth1,ip actions=output(port=controller,max_len=128)'
ovs-ofctl: output to unsupported truncate port: controller

which is already the documented behavior