ip_len byte order in fr_fastroute

I've been having a problem with my gif tunnel in NetBSD/i386
3.1 and  
4.0RC2: It's basically become unusable.  A large portion of
the  
packets going out have the length field of the encapsulated
packet in  
host byte order rather than network byte order.  (I only ran
3.1  
briefly before updating to 4.0RC2; before that, 2.0 was
working  
fairly well for me.)

My tunnel configuration uses ipfilter to force traffic
originating on  
my local subnet to go out through the tunnel, because of ISP
 
filters.  When a packet from a server here arrives on the
NetBSD  
gateway box, the incoming filter sees that the source
address is one  
of my public ones, and the destination is non-local, so it
copies it  
out to the outgoing gif queue.  (It may be that only the
packets  
originating locally but not on the NetBSD box itself are
corrupted.   
The mail server on the NetBSD box itself, accessed with one
of the  
addresses delegated to me via the tunnel, seems to be
okay.)

I tweaked the code a little bit to set a hardware watchpoint
on the  
length field in gif_output, and delete it in in_gif_output
where I'd  
already determined the host-order value has been put in
place.  The  
hardware watchpoint was tripped in fr_fastroute, with the
following  
stack trace according to ddb:

fr_fastroute+0x18c
fr_check+0x605
fr_check_wrapper+0x97
pfil_run_hooks+0x91
ip_input+0x5dd
ipintr+0x24

fr_fastroute+0x18c is at line 1252:

		if (i) {
/* --> */		ip->ip_len = ntohs(ip->ip_len);
			ip->ip_off = ntohs(ip->ip_off);
		}

called from fr_check, fil.c:2730:

		} else if ((fdp->fd_ifp != NULL) &&
			   (fdp->fd_ifp != (struct ifnet *)-1)) {
			/* this is for to rules: */
			(void) fr_fastroute(fin->fin_m, mp, fin, fdp);
			m = *mp = NULL;
		}

So, if I've got it right, fr_fastroute gets a handle on the
packet  
with its length in host order, sets it to network order for
the call  
to the output routine, and then sets it back to host order,
just in  
case the caller of fr_fastroute wants to do something else
with the  
packet (e.g., in the dup-to case, I assume).  Then the gif
soft  
interrupt handler is invoked, and outputs on the real
network  
interface an encapsulated packet with its length in the
host's byte  
order.

Is the packet eventually supposed to get its length restored
to  
network order once again before the soft interrupt for gif
can be  
serviced?  Or should I look at making a copy of the packet
in  
gif_output, or maybe patch up the length in gif_intr or  
in_gif_output?  (I haven't looked to see if IPv6 tunnels
have a  
similar problem, but I'm just using the routing table to get
IPv6  
traffic to the right output queue.)

Ken

On Sun, Nov 04, 2007 at 09:19:43AM -0500, Ken Raeburn
wrote:
> I've been having a problem with my gif tunnel in
NetBSD/i386 3.1 and  
> 4.0RC2: It's basically become unusable.  A large
portion of the  
> packets going out have the length field of the
encapsulated packet in  
> host byte order rather than network byte order.  (I
only ran 3.1  
> briefly before updating to 4.0RC2; before that, 2.0 was
working  
> fairly well for me.)

A day or two ago I upgraded a gateway machine to RC3 and had
the same
problem. The system uses IPF to fastroute packets through a
ppp
connection, but the same thing happened when using a virtual
ethernet
over udp instead.

This problem seems to have been around for quite some time
now:
http://mail-index.netbsd.org/current-users/2005/0
2/24/0001.html
 

-- 
%SYSTEM-F-ANARCHISM, The operating system has been
overthrown

  So, if I've got it right, fr_fastroute gets a handle on
the packet
  with its length in host order, sets it to network order
for the call
  to the output routine, and then sets it back to host
order, just in
  case the caller of fr_fastroute wants to do something else
with the
  packet (e.g., in the dup-to case, I assume).  Then the gif
soft
  interrupt handler is invoked, and outputs on the real
network
  interface an encapsulated packet with its length in the
host's byte
  order.

As you note, there has long been a notion that length is in
host order
at some times and not others, and I'd go at this by
documenting the
rules in as many places as you can, checking in comments. 
But, I think
what might be going on is an assumption that once the call
from the
output routine returns that it's safe to modify the mbuf
again.  It
seems pretty dicey to be doing swap/call/swap when the mbuf
is shared
data structure.  So, making a copy probably is the safe
approach, and
I'm not sure what's necessary to get to safe and efficient.

On Sun, Nov 04, 2007 at 10:32:09AM -0500, Greg Troxel
wrote:
>   So, if I've got it right, fr_fastroute gets a handle
on the packet
>   with its length in host order, sets it to network
order for the call
>   to the output routine, and then sets it back to host
order, just in
>   case the caller of fr_fastroute wants to do something
else with the
>   packet (e.g., in the dup-to case, I assume).  Then
the gif soft
>   interrupt handler is invoked, and outputs on the real
network
>   interface an encapsulated packet with its length in
the host's byte
>   order.
> 
> As you note, there has long been a notion that length
is in host order
> at some times and not others, and I'd go at this by
documenting the
> rules in as many places as you can, checking in
comments.  But, I think
> what might be going on is an assumption that once the
call from the
> output routine returns that it's safe to modify the
mbuf again.  It
> seems pretty dicey to be doing swap/call/swap when the
mbuf is shared
> data structure.  So, making a copy probably is the safe
approach, and
> I'm not sure what's necessary to get to safe and
efficient.

Touching the mbuf at all after it has been passed to
if_output sounds
really wrong. Look at ether_output which frees the mbuf at
the end. The
caller of fr_fastroute really should not be doing anything
with it. For
dup-to, there is a call to M_DUPLICATE in fr_check.

On Nov 4, 2007, at 09:19, I wrote:
> called from fr_check, fil.c:2730:
>
> 		} else if ((fdp->fd_ifp != NULL) &&
> 			   (fdp->fd_ifp != (struct ifnet *)-1)) {
> 			/* this is for to rules: */
> 			(void) fr_fastroute(fin->fin_m, mp, fin, fdp);
> 			m = *mp = NULL;
> 		}
>
> So, if I've got it right, fr_fastroute gets a handle on
the packet  
> with its length in host order, sets it to network order
for the  
> call to the output routine, and then sets it back to
host order,  
> just in case the caller of fr_fastroute wants to do
something else  
> with the packet (e.g., in the dup-to case, I assume). 
Then the gif  
> soft interrupt handler is invoked, and outputs on the
real network  
> interface an encapsulated packet with its length in the
host's byte  
> order.
>
> Is the packet eventually supposed to get its length
restored to  
> network order once again before the soft interrupt for
gif can be  
> serviced?  Or should I look at making a copy of the
packet in  
> gif_output, or maybe patch up the length in gif_intr or
 
> in_gif_output?  (I haven't looked to see if IPv6
tunnels have a  
> similar problem, but I'm just using the routing table
to get IPv6  
> traffic to the right output queue.)

Okay, I found the bit I overlooked before where the network
byte  
order gets restored, in fr_check_wrapper.  However, it only
does so  
if *mp is non-null, and the above code is setting it to
null.   
Perhaps when *mp is set to null after a fr_fastroute call,
the ip_len  
field (and ip_off, apparently) should be restored to network
order in  
fr_check?

Ken

On Sun, Nov 04, 2007 at 10:21:58AM -0500, Ken Raeburn
wrote:
> On Nov 4, 2007, at 09:19, I wrote:
> >called from fr_check, fil.c:2730:
> >
> >		} else if ((fdp->fd_ifp != NULL) &&
> >			   (fdp->fd_ifp != (struct ifnet *)-1)) {
> >			/* this is for to rules: */
> >			(void) fr_fastroute(fin->fin_m, mp, fin,
fdp);
> >			m = *mp = NULL;
> >		}
> >
> >So, if I've got it right, fr_fastroute gets a
handle on the packet with its 
> >length in host order, sets it to network order for
the call to the output 
> >routine, and then sets it back to host order, just
in case the caller of 
> >fr_fastroute wants to do something else with the
packet (e.g., in the dup-to 
> >case, I assume).  Then the gif soft interrupt
handler is invoked, and outputs 
> >on the real network interface an encapsulated
packet with its length in the 
> >host's byte order.
> >
> >Is the packet eventually supposed to get its length
restored to network order 
> >once again before the soft interrupt for gif can be
serviced?  Or should I 
> >look at making a copy of the packet in gif_output,
or maybe patch up the 
> >length in gif_intr or in_gif_output?  (I haven't
looked to see if IPv6 tunnels 
> >have a similar problem, but I'm just using the
routing table to get IPv6 
> >traffic to the right output queue.)
> 
> Okay, I found the bit I overlooked before where the
network byte order gets 
> restored, in fr_check_wrapper.  However, it only does
so if *mp is non-null, 
> and the above code is setting it to null.  Perhaps when
*mp is set to null 
> after a fr_fastroute call, the ip_len field (and
ip_off, apparently) should be 
> restored to network order in fr_check?

IIUC, fr_fastroute sets *mp to NULL because fr_fastroute
effectively
frees the mbuf and nothing should touch it after.

Pavel