On 10/08/2026 20:20, Mike Pattrick wrote:
*External email: Use caution opening links or attachments*
On Mon, Aug 10, 2026 at 6:25 AM Ilya Maximets <[email protected]> wrote:
On 8/9/26 3:03 PM, Eli Britstein wrote:
> RFC 791 permits zero-payload last fragments, but common stacks
> including the Linux kernel discard them. Align the reassembly test
> with that policy: use valid one-byte fragments, and update
expectations
> for duplicate handling (recirc flow and expiry coverage count).
>
> Assisted-by: composer-2.5-fast, Cursor
> Signed-off-by: Eli Britstein <[email protected]>
> ---
> v2-v1:
> - Fixed non robust allocated recirc_id.
>
> tests/ofproto-dpif.at
<http://ofproto-dpif.at/>
| 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/tests/ofproto-dpif.at
<http://ofproto-dpif.at/>
b/tests/ofproto-dpif.at
<http://ofproto-dpif.at/>
> index ee6ac873d..644b7b7fb 100644
> --- a/tests/ofproto-dpif.at
<http://ofproto-dpif.at/>
> +++ b/tests/ofproto-dpif.at
<http://ofproto-dpif.at/>
> @@ -5536,23 +5536,24 @@ table=2 tcp actions=3
> AT_CHECK([ovs-ofctl -O OpenFlow11 replace-flows br0 flows.txt])
>
> dnl Test frag expiry.
> -AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af
0026b98cb0f9 0800 4500 0014 0001 8192 40 06 3316 ac11370d ac11370b"])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af
0026b98cb0f9 0800 4500 0015 0001 8192 40 06 3315 ac11370d ac11370b
00"])
> ovs-appctl time/warp 10000
> -AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af
0026b98cb0f9 0800 4500 0014 0001 8192 40 06 3316 ac11370d ac11370b"])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af
0026b98cb0f9 0800 4500 0015 0001 8192 40 06 3315 ac11370d ac11370b
00"])
>
> dnl Test that no packets flow.
So, this is not true anymore, the second packet is now delivered to
the destination, hence the extra datapath flow below. It seems to
be not an intended behavior of this test. What is also not clear to
me is that the test seems to assume that expiration timeout is 10
seconds, not 15, which may be the root of the issue here. By the
time the second packet arrives the previous one is not expired yet,
though the comments like "Test frag expiry" and "Expire second frag"
suggest that "time/warp 10000" supposed to expire the fragment.
Should we change the warp instead of the expected flows?
This was from a while ago and I don't have any notes on my intention
for this test. But the comment looks incorrect as you point out. From
the functionality of the test it looks like it's testing that an
overlapping fragment gets rejected from ipf and sent, with the time
warp clearing any dp flows first.
Those are partially correct. The change behavior was indeed not
intended. The 2nd flow was because the extra byte in the packet caused
it, not because of timing.
I will add a new commit to properly drop overlap fragments instead of
marking CT_INVALID (see line 855).
-M
CC: Mike.
> -AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\) |
sed s'/recirc(.*)/recirc(X)/'], [0], [dnl
> +AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\) |
sed 's/recirc[[^)]]*)/recirc()/g' | sort], [0], [dnl
nit: Should probably use strip_recirc helper here. And we should keep
the same pattern for all other calls within the test, so it is
consistent.
Ack. I will do it in a separated commit.
> flow-dump from the main thread:
>
-recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth(src=00:26:b9:8c:b0:f9),eth_type(0x0800),ipv4(src=172.17.55.13/128.0.0.0,proto=6,frag=later
<http://172.17.55.13/128.0.0.0,proto=6,frag=later>),
packets:0, bytes:0, used:never, actions:ct(commit,zone=10),recirc(X)
>
+recirc(),in_port(90),packet_type(ns=0,id=0),eth(src=00:26:b9:8c:b0:f9),eth_type(0x0800),ipv4(src=172.17.55.13/128.0.0.0,proto=6,frag=later
<http://172.17.55.13/128.0.0.0,proto=6,frag=later>),
packets:0, bytes:0, used:never, actions:ct(commit,zone=10),recirc()
>
+recirc(),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=later),
packets:0, bytes:0, used:never, actions:2
> ])
>
> dnl Expire second frag.
> ovs-appctl time/warp 10000
>
> dnl Test frag purge
> -AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af
0026b98cb0f9 0800 4500 0014 0002 8192 40 06 3315 ac11370d ac11370b"])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af
0026b98cb0f9 0800 4500 0015 0002 8192 40 06 3314 ac11370d ac11370b
00"])
> ovs-appctl time/warp 33000
> -AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af
0026b98cb0f9 0800 4500 0014 0003 8192 40 06 3314 ac11370d ac11370b"])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af
0026b98cb0f9 0800 4500 0015 0003 8192 40 06 3313 ac11370d ac11370b
00"])
>
> dnl Test that no packets flow.
> AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\) |
sed s'/recirc(.*)/recirc(X)/'], [0], [dnl
> @@ -5563,8 +5564,9 @@
recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth(src=00:26:b9:8c:b0:f9),eth_t
> dnl Purge second frag
> ovs-appctl time/warp 33000
>
> -dnl Make sure all four packets are counted properly in the
coverage.
> -AT_CHECK([ovs-appctl coverage/show | grep -c "^ipf.*total: 2"],
[0], [2
> +dnl Make sure expiry and purge counters reflect the four
packets sent.
> +dnl The duplicate second fragment is counted as overlap, not a
second expiry.
> +AT_CHECK([ovs-appctl coverage/show | grep -c "^ipf.*total: 2"],
[0], [1
> ])
>
> zero1208=$(printf '%0*d' 2416 0)
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
