Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error in delta time calculation in pcap_sendqueue_transmit #580

Open
kayoub5 opened this issue Feb 2, 2022 · 9 comments
Open

Error in delta time calculation in pcap_sendqueue_transmit #580

kayoub5 opened this issue Feb 2, 2022 · 9 comments

Comments

@kayoub5
Copy link

kayoub5 commented Feb 2, 2022

Change introduced in 10d4de9#diff-fbde706694dbead2c023e9f5ba579b5d724a4a35c8831f03df023c9f0ee183c8R2620-R2623 are causing the microsecond part of the timestamp to be ignored.

(TimeFreq.QuadPart) / 1000000 will return zero if TimeFreq is less than 1000000 and inaccurate number if TimeFreq is not multiple of 1000000 (integer division), causing the whole microsecond part to be miscalculated.

As the original code indicates, the division should be the last operation, otherwise the delta time precision will be messed up.

This causes the entire pcap_sendqueue_transmit to miscalculate the delta time and breaking it completely in sync mode.

@kayoub5 kayoub5 changed the title Error in microsecond calculation in pcap_sendqueue_transmit Error in delta time calculation in pcap_sendqueue_transmit Feb 2, 2022
@marotc
Copy link

marotc commented Mar 2, 2022

It was (x * 1000000 + y) * q / 1000000 and now it is x * q + y * q / 1000000. The division is the last operation in both computation (following the left-to-right order of math operation in C). The only difference that I see is that the second one is less likely to overflow. Indeed, if y * q % 1000000 = r, we also have (x * 1000000 + y) * q % 1000000 = r, so the rounding is the same.

Did I missed something ?

@fyodor
Copy link
Member

fyodor commented Mar 7, 2022

Hi @kayoub5. The code does not look wrong to us, but we will add extra parenthesis just to be on the safe side. But the source of your problem may be something different. Can you provide more details on the symptoms you are seeing? Are you using Npcap Version 1.60? We will be happy to take a look.

@kayoub5
Copy link
Author

kayoub5 commented Mar 7, 2022

The problem we are facing is the accuracy of the function, Npcap 1.10 allowed an inter packet duration as low as 10-20us depending on the host.

npcap 1.60 can not handle any sub second gap.

the only related change I was able to see in the changelog was the sync change in 1.20, so I assumed that it is responsible.

@dmiller-nmap
Copy link
Contributor

ok, that makes a bit more sense. We did make some changes to try to avoid monopolizing the kernel for the duration of the packet send operation, so we'll start looking for issues there.

dmiller-nmap pushed a commit that referenced this issue Apr 22, 2022
Driver's BIOCSENDPACKETSSYNC handles inter-packet delays for as many
packets as can be sent in 1s, then returns. Packet.dll must do a
user-mode delay before sending the next packet. Previously, we did a
very busy wait; this change uses millisecond-resolution Sleep instead.
If delay is less than 1ms, we do not wait, trusting the system call
overhead to make the delay close enough.

Also changed calculations to use LONGLONG (64-bit int) data types and to
ensure we do not overflow nor lose precision. Similar changes will be
committed to the driver's sync code.
dmiller-nmap pushed a commit that referenced this issue Apr 22, 2022
Similar to earlier change to PacketSendPackets, this commit changes
calculation of inter-packet delay within the driver for
BIOCSENDPACKETSSYNC. We calculate the microsecond difference first,
considering that tv_usec can hold over an hour of time, then convert to
clock ticks in order to see how much time remains. If over 1s, control
is returned to user mode. Otherwise, and appropriate NDIS delay function
with microsecond resolution is called, eliminating the need to run our
own busy wait or use NdisWaitEvent (millisecond resolution).
@fyodor
Copy link
Member

fyodor commented Jun 27, 2022

Hi @kayoub5 (and anyone else who has experienced this). Are you able to test with the new Npcap Version 1.70. We made some changes in this area (see the commits referenced above and the changelog and so are you able to retest and let us know how it goes? We are leaving this issue open for now but will close if we don't hear back from anyone that the issue persists.

@kayoub5
Copy link
Author

kayoub5 commented Jul 26, 2022

Hi @fyodor,

We are still experiencing performance degradation, previous versions of Npcap were able to allow down to 20µs inter frame gap using pcap_sendqueue_transmit with Npcap we can't even get an accurate 1ms inter frame gap.

@dmiller-nmap
Copy link
Contributor

I've just pushed a change to move the call to KeQueryPerformanceCounter() closer to the point of sending the packets, which should improve accuracy. In the same commit (31cf7d6) I added some debug-mode assertions to prove whether the NDIS-provided functions we are using to introduce any needed delay are "sleeping" for an accurate amount of time.

In the meantime, @kayoub5 can you provide a DiagReport output and/or a detailed description of the system where this timing discrepancy is occurring? What means are you using to verify the timing? Is there a test we can use to get the same result, so we can confirm any fixes have addressed the issue?

@welbouri
Copy link

welbouri commented Feb 3, 2023

Hi @fyodor , we have done a comparison between WinPcap 4.1.3 and Npcap 1.72 using 2 machines with no installed antivirus and having the below characteristic:

  • Windows 10 64-bit operating system, x64-based processor
  • Processor : Intel(R) Core(TM) i5-7200U CPU @ 2.50GHz 2.70GHz
  • Installed RAM : 8.00 Go

In these tests, the packets were sent with a delay of 65µs : traces have been recorded in sender / receiver side.

  1. Use WinPcap when sending from an adapter to another in the same machine : (sender_winpcap_receiver_winpcap)
  • pcap recorded on sender's side shows that 65µs's delay is approximately respected.
  1. Use Ncap when sending from an adapter to another in the same machine : (sender_npcap_receiver_npcap)
  2. Use Winpcap in the first machine to send to another machine that uses Npcap (sender_winpcap_receiver_npcap)
  3. Use Npcap in the first machine to send to another machine that uses WinPcap (sender_npcap_receiver_winpcap)
  • delay was not respected in all other records

Do you have an explanation for this behavior in Npcap ?
Thanks in advance.
winpcapVSnpcap.zip

@kayoub5
Copy link
Author

kayoub5 commented Feb 3, 2023

@dmiller-nmap FYI, me and @welbouri are from the same team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants