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

[Victor Julien]

On 07/22/2013 07:51 PM, Chris Wakelin wrote:
> 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);
> +

p is suricata's packet struct, I think you need to feed the raw data
GET_PKT_DATA(p) instead. As there is no way to give the length of the
data, I assume pfring will have validated the length already at this point.

>      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.
> 



-- 
---------------------------------------------
Victor Julien
http://www.inliniac.net/
PGP: http://www.inliniac.net/victorjulien.asc
---------------------------------------------