cvs commit: src/sbin/ipfw ipfw2.c src/sys/netgraph ng_ipfw.cng_ipfw.h src/sys/netinet ip_fw.h ip_fw2.c ip_fw_pfil.c

[Gleb Smirnoff]

  Andre,

On Thu, Feb 10, 2005 at 04:07:12PM +0100, Andre Oppermann wrote:
A> > You haven't gave an example of normal setup, bringing recursion. Neither
A> > ng_ppp nor ng_l2tp can reinject packet to IP stack. Looking for recursion,
A> > you should look only at edge nodes. ng_ppp and ng_l2tp are protocol implementing
A> > nodes, not edge ones. Probably you meant that packet diverted via ng_ipfw
A> > traverses ng_ppp|ng_l2tp and comes on ng_iface node, which implements system
A> > interface. I will doubtfully count this setup as "normal", but anyway there is
A> > no recursion. ng_iface, as a well designed edge node, queues packets for ISR.
A> 
A> Ok, I was looking for such a clear explanation.  I'm not an netgraph expert
A> at all and need you to tell me what happens in there to get the full picture.
A> Thanks for this writeup.  So far I was assuming the worst case.
A> 
A> Are you sure ng_iface does not and never will do direct dispatch?

It will do a direct dispatch in case of net.isr.enable=1. Thanks for noticing
this! It may re-enter IP stack not only in that ng_ipfw imaginary case. I suppose
it can re-enter IP stack in case of PPTP or PPP over UDP tunneling, in any case
when packets originate from IP layer.

We should probably change netisr_dispatch to schednetisr there. I'll discuss this
with Julian.

A> > Possible recursion, you are speaking about should be solved in other direction.
A> > Every edge node should queue packets, when they exit netgraph. And they all do.
A> > If you find one that does not, we will fix it not ng_ipfw.
A> 
A> How does the return path for ng_ipfw do it before it sends the packet back
A> to ip_input/output()?  And different question: If netgraph doesn't queue
A> the packet why can't you just pick it up again and let it continue directly
A> from ip_ipfw_pfil?  E.g. why does it have to reinject the packet?

Netgraph is not a chain of functions which work on packet and return. When
one node sends packet to its neighbor, it should never access this packet anymore.
Sending packet usually leads to functional calling of rcvdata methods of serie
of nodes, however at any point packet may be queued if node is not capable to
work on it here and now. So, if we did ng_snd_item(), we must never access the item
and asscotiated mbuf. NG_SEND_DATA_ONLY is macro around ng_snd_item().
That's why ng_ipfw implementation is similar to dummynet.

A> > A> > A> The other thing is the passing back of errors from netgraph.  Only
A> > A> > A> certain kinds of errors should be reported back and others converted
A> > A> > A> to some default error.  It is very confusing for an application
A> > A> > A> developer to get a very (from his POV) non-sensical error message
A> > A> > A> like ENOTCONN when writing on a socket.  He doesn't have knowledge
A> > A> > A> of the ipfw/netgraph stuff that happens in the kernel and it makes
A> > A> > A> debugging extremely confusing.  In the he blames FreeBSDs socket
A> > A> > A> implementation whereas it was only some error in setting up the
A> > A> > A> netgraph by the administrator.
A> > A> >
A> > A> > I have already replied this in net@ list. OK, I'll ask again: Do you want
A> > A> > ng_ipfw_rcvdata() to end with "return (0)"?
A> > A>
A> > A> No.  I want it to pass EACCES, ENOMEM/ENOBUFS and everything else as
A> > A> ENXIO (including if hook not connected).  In a previous email I said
A> > A> ESRCH was ok, but it really doesn't make sense and is pretty confusing
A> > A> to an application writer.  ENXIO is much better and not to be confused
A> > A> with possible programming errors from application side.
A> > 
A> > I have asked you in mail: "Do you want this ugly construction at and of
A> > ng_ipfw_input()?"
A> > 
A> >         NG_SEND_DATA_ONLY(error, hook, m);
A> > 
A> >         if (error == EACCES || error == ENOMEM || error == ENOBUFS)
A> >                 return (error);
A> >         else
A> >                 return (0);
A> > 
A> > Do you want it?
A> 
A> No, I want this:
A> 
A> 	NG_SEND_DATA_ONLY(error, hook, m);
A>  
A> 	if (error == EACCES || error == ENOMEM || error == ENOBUFS)
A> 		return (error);
A> 	else if (error != 0)
A> 		return (ENXIO);
A> 	else
A> 		return (0);
A> 
A> or alternatively:
A> 
A> 	switch (error) {
A> 		case 0:
A> 			return (0);
A> 			break;	/* Not reached. */
A> 
A> 		case EACCES:
A> 		case ENOMEM:
A> 		case ENOBUFS:
A> 			return (error);
A> 			break;	/* Not reached. */
A> 
A> 		default:
A> 			return (ENXIO);
A> 			break;	/* Not reached. */
A> 	}

Well, I don't like it, but accept. If Ruslan or Julian agree, feel free to commit.

-- 
Totus tuus, Glebius.
GLEBIUS-RIPN GLEB-RIPE