still looking for ok's for this version of the diff. although i've got mcbride's and claudio's oks for the older version, this is the one i consider correct.
On Fri, Oct 28, 2011 at 3:59 PM, Mike Belopuhov <[email protected]> wrote: > hi, > > icmp6->icmp translation does't work because of the strange "icmp direction" > check in pf_icmp_state_lookup. apparently it's supposed to check that > icmp echo direction matches icmp_dir. the funny thing is how icmp type > was checked: "if ((*state)->rule.ptr->type". ?echo response for ipv4 is 0 > so the check is not performed, but in ipvshit worlds things are different > and both echoreq and echorep have non zero values. ?i suggest to change > this check the following way to give ipvshit a chance to work too. > > ok? > i was uncomfortable with this change and took some time to reevaluate the patch. turned out i made a right decision not to commit it right away. in the old diff i'm changing the logic from "check direction of any icmp type except for icmp echo *states* (not packets, this includes replies)" to "check direction of icmp echo states only". what got me confused is the the check "if ((*state)->rule.ptr->type)" itself. i have arrived at conclusion that it's useless. we can check *all* icmp packets just fine. conversely, when af-to is used there's only one state for both input and output and it's direction is always PF_IN. therefore, it's pointless to check it. instead we should derive direction from the difference of address families in PF_SK_WIRE and PF_SK_STACK state keys. i've beaten this diff for quite some time and have tried different kinds of icmp combinations including "inner" ones: icmp as a payload of another icmp (e.g. destination unreachable for an icmp echo request) and it works just fine. ok? Index: pf.c =================================================================== RCS file: /cvs/src/sys/net/pf.c,v retrieving revision 1.787 diff -u -p -r1.787 pf.c --- pf.c 26 Nov 2011 03:28:46 -0000 1.787 +++ pf.c 28 Nov 2011 00:08:09 -0000 @@ -4557,6 +4557,8 @@ pf_icmp_state_lookup(struct pf_pdesc *pd struct pf_state **state, u_int16_t icmpid, u_int16_t type, int icmp_dir, int *iidx, int multi, int inner) { + int direction; + key->af = pd->af; key->proto = pd->proto; key->rdomain = pd->rdomain; @@ -4592,9 +4594,13 @@ pf_icmp_state_lookup(struct pf_pdesc *pd STATE_LOOKUP(pd->kif, key, pd->dir, *state, pd->m); /* Is this ICMP message flowing in right direction? */ - if ((*state)->rule.ptr->type && - (((!inner && (*state)->direction == pd->dir) || - (inner && (*state)->direction != pd->dir)) ? + if ((*state)->key[PF_SK_WIRE]->af != (*state)->key[PF_SK_STACK]->af) + direction = (pd->af == (*state)->key[PF_SK_WIRE]->af) ? + PF_IN : PF_OUT; + else + direction = (*state)->direction; + if ((((!inner && direction == pd->dir) || + (inner && direction != pd->dir)) ? PF_IN : PF_OUT) != icmp_dir) { if (pf_status.debug >= LOG_NOTICE) { log(LOG_NOTICE,
