Memory issue during packet release #119
Comments
|
I suspect large frames being involved in this memory issue. |
|
If extra anno makes the packet bigger than the DPDK buffer size, this may be a problem. But it is still a bug... |
|
Frame with len 2420 bytes causes the crash in this trace. |
|
Do you use your force length option? What is the type of the packet out of FromDump? Still a mmaped buffer, an "expensively pushed" buffer that will have a non-pool buffer? |
|
Interestingly enough, if I don't use FORCE_LEN (i.e., FORCE_LEN=-1), I uncovered another bug. set_mac_header(p->mac_header() ? data() + p->mac_header_offset() : 0); because p=0 See the stack trace below: click: ../include/click/packet.hh:2118: void Packet::set_mac_header(const unsigned char*): Assertion `p >= buffer() && p <= end_buffer()' failed. 0 0x00007ffff5ed0428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54 I have a fix for this already |
|
With FORCE_LEN=0 (which crafts the packets with their real length), the trace is injected without errors if I intentionally kill packets longer than 2048 bytes. |
This is a temporary solution to the memory issue tbarbette#119 Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com>
This is known and is related to the way click is build, you need a MarkMACHeader after the FromDump. It's the same for IP elements that do not have a CheckIPHeader or MarkIPHeader before them. You need to parse the layer you're working on. I don't think a check should be added in the code. Maybe something static (do I have an element that sets the annotation before me?) but not in the fast path. |
|
max_buffer_length is not really true. Normally when you call make() for a packet too long for the buffer pool, it will do a malloc and will not use the buffer pool. This is not the problem... |
|
Yes, I am digging into it now. The copy operation fails and the packet is getting killed, but it seems that no reference exists. Will be back soon.. |
This is a temporary solution to the memory issue #119 Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com>
* proclikefs: remove unused symbol Moreover it clash with linux/fs.h definition * ToDevice(.k) : Set queue_mapping to 0 Somehow, even with only one queue in RX this field is dangling. Fix tbarbette#359. In ixgbe and i40e that lead to accessing an inexisting queue. Proper support would be to initialize the device with multiple queues, or us multiple ToDevice with a QUEUE argument and use one queue per thread. But that's for another day... * Make KernelTun batch-compatible * GTPTable : verbose argument * Make ICMPRewriter batch-compatible * Fix KernelTun * GTPLookup: Allow to disable checksum * LinkUnqueue : allow no bandwidth * FromDPDKDevice: Allow to change MTU * Small fixes * Sync style and few not important things with Metron * Fix last commit * Include DPDK in the cc and not the hh Fix isse with --enable-dpdk-pool because of a dependency loop * Whoopsie * Fix error in messages A bad merge, probably... This will show clearer the problem when there are not enough queues to serve a TX DPDK device. In this case we must rely on locking, but I don't want to start locking automatically. The user may augment the number of available queues with virtual devices. Should fix tbarbette#71 * Add PRWMP vs RWMP * Separate LockMP and RWMP * CounterTest: allow to pass some read * Add PRWMP vs RWMP * Separate LockMP and RWMP * CounterTest: allow to pass some read * More locks * Add FORCE_SHRINK parameter to the EnsureHeadroom element * Inline method in RecordTimestamp's fast path This patch gives a slight performance boost Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com> * Allow FromDump to set a fixed frame size This patch allows FromDump elements to generate packets from pcap traces while being able to enforce a fixed frame size. This configuration could be handy when you want to use the very same Click code to generate either arbitrary frame sizes from a trace or fixed frame sizes from the same trace. Default behaviour is unchanged. Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com> * Do not print delayed packets if requested User can select to supress warnings about delayed packets. Default behaviour remains as before Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com> k * Fix --enable-dpdk-packet In theory... * dpdk: Warn about broken support for mlx5 * Support DPDK 18.05 * Fixed typos in HashTable docs Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com> * Import changes from Metron regarding DPDK, support up to 18.05 Some changes were made by Georgios Katsikas Signed-off-by: Tom Barbette <tom.barbette@ulg.ac.be> * Update README.md * Link wiki in README.md * click/etherswitch: Forward traffic to a set of ports The default behavior of etherswitch is to forward traffic to all ports except the source when broadcast or to the destination port found in lookup of etherswitch's mac table. This commit expands the functionality of etherswitch to send packet(s) out port(s) iff the port to forward traffic is enabled. Thus, etherswitch can selectively forward traffic to particular ports, a feature proving useful with broadcast traffic so as to avoid traffic loops between click and other external hardware (e.g. switch). Ports can selectively be removed and the port forwarding map can be reset via write handlers "remove_port_forwarding" and "reset_port_forwarding". The arguments to "remove_port_forwarding" write handler is a list of numbers with the first one being a source port and the others being ports to remove. For example, "1 2 3 4, 2 3 4" means port 1 will not forward traffic to ports 2, 3, and 4 -- in addition port 2 will not forward traffic to ports 3 and 4. No arguments are necessary for "reset_port_forwarding" write handler. Also, the port forwarding map can be viewed via read handler "port_forwarding". * Queue do not spawn thread, their puller do * Configuration for the SuperFluidity testbed * GTP fixes * KernelTunMP Add the KernelTunMP element that support multi-thread read from a TUN/TAP and multi-thread write to a TUN/TAP Thread safeness for writing is ensure with the usual automatic thread traversal detection of FastClick. Multi-thread read is done using the THREADS argument. Eg "THREADS 1-3" will use threads 1 to 3 to read packets. Home thread such as set with StaticThreadSched will be ignored ! Seems to work but needs more testing * Move GTP examples to conf/gtp * Contribution rules * Fix categories * URL update * Fix warning in timestamp * Doc for GTP elements * Fix documentation and style issues * More warnings... * Enable batch only if --enable-userlevel is provided * Fix Kernel compilation up to at least 4.4 And move things around to remove things uncompatible with kernel mode. Among them batching. But it is not a technical problem, it justs needs a little refactoring to work in kernel. Anyway, as mainline Click, the kernel mode is not working very well with recent versions, that should be fixed before batching. There may be some unfixed platform dependencies such as ARM, don't hesitate to fix them too. * Fix userlevel I moved some things a little too fast... * Fix test dependencies too * Fix the research package * Preliminary support for DPDK 18.08 This patch is based on DPDK 18.08-rc1 Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com> * Added MLX5 dependency on libmnl This patch adds support for 18.08-rc2. Also, rebased the branch based on latest master changes Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com> * Fix DPDK 18.08 - Mask RSS flags that are not supported - Use hardware default ring size if provided, or DPDKDevice's default if not * Fix compilation with older DPDK versions * Update README.md * Fix compilation with DPDK 18.02 That option was added in 18.05 * Reduce the amount of Travis tests Let's test all DPDK version with batching, but only a few without batching as those features do not influence each other much. * Travis: Compile with machine=default Some travis machines are older, and do not support some instruction set * DPDK: Allow to set PAUSE mode Argument is either NONE, RX, TX or FULL * DPDK: Types and warning fixes * Remove unused variable * Avoid clang override warning as of now Waayyy to many to fix * TimestampDiff : Allow to print delay since a certain index * RecordTimestamp : Add the avg alias * RecordTimestamp: Fix testie On travis, the latency can be very long * TimestampDiff bugfix To compute the average, the sum must be divided by the total - index * click: Add --simtick options Allows to change the amount of tick to warp in simtime mode This allows to force us scale to enable some testing * IPRewriterMap : Very small doc fix * dpdk: Compiling with the default is not an export In the hope to fix travis (finally ?) * Travis: Last supported is 18.08, let's use it * Timewarp test: relax constraint and more robust The timewrap test fails from time to time on travis, which is very slow. The time to launch the process makes the delay longer than expected, so let's relax the constraints. In addition, substract from the real time the time it takes to launch an empty click. This allows to be less error-proned regarding the time a process takes to execute, eg on travis. * Typo * Allow to list pools in handlers * Fix building with shared libs * Pipeliner: Fix message Without consequence, but it did not display information for some threads * Batching helpers Actually vanilla helpers too... * PaintRR element See hh for description. It is also the first example of ClassifyElement. * Update PR per Georgios Katsikas comments * dpdk: Allow to read rx_nombufs stat * Style fixes in fromdpdkdevice.cc Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com> * TimestampDiff: do not compute unneded informations * Make Pad element batch-compatible Also fix the SimpleElement helper * Remove some warnings Various styling, unused stuffs, integer comparison, ... * Repair click-devirtualize and click-makepackage Should fix issue tbarbette#37. Regarding DPDK : - Export needed includes to the tools Regarding makepackage : - smaction is reserved as a inline Packet*. Without nowing that we did some "inline void" smaction, for read-only smaction. So I renamed all of those smaction as rmaction (for read simple action). Regarding click-devirtualize: - Added support for push_batch * Reorganise conf folder Finally clean all that shit per main topics of interest. The README provides some generic explanations. If people read this, it would be great to add : - L2 switch example - More self-contained "tutorial" examples. Probably userlevel with KernelTAP that works nearly always and allow the user to play. * Add tutorial examples * Reference wiki and push to the wiki dev info * Commiy changes generated from the configure.in * Fix DPDK 17.11 * Fix last commit for DPDK > 17.11 with Octeon * If using DPDK, allocate memory from DPDK mem Allows to simply use huge pages and cache alignment (mem is allocated on boundary, and headroom is changed to be a multiple of cacheline size) * Batching support for VLAN elements Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com> * Batching support for EnsureEther Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com> * EtherEncap batching helper Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com> * ReplayUnqueue : Allow to replay according to ts If the TIMING parameter is > 0, ReplayUnqueue will replay according to the packet timestamp. TIMING is an integer and if bigger than 1 will define the acceleration speed. A value of 2 will replay packets twice faster, ... * Fix first packet never being sent Timestamp of diff packets must start with 0, so first packet is always sent and then the difference is computed from that packet * Replay: Make the timing difference over the whole replay Problem is that it was not able to replay at 100G because Click would spend all its time in the clock function. Using the diff over the whole replay time is also much more precise than waiting for the interframe * Reset time when restarting from active handler * RecordTimestamp: Differentiate double read from uninitialized * dpdk: Allow to set number of mbufs per pool * Replay: No need for inlining here * Support for any percentiles * TIMING is only supported by ReplayUnqueue * Small fixes as per Georgios Katsikas comments Also, return 0 if someone asks for the latency of the current index * Use Click parser and fix test The replay start at the time of the first packet now, always * click/lexer: fix double free when aliasess are created for elementclasses In the Lexer, when an elementclass is encountered, the parser saves state and begins parsing the inner configuration. The ParserState is pushed onto a stack and a Compound is attached as a thunk. When you exit the elementclass, the ParserState is popped and the Compound is destroyed. However, when an elementclass is aliased, a synonym references the Compound by pointer. When the synonym is destroyed, it also deletes the thunk; this causes a double free EVERY time the Lexer encounters an aliased elementclass. In kclick, this manifests as a corrupted SLUB freelist, which often results in a memory leak, an infinite loop in kmalloc, or a panic. This change adds a refcount to Compound and performs get/put operations, deleting it when there are zero refs. Prior to this change, AFL was able to generate a config that would double free in remove_element_type. The config no longer double frees after this change. I used valgrind to verify that no memory leaks were caused as a result of refcounting. Originally I modified add_element_type to get a ref; however, only one call site actually needs to do this, so code to put the extra ref needed to be added after all the other call sites. Therefore, I just added the get before that specific call site. I did not add refs when Compounds are chained & it doesn't seem to affect this particular test. Thanks, American Fuzzy Lop Change-Id: Idf20188d14f392aa5ab5f4bd1ad48763eb3a7929 * FromDump reworked This enhances the way parameter FORCE_LEN works. When FORCE_LEN==-1, FromDump works as before. When FORCE_LEN=x, where x in [MIN_MTU, MAX_MTU], each frame is crafted with a fixed length x bytes. When FORCE_LEN=0, each frame takes its original (actual) length. IP checksums are properly computed when needed. Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com> * Clean-up comments in FromDump Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com> * Keep useful comment Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com> * FromDump excludes packets that exceed the max buffer len This is a temporary solution to the memory issue tbarbette#119 Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com> * FromDump can only enlarge frames up to 2048 bytes This is a temporary solution to the problem so let's keep this pull request open until I commit a better solution to the memory problem Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com> * Kill packets upon failure This commit closes the pull request on my side. Now the focus shifts to the memory management issue once FromDump has allocated a large enough packet outside of the DPDK zone. Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com> * lexer: fix Compound::expand_into sanity check The check here appears to be intended to make sure that we only connect if both indexes are valid; specifically, idx can be -1. Unfortunately, it checks (*cp)[0] twice. Instead, check (*cp)[1] too. Thanks, American Fuzzy Lop Change-Id: I5cdeb45ac0cef5bbb0a18c6f19b4540df8007bfe * ip6address: fix parser array bounds issue/crash A malformed IP6Address, i.e. with more than 8 components, will cause the memmove/memset at the end of parsing to fail. This code is supposed to move the post-:: bytes to the end of the address and zero out the bytes in the center. Instead, it moves some of the bytes the wrong way, potentially writing prior to parts[] and then zeroing with a negative size. Instead, make the parser stop when it hits 8 components and let parse figure out that we did not consume the entire String. Thanks: American Fuzzy Lop Change-Id: I2966cb22fb5b44eb9e92dfc6a94e891cf5d02fa7 * Library: Add a library folder in conf Let's have a few elementclass that implements what everyone repeats * Fix documentation * Add link to code in documentation, and support for batching * Ignore vim history * From*Device: Add SCALE parameter If parallel, all available threads will open one queue of each device, for better load balancing. If shared (current default and behaviour), threads will be split among devices, for better CPU locality. * ToDPDKDevice: Fix bug introduced by last commit * SetIPAddress: Make batch-compatible * Move gtp package to tunnel Soon to add GRE and ERSPAN, let's not make thousands packages, those are all for some kind of encapsulation * Support ERSPAN decapsulation Allows to set various informations in annotations, timestamp if Type 3, ... * If egress/ingress not available, use encap as paint * FromDPDKDevice: Allow to read MTU using handlers * Travis : enable tunneling * DPDK: Accept on and off (like ethtool) for pause frame parameters * String: import from metron Add the split function Correction in trim, by Georgios Katsikas * CPUSwitch : import from Metron Correct the get_spawning_threads function CPUSwitch outputs packets oon a given output with only one CPU, of course * Fix warnings * Prepare for DPDK 18.11 * DPDK: Import h_queue_count from Metron The handler allows to query the number of packets left in the queue * DPDK: Fix queue assignation (import from Metron) Some odd assignment could create a problem there * Import from Metron: configure detection * IPReassembler: pack ChunkLink, since it can be arbitrarily placed in Packet::data These two uint16_ts may end up placed at arbitrary offsets into a packet. In order to generate proper unaligned read/write instructions, mark the struct as packed. This was exposed by Defensic suite IPv4 Header Fragment-offset, test 45067 and discovered by Waldin Stone. I verified it by asserting proper alignment of ChunkLinks and found that they were not always properly aligned. * ip/ipreassembler: account for header length changes When storing the first of a series of fragments, IPReassembler copies the IP header from the fragment to the reassembly buffer. However, the header of the new fragment may not be the same length as the original fragment that created the buffer. If the new fragment's header is shorter, we will end up with weirdness preceeding the payload. If the new fragment's header is longer, we will overwrite the payload and possibly overrun it. Both cases show up as 'bad mem_used' warnings. Changing header_delta in the p_off=0 case to account for header length changes (and not just offset changes) fixes the issue. Found by Waldin Stone, running Defensics. Signed-off-by: pallas@meraki.com * ip/ipreassembler: make sure to expand packet enough to hold ChunkLink We actually need to make sure to expand the data area to hold ChunkLink; right now, it's just checking the size of the data, so we might write out of bounds later on. Found-by: jtruba@meraki.com Thanks: AddressSanitizer * Configurable headroom for RatedSource Also description update for InfiniteSource
|
WHere are we on this? |
|
no progress on this, will be back after the submission.. |
|
WritablePacket::make might not succeed. There needs to be a bailout prior to |
|
Indeed, this check is mandatory both in EnsureDPDKBuffer and EnsureNetmapBuffer smactions. |
gkatsikas
added
wait-for-op
|
See a fix proposed in PR #161 |
When I replay a trace with raw IP packets I get the following memory dump:
Thread 1 "click" received signal SIGABRT, Aborted.
0x00007ffff5ed0428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
54 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
No0 0x00007ffff5ed0428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
No1 0x00007ffff5ed202a in __GI_abort () at abort.c:89
No2 0x00007ffff5f127ea in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0x7ffff602bed8 "*** Error in `%s': %s: 0x%s ***\n") at ../sysdeps/posix/libc_fatal.c:175
No3 0x00007ffff5f1b37a in malloc_printerr (ar_ptr=, ptr=, str=0x7ffff6028caf "free(): invalid pointer", action=3) at malloc.c:5006
No4 _int_free (av=, p=, have_lock=0) at malloc.c:3867
No5 0x00007ffff5f1f53c in __GI___libc_free (mem=) at malloc.c:2968
No6 0x000000000066ab2f in Packet::~Packet (this=0x15fbbb0, __in_chrg=) at ../lib/packet.cc:236
No7 WritablePacket::~WritablePacket (this=0x15fbbb0, __in_chrg=) at ../include/click/packet.hh:950
No8 WritablePacket::recycle (p=p@entry=0x15fbbb0) at ../lib/packet.cc:538
No9 0x00000000006019b6 in Packet::kill (this=0x15fbbb0) at ../include/click/packet.hh:1697
No10 EnsureDPDKBuffer::smaction (p=0x15fbbb0, this=0x15ea230) at ../elements/userlevel/ensuredpdkbuffer.cc:87
No11 EnsureDPDKBuffer::simple_action (this=0x15ea230, p=0x15fbbb0) at ../elements/userlevel/ensuredpdkbuffer.cc:106
No12 0x000000000067f9e5 in Element::push (this=0x15ea230, port=0, p=) at ../lib/element.cc:3061
No13 0x00000000006059f8 in Element::Port::push (p=, this=) at ../include/click/element.hh:714
No14 FromDump::run_task (this=0x15e9690) at ../elements/userlevel/fromdump.cc:605
No15 0x00000000006aff29 in Task::fire (this=0x15e9860) at ../include/click/task.hh:584
No16 RouterThread::run_tasks (ntasks=15, this=0x15698f0) at ../lib/routerthread.cc:392
No17 RouterThread::driver (this=0x15698f0) at ../lib/routerthread.cc:613
No18 0x00000000004911ca in main (argc=, argv=) at click.cc:809
To reproduce, use the trace /mnt/traces/caida/caida_939_to_941.pcap on rack7 and a simple configuration:
FromDump -> EnsureDPDKBuffer -> Discard;
The text was updated successfully, but these errors were encountered: