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