> I was looking things over and it's not clear why in SunOS/pfil.c
> pfil_precheck you have:
>
> for (; pfh; pfh = pfh->pfil_next)
> if (pfh->pfil_func) {
> err = (*pfh->pfil_func)(ip, iphlen, qif, out, qpi,
> mp);
> if (err || !*mp)
> break;
> /*
> * fr_pullup may have allocated a new buffer.
> */
> ip = qpi->qpi_data;
> }
>
> instead of:
>
> for (; pfh; pfh = pfh->pfil_next)
> if (pfh->pfil_func) {
> err = (*pfh->pfil_func)(ip, iphlen, qif, out, qpi,
> mp);
> /*
> * fr_pullup may have allocated a new buffer.
> */
> ip = qpi->qpi_data;
>
> if (err || !*mp)
> break;
> }
Because an error is being returned and it's not clear that qpi_data should
actually mean anything in that case.
> which I believe is less error prone. Consider the following:
>
> 1) A packet's byte ordering is change prior to the for loop.
>
> 2) One of the pfil_func functions allocates a new buffer and then
> returns an error.
>
> The pfil-next.tar.gz code will refuse to fix the byte ordering before
> returning the error which leaves the contents of the packet useless.
> This causes a problem in pfilmodwput and pfilmodrput which pass the
> packet to putnext if the error was less than zero. If pfil_precheck
> is going to render the packet useless, then it should explicitly
> discard it to avoid unexpected behavior.
I wouldn't expect a pfil_func thing to be returning < 0. I've used
that as a distinguishing feature to let messages flow up and down
that are requried for IP to work on Solaris. I would class this as
an unspoken/undocumented framework requirement.
> This of course raises the question of what should pfil_precheck return
> for various types of failures. Currently the code that allocates a new
> mblk chain (in order to fix alignment etc) returns -3 if dupb / msgpullup
> fails which is consistent with the old code. This results in pfilmodwput
> and pfilmodrput calling putnext.
>
> a) Considering this is firewall software shouldn't pfilmodwput
> and pfilmodrput discard the packet if pfil_precheck encounters
> an error?
This is in your newly added code...yes, the current code is wrong in
returning -3.
> b) Should ENOBUFS be returned if dup / msgpullup / pullupmsg fails
> (similar to EMSGSIZE being returned for large IP6 packets)?
Yes.
Darren
