Re: [libvirt] [PATCH] nwfilter: increase pcap buffer size to be compatible with TPACKET_V3

On 04/26/2018 04:38 AM, Daniel P. Berrangé wrote: Okay, so I looked into this in more detail. I don't know that I'm totally at the bottom of it, but this is what I found in libpcap: * The function create_ring() calls setsockopt(fd, SOL_PACKET, PACKET_RX_RING, &req ...) to setup a ring buffer for receiving packets. * one of the attributes of req is tp_frame_size, and another is tp_frame_nr. tp_frame_nr is set to the user-supplied (or default) buffer size (for us it was 128k) / tp_frame_nr. * if TPACKETV3 isn't used, tp_frame_size is set to (roughly, there is some aligning done, and a bit additional added for one ethernet header) the user-supplied snaplen (in our case 576). So tp_frame_nr is going to be something > 200. * if TPACKET3 *is* used, tp_frame_size is set to a predefined constant MAXIMUM_SNAPLEN, which is 262144. This means that tp_frame_nr will be 131072/262144, which is 0. So the result will be that setsockopt() is called requesting a ring buffer that can read 0 frames. I didn't trace through the call to see exactly what happens, but it obviously can't be good. (Right after that, tp_frame_nr is also used in a call to malloc a buffer to hold pointers to frame headers, and that of course would also fail if we ever got that far. I'm assuming we don't. Why are the frames in the ring buffer so large for TPACKETV3? The code says this:    /* The "frames" for this are actually buffers that     * contain multiple variable-sized frames.     *     * We pick a "frame" size of 128K to leave enough     * room for at least one reasonably-sized packet     * in the "frame".     */ This all explains why changing the buffer size to 256k works for us - that gets tp_frame_nr to 1 (just barely!) so there is no multiplying by 0. And because multiple packets are received into this single "frame", we still operate correctly. However, this all seems very arbitrary - there's an API for setting the size of a buffer, but it's not defined anywhere in the API what are the units of measure (that's not an exactly accurate description, but I'm too tired to think of the right way to say it). pcap-int.h itself even admits this in the comment above its definition of MAXIMUM_SNAPLEN:    /*     * Maximum snapshot length.     *     * Somewhat arbitrary, but chosen to be:     *     * [blah blah blah details details...]     */     #define MAXIMUM_SNAPLEN       262144 Also, note that the comment where MAXIMUM_SNAPLEN is used in pcap-linux.c says that it is 128k, but the actual definition sets it at 256k. :-/ This gives me 0 confidence that any value we choose would continue to be viable in the future. Yeah, I'm guessing this is why Stefan chose to set the buffer size. In the case of static IP (the learnIPAddress codepath) we open the pcap socket, get a single packet that has an IP address, and then close the pcap socket, so we don't normally have more than a few around at any time, and the size of the buffer is a non-issue so it's left at the default. For the case of dhcp snooping, though, the handle remains open for the entire lifetime of the domain (I guess in case the address is released and re-requested and a different address is handed out the next time). So if we're using the default 2MB buffer, there would be 2MB for each client. On one hand, for a system with 100 clients, that's 200MB of memory. On the other hand, any system that's miniscule in relation to the total amount of memory (it might reduce the capacity of a very large host by one guest at most). (NB: I checked this out by putting VIR_WARNs in the source at strategic places, and found that we are actually keeping open *TWO* pcap sockets for each domain that has CTRL_IP_LEARNING=dhcp. We should probably look into that...) So if everyone is okay with that extra memory usage, I'd suggest Christian could send his original patch (deleting the pcap_set_buffer_size()) to the list (but including the comments from my patch about the exact error message, Resolves: blah.bugzilla.blah, etc). (Or if you don't have time, I can send it and credit you in the commit log). change the recommended format for some tools and I missed it? Who is "we"? :-) The overwhelming majority of these tags in libvirt commit logs have always been Resolves: laine@vhost2 ~/devel/libvirt (master*)>git log | grep "Resolves: h" | wc     508    1017   33340 laine@vhost2 ~/devel/libvirt (master*)>git log | grep "Fixes: h" | wc       9      18     654 I do notice that the few Fixes: are for bugs on launchpad (with one exception), and the Resolves: are all bugzilla.redhat.com and a few opensuse.org/suse.com