On Thu, Jun 16, 2011 at 11:18:30PM +0200, Claudio Jeker wrote: > On Wed, Jun 15, 2011 at 11:06:16PM -0400, Kenneth R Westerback wrote: > > On Wed, Jun 15, 2011 at 09:50:51PM -0400, Kenneth R Westerback wrote: > > > On Tue, Jun 14, 2011 at 10:23:36PM +0200, Claudio Jeker wrote: > > > > On Wed, Jun 08, 2011 at 09:53:24AM +0200, Claudio Jeker wrote: > > > > > Next step on my quest to unify pf_test and pf_test6. > > > > > Move the fragment handling and some other protocol specific tests into > > > > > pf_setup_pdesc(). IPv6 already does this mostly but only IPv4 did > > > > > not. So > > > > > this diff brings that more into sync. It also includes some additional > > > > > cleanups. > > > > > > > > > > Works for me but needs some more testing. > > > > > > > > New version with fixes found by bluhm@ (mostly make sure that the right > > > > header and mbufs are used in all cases). > > > > > > > > -- > > > > :wq Claudio > > > > > > Blows up REAL good on my 6xamd64 box. Within a minute or two. > > > > > > I put some debug in and pf_scrub() is being passed a NULL 'm' parameter > > > from pf_test(). Hand transcribed trace: > > > > > > pf_scrub()+0x12 > > > pf_test()+0x93b > > > ipv4_input()+0x22a > > > ipintr()+0x51 > > > netintr()+0xda > > > softintr_dispatch()+0x5d > > > Xsoftnet()+0x2d > > > > > > This box is my nfs server and is using the default pf.conf file as far > > > as I know. Also blew up on my firewall as soon as traffic started > > > transitting it. Same trace as far as I can see. > > > > > > .... Ken > > > > > > > panic'ing my way along the stack I find that the problem starts in > > pf_reassemble(), when the 'if (!pf_isfull_fragment(frag))' test > > leaves m0 as NULL and returns PF_PASS to pf_normalize_ip(). *m0 is > > tested and found to be NULL, so pf_normalize_ip() returns PF_PASS to > > pf_setup_pdesc(). Where *m0 is tested for NULL and causes *action to > > be set to PF_PASS and pf_setup_pdesc() to return -1 to pf_test. m is > > set to *m0 (i.e. NULL) and then pf_test goto's done:. action is PF_PASS > > and 'if (s)' fails so control passes to the first statement of the else, > > which is a pf_scrub() call with m being NULL. Boom. > > > > Yes, very good analysis. I came to the same conclusion. I do not > understand why we do the goto done; case for PF_PASS in case of a > pf_setup_pdesc() failure. Since in that case *m0 is NULL and so we can > just return directly from there. I changed my diff to do exactly that. On > further inspection I think there is no other case where pf_setup_pdesc() > returns -1 but action is set to PF_PASS. IMO this is a lot saver then > trying to execute all that code after the done: label. > > New diff below > -- > :wq Claudio
This doesn't immediately crash my nfs server like the original did. I'll let you know if anything else goes wrong. :-) .... Ken
