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

The Npcap API documentation mentions an apparently non-existent pcap_setuserbuffer() routine #1327

Open
guyharris opened this Issue Sep 17, 2018 · 8 comments

Comments

Projects
None yet
2 participants
@guyharris

guyharris commented Sep 17, 2018

The Npcap API documentation mentions a pcap_setuserbuffer() routine; that appears to be present in WinPcap 4.1.3, but doesn't appear to be in tip of the master branch of the Npcap Git repository. Was it removed?

(If pcap_create(), pcap_set_buffer_size(), and pcap_activate() are used, the user-mode buffer is large enough to handle that kernel buffer size, so pcap_setuserbuffer() is not necessary; I'm not sure whether pcap_setbuff() and pcap_setuserbuffer() need to be used together, but, whether it is or isn't, pcap_set_buffer_size() is what should be used in any code written for libpcap 1.0 or later and for Npcap.)

@dmiller-nmap

This comment has been minimized.

Show comment
Hide comment
@dmiller-nmap

dmiller-nmap Sep 17, 2018

It was upstreamed to the-tcpdump-group/libpcap. I agree that pcap_set_buffer_size could be overloaded to set this userspace buffer size (p->bufsize), too, but it doesn't do so right now. So the documentation currently reflects the state of things as of today (Npcap based on libpcap 1.8.1).

dmiller-nmap commented Sep 17, 2018

It was upstreamed to the-tcpdump-group/libpcap. I agree that pcap_set_buffer_size could be overloaded to set this userspace buffer size (p->bufsize), too, but it doesn't do so right now. So the documentation currently reflects the state of things as of today (Npcap based on libpcap 1.8.1).

@guyharris

This comment has been minimized.

Show comment
Hide comment
@guyharris

guyharris Sep 17, 2018

I agree that pcap_set_buffer_size could be overloaded to set this userspace buffer size (p->bufsize), too, but it doesn't do so right now.

That's not overloading it, that's fixing it. I'm not even sure what the point of having the userspace buffer size being different from the kernel buffer size is.

guyharris commented Sep 17, 2018

I agree that pcap_set_buffer_size could be overloaded to set this userspace buffer size (p->bufsize), too, but it doesn't do so right now.

That's not overloading it, that's fixing it. I'm not even sure what the point of having the userspace buffer size being different from the kernel buffer size is.

@guyharris

This comment has been minimized.

Show comment
Hide comment
@guyharris

guyharris Sep 17, 2018

It was upstreamed to the-tcpdump-group/libpcap.

"It" and "upstreamed" in what sense?

guyharris commented Sep 17, 2018

It was upstreamed to the-tcpdump-group/libpcap.

"It" and "upstreamed" in what sense?

@guyharris

This comment has been minimized.

Show comment
Hide comment
@guyharris

guyharris Sep 17, 2018

Unless you can think of a compelling reason why the userspace buffer size shouldn't start out as being the same as the kernel buffer size (in which case, please list it here), please do upstream that issue by filing either an issue or a pull request against libpcap.

guyharris commented Sep 17, 2018

Unless you can think of a compelling reason why the userspace buffer size shouldn't start out as being the same as the kernel buffer size (in which case, please list it here), please do upstream that issue by filing either an issue or a pull request against libpcap.

@guyharris

This comment has been minimized.

Show comment
Hide comment
@guyharris

guyharris Sep 17, 2018

And, given that WinPcap has pcap_setuserbuffer(), I will be adding it as a Windows-only routine to libpcap.

guyharris commented Sep 17, 2018

And, given that WinPcap has pcap_setuserbuffer(), I will be adding it as a Windows-only routine to libpcap.

@guyharris

This comment has been minimized.

Show comment
Hide comment
@guyharris

guyharris Sep 17, 2018

And, given that WinPcap has pcap_setuserbuffer(), I will be adding it as a Windows-only routine to libpcap.

And, given that libpcap already had pcap_setuserbuffer() as a Windows-only routine, I obviously won't be adding it as a Windows-only routine to libpcap. :-)

(Is that what you mean by "upstreamed"? It was added because it already was in WinPcap 4.1.3, and possibly earlier, even if they didn't document it.)

guyharris commented Sep 17, 2018

And, given that WinPcap has pcap_setuserbuffer(), I will be adding it as a Windows-only routine to libpcap.

And, given that libpcap already had pcap_setuserbuffer() as a Windows-only routine, I obviously won't be adding it as a Windows-only routine to libpcap. :-)

(Is that what you mean by "upstreamed"? It was added because it already was in WinPcap 4.1.3, and possibly earlier, even if they didn't document it.)

@dmiller-nmap

This comment has been minimized.

Show comment
Hide comment
@dmiller-nmap

dmiller-nmap Sep 26, 2018

Yes, that's what I meant; libpcap 1.8.1 already includes pcap_setuserbuffer() on Windows. I haven't done any performance testing, but it seems like there shouldn't be any reason for the user buffer to be different than the kernel buffer. After all, user memory is "cheaper" than kernel memory. The only reason I can see is if you know your utilization will be low (only a few packets received between calls to read_op), and that doesn't seem like a predictable thing. Even the old WinPcap documentation for Packet library (doxygen comments not rendered in the SDK) says that it's preferable to use a large user buffer so as to limit the number of needed calls to the kernel.

So my recommendation would be:

  1. libpcap: fix the sizes together in pcap_set_buffer_size and make the defaults the same, too. Currently, WIN32_DEFAULT_USER_BUFFER_SIZE is 256,000 bytes and WIN32_DEFAULT_KERNEL_BUFFER_SIZE is 1,000,000 bytes.
  2. Npcap: document that pcap_set_buffer_size is the new-API alternative to pcap_setbuff, but pcap_setuserbuffer is still needed. If and when libpcap releases with the sizes fixed together, we will update the documentation to indicate that pcap_setuserbuffer is unnecessary for most cases.
  3. Nmap: update Nmap to use pcap_setuserbuffer(1000000) to improve performance.

Does this sound appropriate?

dmiller-nmap commented Sep 26, 2018

Yes, that's what I meant; libpcap 1.8.1 already includes pcap_setuserbuffer() on Windows. I haven't done any performance testing, but it seems like there shouldn't be any reason for the user buffer to be different than the kernel buffer. After all, user memory is "cheaper" than kernel memory. The only reason I can see is if you know your utilization will be low (only a few packets received between calls to read_op), and that doesn't seem like a predictable thing. Even the old WinPcap documentation for Packet library (doxygen comments not rendered in the SDK) says that it's preferable to use a large user buffer so as to limit the number of needed calls to the kernel.

So my recommendation would be:

  1. libpcap: fix the sizes together in pcap_set_buffer_size and make the defaults the same, too. Currently, WIN32_DEFAULT_USER_BUFFER_SIZE is 256,000 bytes and WIN32_DEFAULT_KERNEL_BUFFER_SIZE is 1,000,000 bytes.
  2. Npcap: document that pcap_set_buffer_size is the new-API alternative to pcap_setbuff, but pcap_setuserbuffer is still needed. If and when libpcap releases with the sizes fixed together, we will update the documentation to indicate that pcap_setuserbuffer is unnecessary for most cases.
  3. Nmap: update Nmap to use pcap_setuserbuffer(1000000) to improve performance.

Does this sound appropriate?

@guyharris

This comment has been minimized.

Show comment
Hide comment
@guyharris

guyharris Sep 28, 2018

I'm not even sure what the point of having the userspace buffer size being different from the kernel buffer size is.

That's because I was thinking of BPF (as in "the capture mechanism in *BSD, macOS, Solaris 11, and AIX", not as in "the filtering mechanism"), which has two buffers, with packets added to one buffer - the "store buffer" - and read from the other - the "hold buffer" - with the buffers swapped if the store buffer is full and the hold buffer is empty.

The only size that can be specified with BPF is the size of the buffers (they're the same size), and a read from the hold buffer returns the entire buffer, so there's no point in the user buffer being any size other than the kernel buffer size.

This also means that a wakeup is delivered when the full kernel buffer is, in effect, half-full, i.e. when the store buffer fills up and is rotated to become the hold buffer. (It's also delivered when the timeout expires.)

NPF, at least as I read the documentation and read^Wskim the code, has a single buffer, with the writer adding packets to the tail and the reader consuming packets from the head. It has two kernel sizes - the buffer size and the "minimum to copy" size. Unless the "minimum to copy" size is sufficiently smaller than the buffer size, with a sufficiently high rate of packet data being captured there's probably a significant risk of the wakeup occurring when there's insufficient time to drain the packet buffer before it overflows.

In this model, the appropriate size for the user buffer is probably something between the "minimum to copy" size and the kernel buffer size. If it's just the "minimum to copy" size, you run the risk that you're not getting all the available packets once you wake up - packets that arrive after the wakeup won't be seen. If it's the kernel buffer size, you may, in practice, be wasting memory, as the buffer probably will rarely, if ever, be completely full when you do the read (if it is completely full, there's probably a good chance that you're dropping packets).

guyharris commented Sep 28, 2018

I'm not even sure what the point of having the userspace buffer size being different from the kernel buffer size is.

That's because I was thinking of BPF (as in "the capture mechanism in *BSD, macOS, Solaris 11, and AIX", not as in "the filtering mechanism"), which has two buffers, with packets added to one buffer - the "store buffer" - and read from the other - the "hold buffer" - with the buffers swapped if the store buffer is full and the hold buffer is empty.

The only size that can be specified with BPF is the size of the buffers (they're the same size), and a read from the hold buffer returns the entire buffer, so there's no point in the user buffer being any size other than the kernel buffer size.

This also means that a wakeup is delivered when the full kernel buffer is, in effect, half-full, i.e. when the store buffer fills up and is rotated to become the hold buffer. (It's also delivered when the timeout expires.)

NPF, at least as I read the documentation and read^Wskim the code, has a single buffer, with the writer adding packets to the tail and the reader consuming packets from the head. It has two kernel sizes - the buffer size and the "minimum to copy" size. Unless the "minimum to copy" size is sufficiently smaller than the buffer size, with a sufficiently high rate of packet data being captured there's probably a significant risk of the wakeup occurring when there's insufficient time to drain the packet buffer before it overflows.

In this model, the appropriate size for the user buffer is probably something between the "minimum to copy" size and the kernel buffer size. If it's just the "minimum to copy" size, you run the risk that you're not getting all the available packets once you wake up - packets that arrive after the wakeup won't be seen. If it's the kernel buffer size, you may, in practice, be wasting memory, as the buffer probably will rarely, if ever, be completely full when you do the read (if it is completely full, there's probably a good chance that you're dropping packets).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment