diff mbox series

[ovs-dev] tests: Add check for correct l3l4 conntrack frag reassembly

Message ID 1570116942-30058-1-git-send-email-gvrose8192@gmail.com
State Superseded
Headers show
Series [ovs-dev] tests: Add check for correct l3l4 conntrack frag reassembly | expand

Commit Message

Gregory Rose Oct. 3, 2019, 3:35 p.m. UTC 
Two commits recently fixed an issue with setting the corrrect l3 and l4
flow information when conntrack reassembles packet fragments.

c98f776 datapath: Clear the L4 portion of the key for "later" fragments
2609173 datapath: Properly set L4 keys on "later" IP fragments

This test checks for regressions that might break this feature.  It
counts on the fact that when the bug is present the udp src port
will not be correct.  It will either be zero or else some other
garbage value.  So the test feeds some fragments through for
reassembly and then checks to make sure that the udp srce port
is actually the correct value of 5001.

Tested by reverting the above commits and observing that the test
then fails.

Signed-off-by: Greg Rose <gvrose8192@gmail.com>
---
 tests/system-traffic.at | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

+AT_CLEANUP
+
 AT_BANNER([conntrack - L7])
 
 AT_SETUP([conntrack - IPv4 HTTP])

Comments

Ben Pfaff Oct. 3, 2019, 1:08 p.m. UTC | #1 
On Thu, Oct 03, 2019 at 08:35:42AM -0700, Greg Rose wrote:
> Two commits recently fixed an issue with setting the corrrect l3 and l4
> flow information when conntrack reassembles packet fragments.
> 
> c98f776 datapath: Clear the L4 portion of the key for "later" fragments
> 2609173 datapath: Properly set L4 keys on "later" IP fragments
> 
> This test checks for regressions that might break this feature.  It
> counts on the fact that when the bug is present the udp src port
> will not be correct.  It will either be zero or else some other
> garbage value.  So the test feeds some fragments through for
> reassembly and then checks to make sure that the udp srce port
> is actually the correct value of 5001.
> 
> Tested by reverting the above commits and observing that the test
> then fails.
> 
> Signed-off-by: Greg Rose <gvrose8192@gmail.com>

This patch doesn't apply, with:

    error: corrupt patch at line 43

I think that it's probably because this has very long lines.  Email
doesn't do well with lines over about 1000 bytes long.  I know of two
ways to avoid the problem: use pull requests instead of email, or
shorten the lines, e.g. by splicing with \ or dnl.  The latter might be
a little better because it means that patches that later update or
correct the tests will pass through email OK.

Will you try one of these?

Thanks,

Ben.
0-day Robot Oct. 3, 2019, 3:57 p.m. UTC | #2 
Bleep bloop.  Greetings Greg Rose, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
fatal: corrupt patch at line 43
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 tests: Add check for correct l3l4 conntrack frag reassembly
The copy of the patch that failed is found in:
   /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch
When you have resolved this problem, run "git am --resolved".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Gregory Rose Oct. 3, 2019, 4:32 p.m. UTC | #3 
On 10/3/2019 6:08 AM, Ben Pfaff wrote:
> On Thu, Oct 03, 2019 at 08:35:42AM -0700, Greg Rose wrote:
>> Two commits recently fixed an issue with setting the corrrect l3 and l4
>> flow information when conntrack reassembles packet fragments.
>>
>> c98f776 datapath: Clear the L4 portion of the key for "later" fragments
>> 2609173 datapath: Properly set L4 keys on "later" IP fragments
>>
>> This test checks for regressions that might break this feature.  It
>> counts on the fact that when the bug is present the udp src port
>> will not be correct.  It will either be zero or else some other
>> garbage value.  So the test feeds some fragments through for
>> reassembly and then checks to make sure that the udp srce port
>> is actually the correct value of 5001.
>>
>> Tested by reverting the above commits and observing that the test
>> then fails.
>>
>> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
> This patch doesn't apply, with:
>
>      error: corrupt patch at line 43
>
> I think that it's probably because this has very long lines.  Email
> doesn't do well with lines over about 1000 bytes long.  I know of two
> ways to avoid the problem: use pull requests instead of email, or
> shorten the lines, e.g. by splicing with \ or dnl.  The latter might be
> a little better because it means that patches that later update or
> correct the tests will pass through email OK.
>
> Will you try one of these?

Hmm, yes.  git send-email complained about that.  OK, let me try 
reformatting the patch so it works
better with email.

- Greg
Gregory Rose Oct. 3, 2019, 5:48 p.m. UTC | #4 
> On 10/3/2019 6:08 AM, Ben Pfaff wrote:
>> On Thu, Oct 03, 2019 at 08:35:42AM -0700, Greg Rose wrote:
>>> Two commits recently fixed an issue with setting the corrrect l3 and l4
>>> flow information when conntrack reassembles packet fragments.
>>>
>>> c98f776 datapath: Clear the L4 portion of the key for "later" fragments
>>> 2609173 datapath: Properly set L4 keys on "later" IP fragments
>>>
>>> This test checks for regressions that might break this feature.  It
>>> counts on the fact that when the bug is present the udp src port
>>> will not be correct.  It will either be zero or else some other
>>> garbage value.  So the test feeds some fragments through for
>>> reassembly and then checks to make sure that the udp srce port
>>> is actually the correct value of 5001.
>>>
>>> Tested by reverting the above commits and observing that the test
>>> then fails.
>>>
>>> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
>> This patch doesn't apply, with:
>>
>>      error: corrupt patch at line 43
>>
>> I think that it's probably because this has very long lines. Email
>> doesn't do well with lines over about 1000 bytes long.  I know of two
>> ways to avoid the problem: use pull requests instead of email, or
>> shorten the lines, e.g. by splicing with \ or dnl.  The latter might be
>> a little better because it means that patches that later update or
>> correct the tests will pass through email OK.
>>
>> Will you try one of these?
>

Hi Ben,

I sent a V2 and I just tried applying it from patchworks and seems OK.

thanks,

- Greg
diff mbox series

Patch

diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index bfc6bb5..804a3d2 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -3245,6 +3245,27 @@  AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([conntrack - fragment reassembly test])
+CHECK_CONNTRACK()
+OVS_TRAFFIC_VSWITCHD_START()
+
+AT_DATA([flows.txt], [dnl
+action=normal
+])
+
+AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
+
+AT_CHECK([ovs-ofctl packet-out br0 "packet=52540003287c525400444ab586dd6006f70605b02c4020010001000000000000000000000020200100010000000000000000000000101100000134e88deb138913890808031361616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161
 61616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616
 16161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161
 616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161, actions=ct(table=1)"])
+
+AT_CHECK([ovs-ofctl packet-out br0 "packet=52540003287c525400444ab586dd6006f70602682c402001000100000000000000000000002020010001000000000000000000000010110005a834e88deb616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161
 6161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161, actions=ct(table=1)"])
+
+AT_CHECK([ovs-ofctl packet-out br0 "packet=52540003287c525400444ab586dd6006f706033d1140200100010000000000000000000000202001000100000000000000000000001013891389033d9238616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161
 616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161610a, actions=ct(table=1)"])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | head -2 | tail -1 | grep -q -e ["]udp[(]src=5001["]])
+
+OVS_TRAFFIC_VSWITCHD_STOP