Re: [PATCH] Adjust Packet's MDL when stripping ETHERNET_TAG from headers

[Owen Smith <owen.smith@xxxxxxxxxx>]

Packet->Mdl.MappedSystemVa = (PUCHAR)Packet->Mdl.MappedSystemVa + sizeof(ETHERNET_TAG);

might have been a cleaner statement of what the MDL manipulation is actually doing

Owen

On Tue, Mar 5, 2024 at 9:56 AM Owen Smith <owen.smith@xxxxxxxxxx> wrote:

The addition of the TAG size then subtraction of the offset was confusing - setting the BaseVa to MappedSystemVa then adjusting it clarified the intent to adjust the MappedSystemVa (a cleaner approach would have been to adjust the Mdl->MappedSystemVa += sizeof(ETHERNET_TAG); but this would require significant casting to function correctly).

> +    Packet->Mdl.ByteOffset += sizeof (ETHERNET_TAG);
> +    Packet->Mdl.ByteCount -= sizeof (ETHERNET_TAG);

is the only required change.

Recalculating EthernetHeader is not required, as its not used again in that function.

Owen

On Tue, Mar 5, 2024 at 9:43 AM Paul Durrant <xadimgnik@xxxxxxxxx> wrote:

On 04/03/2024 07:11, Owen Smith wrote:
> Tagged headers have the ETHERNET_TAG stripped to convert to
> ETHERNET_UNTAGGED_HEADERs. As part of the fix up, the offsets for each
> header was adjusted, and the underlying MDL's MappedSystemVa was adjusted.
> However, the ByteCount and ByteOffset were not adjusted, resulting in
> instances of data corruption when MappedSystemVa != StartVa + ByteOffset.
>
> It is possible to configure the default gateway to respond to ARP packets
> with TPID wrappings. Openvswitch will not strip this header since this commit:
>
>      commit f0fb825a3785320430686834741c718ff4f8ebf4
>      Author: Eric Garver <e@xxxxxxx>
>      Date:   Wed Mar 1 17:47:59 2017 -0500
>
>          Add support for 802.1ad (QinQ tunneling)
>
> Which means Windows was receiving corrupted ARP responses, leading to an
> inability to access the network correctly. This would usually show as
> Windows indicating that no internet access was available, or being unable
> it access other VMs or other resources beyond the default gateway.
>
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxx>
> ---
>   src/xenvif/receiver.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/src/xenvif/receiver.c b/src/xenvif/receiver.c
> index 0ce5b67..720d1d1 100644
> --- a/src/xenvif/receiver.c
> +++ b/src/xenvif/receiver.c
> @@ -436,16 +436,23 @@ ReceiverRingProcessTag(
>   
>       Packet->TagControlInformation = NTOHS(EthernetHeader->Tagged.Tag.ControlInformation);
>   
> +    // Convert ETHERNET_TAGGED_HEADER into ETHERNET_UNTAGGED_HEADER
>       Offset = FIELD_OFFSET(ETHERNET_TAGGED_HEADER, Tag);
>       RtlMoveMemory((PUCHAR)EthernetHeader + sizeof (ETHERNET_TAG),
>                     (PUCHAR)EthernetHeader,
>                     Offset);
>   
>       // Fix up the packet information
> +    BaseVa = Packet->Mdl.MappedSystemVa;
> +    ASSERT(BaseVa != NULL);
> +
>       BaseVa += sizeof (ETHERNET_TAG);
>   
> -    BaseVa -= Packet->Offset;
>       Packet->Mdl.MappedSystemVa = BaseVa;

You only need...

> +    Packet->Mdl.ByteOffset += sizeof (ETHERNET_TAG);
> +    Packet->Mdl.ByteCount -= sizeof (ETHERNET_TAG);
> +
> +    ASSERT3P((PUCHAR)Packet->Mdl.StartVa + Packet->Mdl.ByteOffset, ==, Packet->Mdl.MappedSystemVa);
>   

these lines to achieve the aim of the patch AFAICT. Why mess with BaseVa?

   Paul

>       Packet->Length -= sizeof (ETHERNET_TAG);
>   
> @@ -468,10 +475,6 @@ ReceiverRingProcessTag(
>   
>       Info->Length -= sizeof (ETHERNET_TAG);
>   
> -    BaseVa += Packet->Offset;
> -
> -    EthernetHeader = (PETHERNET_HEADER)(BaseVa + Info->EthernetHeader.Offset);
> -
>       ASSERT3U(PayloadLength, ==, Packet->Length - Info->Length);
>   }
>