On Tue, Jan 05, 2021 at 10:05:39PM +1000, David Gwynne wrote: > If the idea is to avoid running most of pf_test again if route-to is > applied during ip_output, I think this tweaked diff is simpler. Is there > a valid use case for running some of pf_test again after route-to is > applied?
I found the original commit that introduced this strange check. ---------------------------- revision 1.294 date: 2003/01/02 01:56:56; author: dhartmei; state: Exp; lines: +27 -49; When route-to/reply-to is used in combination with address translation, pf_test() may be called twice for the same packet. In this case, make sure the translation is only applied in the second call. This solves the problem with state insert failures where the second pf_test() call tried to insert another state entry after the first call's translation. ok henning@, mcbride@, thanks to Joe Nall for additional testing. ---------------------------- I have tested your diffs in my setup, they all pass. I have not tested the scenario mentioned in the commit message. Note that the address translation implementation in 2003 was different from what we have now. And sasha@'s analysis shows that the current code is wrong in other use cases. The check in pf_find_state() seems to be unrelated to the call to pf_test() in pf_route(). I have to rethink it separately. How can we figure out what happens when we remove the check? It may harm some cases and benefit others or make no sense at all. My regression test, which tests each feature individually, is not affected. The only way to find out is to commit it. It reduces comlexity that noone understands. OK bluhm@ to remove the check Please leave the "if (pd->kif->pfik_ifp != ifp)" around pf_test() in pf_route() as it is for now.