[Oisf-devel] Suricata 2.0dev + PF_RING 5.6.0 sporadic crashes in HTPCallbackRequest

[Chris Wakelin]

On 22/07/13 18:41, Victor Julien wrote:
> On 07/22/2013 07:28 PM, Chris Wakelin wrote:
>> On 22/07/13 18:01, Chris Wakelin wrote:
>>> On 22/07/13 17:54, Victor Julien wrote:
>>>>>>>> Still crashing sporadically I'm afraid, but now it's mostly in
>>>>>>>> htp_validate_hostname. I've attached another backtrace - does the frame
>>>>>>>> in ReceivePfringLoop make any sense?
>>>>>>>
>>>>>>> I agree the pfring data looks weird. Might be uninitialized, maybe
>>>>>>> pfring doesn't init it if it doesn't use it. Despite this value, suri
>>>>>>> figured out it's TCP.
>>>>>>>
>>>>>>
>>>>>> I did as Ivan suggested and upgraded to latest libhtp git (I was running
>>>>>> master from Friday 19th July) and latest Suricata too (likewise)
>>>>>>
>>>>>> Here's a backtrace for another crash we've been seeing a bit less
>>>>>> frequently but has just re-occurred (with the latest git releases).
>>>>>>
>>>>>> Again the ReceivePfringLoop looks a bit suspect to me :)
>>>>>
>>>>> I've pinged Luca about it, will let you know.
>>>>>
>>>>
>>>> I was told:
>>>>
>>>> "in order to optimise the computation the pfring header does not contain
>>>> parsing info by default:
>>>> - with standard (kernel-based) drivers you should add
>>>> PF_RING_LONG_HEADER to the pfring_open() flags.
>>>> - with dna we parse the packet in 1-copy mode only (passing a buffer
>>>> with a buffer-len > 0 to pfring_recv()) for the same reason.
>>>> You can explicitly call the parsing routine calling pfring_parse_pkt()"
>>>>
>>>> So the seemingly random/weird data is the result of us not using
>>>> PF_RING_LONG_HEADER and pfring_parse_pkt(). So it's not an indication of
>>>> something bad.
>>>>
>>>> Cheers,
>>>> Victor
>>>>
>>>
>>> I was wanting to find the traffic that caused suricata to segfault.
>>> Perhaps I should add the "PF_RING_LONG_HEADER" flag to my debug version
>>> (the one compiled with CFLAGS="-ggdb -O0"), though I suspect it
>>> decreases performance slightly.
>>>
>>> I guess we should also have told him I'm using PF_RING with DNA and
>>> libzero (though if you told him it was me, he probably already knows :-) )
>>>
>>> Best Wishes,
>>> Chris
>>>
>>
>> Actually, we already have PF_RING_LONG_HEADER in the flags in
>> src/source-pfring.c - and I should have read his reply more carefully;
>> it seems it doesn't apply to DNA anyway. I'll look at adding an explicit
>> pfring_parse_pkt() call when I work out what parameters it needs and
>> where to put it!
> 
> Ah, good point. I missed us using LONG_HEADER :) So for the dna case we
> should call pfring_parse_pkt() maybe. I guess if we somehow need the
> LONG_HEADER, so we'd need the parse_pkt then too.
> 

Just trying:
--- src/source-pfring.c.orig    2013-07-19 15:01:11.472332606 +0100
+++ src/source-pfring.c 2013-07-22 18:44:33.806959742 +0100
@@ -209,6 +209,8 @@

     PfringThreadVars *ptv = (PfringThreadVars *)user;

+    pfring_parse_pkt(p, h, 4, 1, 0);
+
     ptv->bytes += h->caplen;
     ptv->pkts++;
     (void) SC_ATOMIC_ADD(ptv->livedev->pkts, 1);

assuming that in

int pfring_parse_pkt(u_char *pkt, struct pfring_pkthdr *hdr, u_int8_t
level, u_int8_t add_timestamp, u_int8_t add_hash)

level ("The header level where to stop parsing") is the OSI level and we
may as well get PF_RING to add the timestamp.

If it has a performance impact it might be worth including only for DNA
(because for non-DNA the PF_RING_LONG_HEADER flag should do the same
thing) and perhaps only for debug.

Best Wishes,
Chris

-- 
--+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+-
Christopher Wakelin,                           c.d.wakelin at reading.ac.uk
IT Services Centre, The University of Reading,  Tel: +44 (0)118 378 2908
Whiteknights, Reading, RG6 6AF, UK              Fax: +44 (0)118 975 3094