Sure, happy to help if we have a patch. Best, Wenying
On Thu, Jun 6, 2024 at 7:19 PM Alin Serdean <[email protected]> wrote: > Hi Ilya, > > Thanks for the patch. Will try to get a setup and test it. > > In theory if we have a new analyze target of a newer visual studio it > should flag those types of issues. > > Will try to send a patch for the aforementioned target. > > Wenying can you help in testing the patch? > > Thank you, > Alin. > ------------------------------ > *From:* Ilya Maximets <[email protected]> > *Sent:* Thursday, June 6, 2024 12:52 PM > *To:* [email protected] <[email protected]> > *Cc:* Alin Serdean <[email protected]>; Wenying Dong <[email protected]>; > Ilya Maximets <[email protected]> > *Subject:* [PATCH] datapath-windows: Fix parsing of split buffers in > OvsGetTcpHeader. > > NdisGetDataBuffer() is called without providing a buffer to copy packet > data in case it is not contiguous. So, it fails in some scenarios > where the packet is handled by the general network stack before OVS > and headers become split in multiple buffers. > > Use existing helpers to retrieve the headers instead, they are using > OvsGetPacketBytes() which should be able to handle split data. > > It might be a bit slower than getting direct pointers that may be > provided by NdisGetDataBuffer(), but it's better to optimize commonly > used OvsGetPacketBytes() helper in the future instead of optimizing > every single caller separately. And we're still copying the TCP > header anyway. > > Fixes: 9726a016d9d6 ("datapath-windows: Implement locking in conntrack > NAT.") > Reported-at: https://github.com/openvswitch/ovs-issues/issues/323 > Signed-off-by: Ilya Maximets <[email protected]> > --- > > WARNING: I beleive this code is correct, but I did not test it with real > traffic, I only verified that it compiles. Should not be applied > unless someone tests it in an actual Windows setup. > > datapath-windows/ovsext/Conntrack.c | 45 ++++++++++++++--------------- > 1 file changed, 21 insertions(+), 24 deletions(-) > > diff --git a/datapath-windows/ovsext/Conntrack.c > b/datapath-windows/ovsext/Conntrack.c > index 39ba5cc10..4649805dd 100644 > --- a/datapath-windows/ovsext/Conntrack.c > +++ b/datapath-windows/ovsext/Conntrack.c > @@ -678,46 +678,43 @@ OvsGetTcpHeader(PNET_BUFFER_LIST nbl, > VOID *storage, > UINT32 *tcpPayloadLen) > { > - IPHdr *ipHdr; > - IPv6Hdr *ipv6Hdr; > - TCPHdr *tcp; > + IPv6Hdr ipv6HdrStorage; > + IPHdr ipHdrStorage; > + const IPv6Hdr *ipv6Hdr; > + const IPHdr *ipHdr; > + const TCPHdr *tcp; > VOID *dest = storage; > uint16_t ipv6ExtLength = 0; > > if (layers->isIPv6) { > - ipv6Hdr = NdisGetDataBuffer(NET_BUFFER_LIST_FIRST_NB(nbl), > - layers->l4Offset + sizeof(TCPHdr), > - NULL, 1, 0); > + ipv6Hdr = OvsGetPacketBytes(nbl, sizeof *ipv6Hdr, > + layers->l3Offset, &ipv6HdrStorage); > if (ipv6Hdr == NULL) { > return NULL; > } > > - tcp = (TCPHdr *)((PCHAR)ipv6Hdr + layers->l4Offset); > - ipv6Hdr = (IPv6Hdr *)((PCHAR)ipv6Hdr + layers->l3Offset); > - if (tcp->doff * 4 >= sizeof *tcp) { > - NdisMoveMemory(dest, tcp, sizeof(TCPHdr)); > - ipv6ExtLength = layers->l4Offset - layers->l3Offset - > sizeof(IPv6Hdr); > - *tcpPayloadLen = (ntohs(ipv6Hdr->payload_len) - ipv6ExtLength > - TCP_HDR_LEN(tcp)); > - return storage; > + tcp = OvsGetTcp(nbl, layers->l4Offset, dest); > + if (tcp == NULL) { > + return NULL; > } > + > + ipv6ExtLength = layers->l4Offset - layers->l3Offset - > sizeof(IPv6Hdr); > + *tcpPayloadLen = (ntohs(ipv6Hdr->payload_len) - ipv6ExtLength - > TCP_HDR_LEN(tcp)); > } else { > - ipHdr = NdisGetDataBuffer(NET_BUFFER_LIST_FIRST_NB(nbl), > - layers->l4Offset + sizeof(TCPHdr), > - NULL, 1 /*no align*/, 0); > + ipHdr = OvsGetIp(nbl, layers->l3Offset, &ipHdrStorage); > if (ipHdr == NULL) { > return NULL; > } > > - ipHdr = (IPHdr *)((PCHAR)ipHdr + layers->l3Offset); > - tcp = (TCPHdr *)((PCHAR)ipHdr + ipHdr->ihl * 4); > - > - if (tcp->doff * 4 >= sizeof *tcp) { > - NdisMoveMemory(dest, tcp, sizeof(TCPHdr)); > - *tcpPayloadLen = TCP_DATA_LENGTH(ipHdr, tcp); > - return storage; > + tcp = OvsGetTcp(nbl, layers->l4Offset, dest); > + if (tcp == NULL) { > + return NULL; > } > + > + *tcpPayloadLen = TCP_DATA_LENGTH(ipHdr, tcp); > } > - return NULL; > + > + return storage; > } > > static UINT8 > -- > 2.45.0 > > -- 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
