Hi Wilson, Just wondering if you got my comments for this patch.
Alin. On Tue, Aug 6, 2024 at 1:57 AM Alin Serdean <[email protected]> wrote: > > > ------------------------------ > *From:* dev <[email protected]> on behalf of Wilson Peng > via dev <[email protected]> > *Sent:* Monday, August 5, 2024 10:55 AM > *To:* [email protected] <[email protected]> > *Subject:* [ovs-dev] [PATCH v2 1/1] datapath-windows: Merge split > dis-continuous net-buf. > > From: Wilson Peng <[email protected]> > > 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. > > In the fix it will supply the stack buffer to copy packet data when > call NdisGetDataBuffer(). > > In the conntrack Action process, it will do OvsPartialCopyNBL firstly > with the size of layers l7offsets. If the header is split the header > will be merged to one continuous buffer. > > But IPV6 traffic is not handed in this patch to merge the split > dis-continuous net-buf. > Reported-at: https://github.com/openvswitch/ovs-issues/issues/323 > > Signed-off-by: Wilson Peng <[email protected]> > --- > datapath-windows/ovsext/Actions.c | 8 +++++++ > datapath-windows/ovsext/BufferMgmt.c | 30 +++++++++++++------------- > datapath-windows/ovsext/Conntrack.c | 4 +++- > datapath-windows/ovsext/IpFragment.c | 8 +++++-- > datapath-windows/ovsext/PacketParser.h | 1 + > 5 files changed, 33 insertions(+), 18 deletions(-) > > diff --git a/datapath-windows/ovsext/Actions.c > b/datapath-windows/ovsext/Actions.c > index 97029b0f4..3a2086048 100644 > --- a/datapath-windows/ovsext/Actions.c > +++ b/datapath-windows/ovsext/Actions.c > @@ -2414,6 +2414,14 @@ OvsDoExecuteActions(POVS_SWITCH_CONTEXT > switchContext, > } > > PNET_BUFFER_LIST oldNbl = ovsFwdCtx.curNbl; > + if (layers->value != 0 && layers->isIPv4 && layers->l7Offset > != 0) { > + PUINT8 bufferStart = NULL; > + bufferStart = OvsGetHeaderBySize(&ovsFwdCtx, > layers->l7Offset); > + if (!bufferStart) { > + dropReason = L"OVS-Netbuf reallocated failed"; > + goto dropit; > + } > + } > [Alin] If we are reallocating/cloning the net buffer list here, the > headers are contiguous... In that case, why do we still need to update > OvsGetTcpHeader ? > It would make more sense just to ensure the headers are contiguous when we > want to use them and clone the NBL as the last resort. > status = OvsExecuteConntrackAction(&ovsFwdCtx, key, > (const PNL_ATTR)a); > if (status != NDIS_STATUS_SUCCESS) { > diff --git a/datapath-windows/ovsext/BufferMgmt.c > b/datapath-windows/ovsext/BufferMgmt.c > index 5c52757a0..9a7f713ce 100644 > --- a/datapath-windows/ovsext/BufferMgmt.c > +++ b/datapath-windows/ovsext/BufferMgmt.c > @@ -1106,24 +1106,24 @@ GetIpHeaderInfo(PNET_BUFFER_LIST curNbl, > const POVS_PACKET_HDR_INFO hdrInfo, > UINT32 *hdrSize) > { > - EthHdr *eth; > - IPHdr *ipHdr; > - IPv6Hdr *ipv6Hdr; > - PNET_BUFFER curNb; > - > - curNb = NET_BUFFER_LIST_FIRST_NB(curNbl); > - ASSERT(NET_BUFFER_NEXT_NB(curNb) == NULL); > - eth = (EthHdr *)NdisGetDataBuffer(curNb, > - hdrInfo->l4Offset, > - NULL, 1, 0); > - if (eth == NULL) { > - return NDIS_STATUS_INVALID_PACKET; > - } > - > if (hdrInfo->isIPv6) { > - ipv6Hdr = (IPv6Hdr *)((PCHAR)eth + hdrInfo->l3Offset); > *hdrSize = (UINT32)(hdrInfo->l4Offset); > } else { > + EthHdr *eth; > + IPHdr *ipHdr; > + CHAR tempBuf[MAX_IPV4_PKT_HDR_LEN]; > + PNET_BUFFER curNb; > + > + curNb = NET_BUFFER_LIST_FIRST_NB(curNbl); > + ASSERT(NET_BUFFER_NEXT_NB(curNb) == NULL); > + > + NdisZeroMemory(tempBuf, MAX_IPV4_PKT_HDR_LEN); > + eth = (EthHdr *)NdisGetDataBuffer(curNb, > + hdrInfo->l4Offset, > + (PVOID)tempBuf, 1, 0); > + if (eth == NULL) { > + return NDIS_STATUS_INVALID_PACKET; > + } > ipHdr = (IPHdr *)((PCHAR)eth + hdrInfo->l3Offset); > *hdrSize = (UINT32)(hdrInfo->l3Offset + (ipHdr->ihl * 4)); > } > diff --git a/datapath-windows/ovsext/Conntrack.c > b/datapath-windows/ovsext/Conntrack.c > index 39ba5cc10..608712bf0 100644 > --- a/datapath-windows/ovsext/Conntrack.c > +++ b/datapath-windows/ovsext/Conntrack.c > @@ -683,6 +683,7 @@ OvsGetTcpHeader(PNET_BUFFER_LIST nbl, > TCPHdr *tcp; > VOID *dest = storage; > uint16_t ipv6ExtLength = 0; > + CHAR tempBuf[MAX_IPV4_PKT_HDR_LEN]; > > if (layers->isIPv6) { > ipv6Hdr = NdisGetDataBuffer(NET_BUFFER_LIST_FIRST_NB(nbl), > @@ -701,9 +702,10 @@ OvsGetTcpHeader(PNET_BUFFER_LIST nbl, > return storage; > } > } else { > + NdisZeroMemory(tempBuf, MAX_IPV4_PKT_HDR_LEN); > ipHdr = NdisGetDataBuffer(NET_BUFFER_LIST_FIRST_NB(nbl), > layers->l4Offset + sizeof(TCPHdr), > - NULL, 1 /*no align*/, 0); > + (PVOID)tempBuf, 1 /*no align*/, 0); > if (ipHdr == NULL) { > return NULL; > } > diff --git a/datapath-windows/ovsext/IpFragment.c > b/datapath-windows/ovsext/IpFragment.c > index afb8e50d6..18d806a69 100644 > --- a/datapath-windows/ovsext/IpFragment.c > +++ b/datapath-windows/ovsext/IpFragment.c > @@ -153,12 +153,14 @@ OvsIpv4Reassemble(POVS_SWITCH_CONTEXT switchContext, > PNET_BUFFER_LIST newNbl = NULL; > UINT16 ipHdrLen, packetHeader; > UINT32 packetLen; > + CHAR tempBuf[MAX_IPV4_PKT_HDR_LEN]; > > curNb = NET_BUFFER_LIST_FIRST_NB(*curNbl); > ASSERT(NET_BUFFER_NEXT_NB(curNb) == NULL); > > + NdisZeroMemory(tempBuf, MAX_IPV4_PKT_HDR_LEN); > eth = (EthHdr*)NdisGetDataBuffer(curNb, layers->l4Offset, > - NULL, 1, 0); > + (PVOID)tempBuf, 1, 0); > if (eth == NULL) { > return NDIS_STATUS_INVALID_PACKET; > } > @@ -253,12 +255,14 @@ OvsProcessIpv4Fragment(POVS_SWITCH_CONTEXT > switchContext, > POVS_IPFRAG_ENTRY entry; > POVS_FRAGMENT_LIST fragStorage; > LOCK_STATE_EX htLockState; > + CHAR tempBuf[MAX_IPV4_PKT_HDR_LEN]; > > curNb = NET_BUFFER_LIST_FIRST_NB(*curNbl); > ASSERT(NET_BUFFER_NEXT_NB(curNb) == NULL); > > + NdisZeroMemory(tempBuf, MAX_IPV4_PKT_HDR_LEN); > eth = (EthHdr*)NdisGetDataBuffer(curNb, layers->l4Offset, > - NULL, 1, 0); > + (PVOID)tempBuf, 1, 0); > if (eth == NULL) { > return NDIS_STATUS_INVALID_PACKET; > } > diff --git a/datapath-windows/ovsext/PacketParser.h > b/datapath-windows/ovsext/PacketParser.h > index 0d5c0a6cb..23eea9440 100644 > --- a/datapath-windows/ovsext/PacketParser.h > +++ b/datapath-windows/ovsext/PacketParser.h > @@ -18,6 +18,7 @@ > #define __PACKET_PARSER_H_ 1 > > #define MIN_IPV4_HLEN 20 > +#define MAX_IPV4_PKT_HDR_LEN (ETH_MAX_HEADER_LEN + MAX_IPV4_HLEN) > > #include "precomp.h" > #include "NetProto.h" > -- > 2.39.2 (Apple Git-143) > [[Alin] As you said this would fix only IPv4. Let's aim to fix both IPv4 > and IPv6. > > _______________________________________________ > 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
