diff mbox series

[ovs-dev,v1,1/1] datapath-windows : TFTP rep Avoid a deadlock when processing TFTP conntrack.

Message ID 20240604090625.62514-1-svc.ovs-community@vmware.com
State Superseded
Headers show
Series [ovs-dev,v1,1/1] datapath-windows : TFTP rep Avoid a deadlock when processing TFTP conntrack. | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation fail test: fail

Commit Message

Wilson Peng June 4, 2024, 9:06 a.m. UTC 
From: Wilson Peng <pweisong@vmware.com>

It is found the TFTP reply packet with source port 69 will trigger host hang
And the possible coredump.

According to part 4 in TFTP RFC https://datatracker.ietf.org/doc/html/rfc1350,
The TFTP reply packet should use a new source-port(not 69) to connect to
Client.

Upon this TFTP reply packet ovs-windows kernel part will meet deadlock as it
Does set entry->parent to be equal to entry itself.

Reproducing step(installing ovsext driver on Win2019 server):

Topo: podvif38--192.168.10.38-----ovs----192.168.10.40----podvif40

Setup flow on local setup,
$Vif38Name="podvif38"
$Vif40Name="podvif40"
ovs-ofctl del-flows br-int --strict "table=0,priority=0"
ovs-ofctl add-flow br-int "table=0,in_port=$Vif38Name,udp,
    actions=ct(commit,alg=tftp),output:$Vif40Name"

Constructing a TFTP request and reply packet using scapy below,
TFTPPacket1=Ether(dst="00:15:5d:04:a0:a2",src="00:15:5d:04:a0:a1")/
            IP(src="192.168.10.38",dst="192.168.10.40")/
            UDP(sport=51000,dport=69)/TFTP()/
            TFTP_RRQ(filename="OVSIM_CERT.cer",mode="octet")

TFTPPacket2=Ether(dst="00:15:5d:04:a0:a1",src="00:15:5d:04:a0:a2")/
            IP(src="192.168.10.40",dst="192.168.10.38")/
            UDP(sport=69,dport=51000)/TFTP()/
            TFTP_ERROR(errorcode=4,errormsg=b'Access violation')

Sending the packet via ovs CMD,
ovs-ofctl packet-out br-int 1 'resubmit(,0)'
00155D04A0A800155D04A0A7080045000033000100004011E51AC0A80A26C0A80A28C7380045001'
FBFCC00014F5653494D5F434552542E636572006F6374657400

ovs-ofctl packet-out br-int 1 'resubmit(,0)'
00155D04A0A700155D04A0A8080045000031000100004011E51CC0A80A28C0A80A260045C73800'
1DB033000500044163636573732076696F6C6174696F6E2E00

Without the fix, it will HANG and not recovering.

It is good to backport the fix to main and backported until 3.1

Signed-off-by: Wilson Peng <pweisong@vmware.com>
---
 datapath-windows/ovsext/Conntrack.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

0-day Robot June 4, 2024, 9:20 a.m. UTC | #1 
References:  <20240604090625.62514-1-svc.ovs-community@vmware.com>
 

Bleep bloop.  Greetings Wilson Peng, 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.


checkpatch:
WARNING: The subject, '<area>: <summary>', is over 70 characters, i.e., 76.
Subject: datapath-windows : TFTP rep Avoid a deadlock when processing TFTP conntrack.
Lines checked: 82, Warnings: 1, Errors: 0


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

Thanks,
0-day Robot
diff mbox series

Patch

diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
index 39ba5cc10..ae91ed18e 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -1033,9 +1033,15 @@  OvsProcessConntrackEntry(OvsForwardingContext *fwdCtx,
             } else {
                 POVS_CT_ENTRY parentEntry;
                 parentEntry = OvsCtRelatedLookup(ctx->key, currentTime);
-                entry->parent = parentEntry;
-                if (parentEntry != NULL) {
-                    state |= OVS_CS_F_RELATED;
+                if (((layers->isIPv6 && key->ipv6Key.nwProto == IPPROTO_UDP) ||
+                    (!(layers->isIPv6) && key->ipKey.nwProto == IPPROTO_UDP)) &&
+                    (parentEntry == entry)) {
+                    /* Do nothing here, it would deadlock for invalid tftp packet*/
+                } else {
+                   entry->parent = parentEntry;
+                   if (parentEntry != NULL) {
+                       state |= OVS_CS_F_RELATED;
+                   }
                 }
             }
         }