Hi, Simon,
In this case, for tftp packet processing, it does have a child-parent
processing logic just like ftp in tcp.
Tftp packet1 from port1 to 69 and it will create one new conntrack entry
and create one related conntrack
entry also with src-port 0 and dst-port port1. Original idea is if the
tftp reply packet is from port2 to port1 and
then it will create one new conntrack entry then it could set this
conntrack entry(port2->port1) parent be equal
to conntrack_entry(port1-->69).
In the processing part, if the tftp reply packet is from 69 to port1
then it would hit the original conntrack entry
(port1-->69) and lead to conntrack_entry parent be equal to itself.
Only ftp/tftp traffic processing will have parent-child logic.
Regards
Wilson
On Wed, Jun 5, 2024 at 8:32 PM Simon Horman <[email protected]> wrote:
> On Wed, Jun 05, 2024 at 01:35:52PM +0800, Wilson Peng via dev wrote:
> > From: Wilson Peng <[email protected]>
> >
> > 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')
>
> Hi Wilson,
>
> From an L4 perspective the above packet flow seems very normal -
> the src/dst addresses and ports are reversed. So I am curious
> to know what it is about TFTP - other than the RFC reference above -
> that makes this special from an OVS PoV.
>
> In other words, why does parent == entry in this case,
> but, I assume, not for other Ether/IPv4/UDP cases?
>
> >
> > 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, Windows node will HANG and not responding.
> >
> > It is good to backport the fix to main and backported until 3.1
> >
> > Signed-off-by: Wilson Peng <[email protected]>
> > ---
> > datapath-windows/ovsext/Conntrack.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/datapath-windows/ovsext/Conntrack.c
> b/datapath-windows/ovsext/Conntrack.c
> > index 39ba5cc10..17a1257f6 100644
> > --- a/datapath-windows/ovsext/Conntrack.c
> > +++ b/datapath-windows/ovsext/Conntrack.c
> > @@ -410,6 +410,8 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
> > }
> > if (state != OVS_CS_F_INVALID && commit) {
> > if (entry) {
> > + /* make sure that parentEntry is not equal to entry*/
> > + ASSERT(!parentEntry || (parentEntry != entry));
> > entry->parent = parentEntry;
> > if (OvsCtAddEntry(entry, ctx, natInfo, currentTime)) {
> > *entryCreated = TRUE;
> > @@ -1033,8 +1035,9 @@ OvsProcessConntrackEntry(OvsForwardingContext
> *fwdCtx,
> > } else {
> > POVS_CT_ENTRY parentEntry;
> > parentEntry = OvsCtRelatedLookup(ctx->key, currentTime);
> > - entry->parent = parentEntry;
> > - if (parentEntry != NULL) {
> > + /* make sure that parentEntry is not equal to entry*/
> > + if ((parentEntry != NULL) && (parentEntry != entry)) {
> > + entry->parent = parentEntry;
> > state |= OVS_CS_F_RELATED;
> > }
> > }
> > --
> > 2.39.2 (Apple Git-143)
> >
> > _______________________________________________
> > dev mailing list
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
--
This electronic communication and the information and any files transmitted
with it, or attached to it, are confidential and are intended solely for
the use of the individual or entity to whom it is addressed and may contain
information that is confidential, legally privileged, protected by privacy
laws, or otherwise restricted from disclosure to anyone else. If you are
not the intended recipient or the person responsible for delivering the
e-mail to the intended recipient, you are hereby notified that any use,
copying, distributing, dissemination, forwarding, printing, or copying of
this e-mail is strictly prohibited. If you received this e-mail in error,
please return the e-mail to the sender, delete it from your computer, and
destroy any printed copy of it.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
