Hi, Alin,
I have explained the issue in this thread below(
https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/).
And also I have supplied the reproducing CMD for this hang issue.
If it is constructing some TFTP (request/reply) packet to inject into
the ovs windows setup, the WIndows node will
hange. In a simple word, the SPIN_LOCK will be called twice on the same
conntrack entry if the entry is equal to parent
entry.( this is possible for TFTP packet handling).
The TFTP packet logic is added around 2022, this patch would fix the
potential hang issue with the fix.
Regards
Wilson
on datapath-windows/ovsext/Conntrack.c OvsCtExecute_() function
1275 OVS_ACQUIRE_SPIN_LOCK(&(entry->lock), irql); ------>SPIN_LOCK 1st time
1343 OVS_ACQUIRE_SPIN_LOCK(&(parent->lock), irql);
--->>SPIN_LOCK 2nd time causing deadlock
On Tue, Aug 6, 2024 at 6:48 AM Alin Serdean <[email protected]> wrote:
> Hi Wilson,
>
> Apologies for the delay and thank you for your patch.
>
> Its really hard for me to wrap my head around how this issue occurred.
>
> Would it be possible to reproduce the issue and run the !locks command
> inside WinDbg and provide the output?
>
> Thank you,
> Alin.
> ------------------------------
> *From:* dev <[email protected]> on behalf of Wilson Peng
> via dev <[email protected]>
> *Sent:* Thursday, June 6, 2024 3:47 PM
> *To:* Simon Horman <[email protected]>; [email protected] <
> [email protected]>
> *Subject:* Re: [ovs-dev] [PATCH v2 1/1] datapath-windows : Avoid a
> deadlock when processing TFTP conntrack.
>
> Hi, Simon,
>
> Below list why it would go into deadlock,
>
> 1). 1st place OvsCtExecute_()->OvsProcessConntrackEntry(). entry->parent
> = parentEntry;(here parentEntry == entry)
> 2). After it return back from OvsProcessConntrackEntry(), it goes to the
> lines below,(Still on function OvsCtExecute_())
> 1263 if (entry == NULL) {
> 1264 return status;
> 1265 }
> 1266
> 1267 /*
> 1268 * Note that natInfo is not the same as entry->natInfo here.
> natInfo
> 1269 * is decided by action in the openflow rule, entry->natInfo is
> decided
> 1270 * when the entry is created. In the reverse NAT case, natInfo is
> 1271 * NAT_ACTION_REVERSE, yet entry->natInfo is NAT_ACTION_SRC or
> 1272 * NAT_ACTION_DST without NAT_ACTION_REVERSE
> 1273 */
> 1274 KIRQL irql = KeGetCurrentIrql();
> 1275 OVS_ACQUIRE_SPIN_LOCK(&(entry->lock), irql);
> ------>SPIN_LOCK 1st time
> 1276 if (natInfo->natAction != NAT_ACTION_NONE) {
> 1277 OvsNatPacket(fwdCtx, entry, entry->natInfo.natAction,
> 1278 key, ctx.reply);
> 1279 }
>
> 3, Then it will goto the next same SPIN_LOCK for then lines below(Still on
> function OvsCtExecute_())
> 1340 /* Add original tuple information to flow Key */
> 1341 if (entry->key.dl_type == ntohs(ETH_TYPE_IPV4)) {
> 1342 if (parent != NULL) {
> 1343 OVS_ACQUIRE_SPIN_LOCK(&(parent->lock), irql);
> --->>SPIN_LOCK 2nd time causing deadlock
> 1344 OvsCtUpdateTuple(key, &parent->key);
> 1345 OVS_RELEASE_SPIN_LOCK(&(parent->lock), irql);
> 1346 } else {
> 1347 OvsCtUpdateTuple(key, &entry->key);
> 1348 }
> ...
>
> 1365 OVS_RELEASE_SPIN_LOCK(&(entry->lock), irql); --->this is the
> SPIN_LOCK release place.
>
> Regards
> Wilson
>
> On Thu, Jun 6, 2024 at 8:56 PM Simon Horman <[email protected]> wrote:
>
> > On Wed, Jun 05, 2024 at 09:26:02PM +0800, Wilson Peng wrote:
> > > 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.
> >
> > Hi Wilson,
> >
> > Thanks for the explanation.
> >
> > I understand that this patch will prevent OVS from crashing (good!). But
> > I'm unclear how flows that would have caused a crash will be handled by
> > conntrack with this patch present. Could you shed some light on that
> too?
> >
>
> --
> 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
>
--
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
