Control: severity -1 grave
On 08/02/15 03:19, Tomasz Buchert wrote:
> [...]
Hi,
I've analyzed my problem and found what follows.
ANALYSIS
=========
First, only the QUERY packets are missing in dnstop, the RESPONSE
packets show (mostly) fine with dns -Q -R <interface>. I say "mostly"
because they can be still lost if there is too much traffic coming to
the card.
Second, the use of "pcap_fileno" is incorrect, it seems that
"pcap_get_selectable_fd" should be used instead. I'm not sure it
changes anything in practice, though.
Third, "pcap_select" in the code is called with 1s of timeout: for
reasons that are unclear to me, this makes the pcap library drop the
packets randomly (verified with "pcap_stats") and these drops are the
reason for this bug. Strangely, making the timeout smaller (say 50ms)
makes the problem go away. Removing the use of "pcap_select"
altogether works as well, however this causes dnstop to eat 100% of
ther CPU due to "pcap_setnonblock".
SOLUTIONS
===========
(1) Changing the "pcap_select" timeout to something like 50ms works
for me, but this is hardly a real solution.
(2) Removing "pcap_setnonblock" and "pcap_select" from the code solves
the problem as well. In this case we should probably also increase
"to_ms" in "pcap_open_live" to something bigger than 1ms (I've set
it to 50ms - a tolerable time for an interface to freeze).
(3) Increasing the buffer size for the capture could work as well, but
I haven't tried it.
===========
My preferred solution is (2) and I attach a proof-of-concept
patch. The reason for having (1) in the upstream is a support for
MacOSX which we don't really care about in Debian. However, we have
non-Linux ports (FreeBSD officialy and hurd) and I have no idea
whether (2) will work for them. For example, while researching this,
I found that there is no promise that "pcap_dispatch" will respect the
timeout given in "pcap_open_live" (it may actually block).
Personally I think this bug is RC (the package does not work
out of the box) and I'm bumping the severity.
Cheers,
Tomasz
From: Tomasz Buchert <[email protected]>
Date: Sun, 15 Feb 2015 20:20:54 +0100
Subject: proof-of-concept patch
---
dnstop.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/dnstop.c b/dnstop.c
index 57e12cc..e4b8f6d 100644
--- a/dnstop.c
+++ b/dnstop.c
@@ -1901,7 +1901,7 @@ main(int argc, char *argv[])
if (readfile_state) {
pcap = pcap_open_offline(device, errbuf);
} else {
- pcap = pcap_open_live(device, PCAP_SNAPLEN, promisc_flag, 1, errbuf);
+ pcap = pcap_open_live(device, PCAP_SNAPLEN, promisc_flag, 50, errbuf);
}
if (NULL == pcap) {
fprintf(stderr, "pcap_open_*: %s\n", errbuf);
@@ -1934,7 +1934,7 @@ main(int argc, char *argv[])
* workaround and dnstop does not require non-blocking, we'll won't
* check the return status.
*/
- pcap_setnonblock(pcap, 1, errbuf);
+ /* pcap_setnonblock(pcap, 1, errbuf); */
switch (pcap_datalink(pcap)) {
case DLT_EN10MB:
handle_datalink = handle_ether;
@@ -1993,8 +1993,8 @@ main(int argc, char *argv[])
* packets to process. Thus, we always ignore its return value
* and just call pcap_dispatch() anyway.
*/
- if (0 == readfile_state) /* interactive */
- pcap_select(pcap, 1, 0);
+ /* if (0 == readfile_state)
+ pcap_select(pcap, 1, 0); */
x = pcap_dispatch(pcap, 50, handle_pcap, NULL);
}
if (0 == x && 1 == readfile_state) {
