From 4a1a008009563f12e995eb1f01dd0bdd4f3c62de Mon Sep 17 00:00:00 2001 From: Eric Leblond Date: Sat, 8 Sep 2012 11:48:59 +0200 Subject: [PATCH] af-packet: fix looping in ring buffer. A crash can occurs in the following conditions: * Suricata running in other mode than "workers" * Kernel fill in the ring buffer Under this conditions, it is possible that the capture thread reads a packet that has not yet released by one of the treatment threads because there is no modification done on the ring buffer entry when a packet is read. Doing, this it access to memory which can be released to the kernel and modified. This results in a kind of memory corruption. This bug has only been seen recently and this has to be linked with the read speed improvement recently made in AF_PACKET support. The patch fixes the issue by modifying the tp_status bitmask in the ring buffer. It sets the TP_STATUS_USER_BUSY flag when it is confirmed that the packet will be treated. And at the start of the read, it exits from the reading loop (returning to poll) when it reaches a packet with the flag set. As tp_status is set to 0 during packet release the flag is destroyed when releasing the packet. Regarding concurrency, we've got a sequence of modification. The capture thread read the packet and set the flag, then it passes the queue and the packet get processed by other threads. The change on tp_status are thus made at different time. Regarding the value of the flag, the patch uses the last bit of tp_status to avoid be impacting by a change in kernel. I will propose a patch to have TP_STATUS_USER_BUSY included in kernel as this is a generic issue for multithreading application using AF_PACKET mechanism. --- src/source-af-packet.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/source-af-packet.c b/src/source-af-packet.c index bdad54048b..904d571f5d 100644 --- a/src/source-af-packet.c +++ b/src/source-af-packet.c @@ -124,6 +124,11 @@ TmEcode NoAFPSupportExit(ThreadVars *tv, void *initdata, void **data) #define POLL_TIMEOUT 100 +#ifndef TP_STATUS_USER_BUSY +/* for new use latest bit available in tp_status */ +#define TP_STATUS_USER_BUSY (1 << 31) +#endif + /** protect pfring_set_bpf_filter, as it is not thread safe */ static SCMutex afpacket_bpf_set_filter_lock = PTHREAD_MUTEX_INITIALIZER; @@ -696,6 +701,12 @@ int AFPReadFromRing(AFPThreadVars *ptv) read_pkts++; + /* Our packet is still used by suricata, we exit read loop to + * gain some time */ + if (h.h2->tp_status & TP_STATUS_USER_BUSY) { + SCReturnInt(AFP_READ_OK); + } + if ((ptv->flags & AFP_EMERGENCY_MODE) && (emergency_flush == 1)) { h.h2->tp_status = TP_STATUS_KERNEL; goto next_frame; @@ -706,6 +717,11 @@ int AFPReadFromRing(AFPThreadVars *ptv) SCReturnInt(AFP_FAILURE); } + /* Suricata will treat packet so telling it is busy, this + * status will be reset to 0 (ie TP_STATUS_KERNEL) in the release + * function. */ + h.h2->tp_status |= TP_STATUS_USER_BUSY; + p->afp_v.relptr = h.raw; p->ReleaseData = AFPReleaseDataFromRing; p->afp_v.mpeer = ptv->mpeer; -- 2.47.2