Subject: Re: Try again, itojun, patches need more work.
To: None <itojun@iijlab.net>
From: Darren Reed <avalon@caligula.anu.edu.au>
List: tech-net
Date: 06/30/2003 11:45:55
In some mail from itojun@iijlab.net, sie said:
> 
> >>Furthermore, the patches that bring pf into IP do not use
> >>pfil(9).  That is not acceptable.  It's there for a reason
> >>and the reason is for things like pf to use it.  If there
> >>is a deficiency in the interface then bring it up for
> >>discussion.
> >	please check near pf_test() calls.
> >	ip_input: i need to pass a parameter to ip_forward() (pfrdr),
> >		which is not possible with pfil(9) infrastructure.
> >	ip_output, ip6_*: i could use pfil(9), but i needed to patch ip_input
> >		anyways, so i did not bother to use pfil(9).
> >
> >	if you have suggestions wrt how ip_input() hook should be done,
> >	please let me know.  i have no clue how i can pass parameter to
> >	ip_forward.
> 
> 	or i can forget about pfrdr variable for now, and change pf to
> 	use pfil(9).  then you will be happy.

So what this "pfrdr" does, no thanks to the lack of appropriate comments,
is allow redirects back out the same interface a packet was received from
without generating an ICMP redirect.  Reviewing this, the comment header
for ip_forward() should be updated to indicate the *real* role of srcrt
(and that is to prevent redirects from being issued) and the code in
ip_input() should....well, if deemed useful it should go around the
pfil(9) hooks BUT putting an IP address in an "int" just seems so very
very wrong!  Maybe I'm spending too much time with C++ these days.  In
general, that seems like a useful change to make, just its nature seems
offensive.

On the API issues, moving the pf*() functions into uipc_mbuf2.c does
not seem like a long term solution, am I right ?

Or will you be merging those changes back into OpenBSD but have not
yet ?

I look in the pf code within the patch and I see #ifdef's to not use
the uipc_mbuf2.c routines for OpenBSD.  There should be no need for
those #ifdef's if you've settled on the API and both OS's are going
to start using the same code.

On a separate note, a standard part of all internal kernel API's is
a section 9 man page describing how they're used.

Is this "API" in the patch you've posted a final API or is it known
to be an interim measure and will be reviewed "soonish" ?  Kenjiro's
comments support the latter and you've given no comment either way.
If Kenjiro is the authority here then until that is finalised there
should be no patches/diffs or merging.

Darren