[CFT/Review] net byte order for AF_INET

[Maxim Dounin]

Hello!

On Wed, Oct 10, 2012 at 11:58:42PM +0400, Gleb Smirnoff wrote:

> On Tue, Oct 09, 2012 at 07:41:28PM +0400, Gleb Smirnoff wrote:
> T>   this is a patch that switches entire IPv4 stack to network
> T> byte order. That means, that at any layer any module should
> T> expect IP header in network byte order. Any host byte order
> T> values can be stored in local variables only and are never stored
> T> into a packet itself.
> 
> Update:
> 
>   - fresh patch against r241405, should apply to r241405
>   - fixed raw sockets` as Luigi and Max Dounin noticed, also fixed
>     another issue with raw sockets, now work fine - tested with ospfd
>   - fixed gre(4)
>   - took into account Luigi's comments about using host byte order
>     in any ==, <, > comparisons.
> 
> Decided not to touch ip_reass().
> 
> -- 
> Totus tuus, Glebius.

[...]

> --- sys/netinet/raw_ip.c	(revision 241405)
> +++ sys/netinet/raw_ip.c	(working copy)
> @@ -292,7 +292,7 @@
>  	 * not modify the packet except for some
>  	 * byte order swaps.
>  	 */
> -	ip->ip_len += off;
> +	ip->ip_len = htons(ip->ip_len) + off;

This one is certainly wrong as it uses arithmetic with a number in 
network byte order.

>  
>  	hash = INP_PCBHASH_RAW(proto, ip->ip_src.s_addr,
>  	    ip->ip_dst.s_addr, V_ripcbinfo.ipi_hashmask);
> @@ -449,11 +449,11 @@
>  		ip = mtod(m, struct ip *);
>  		ip->ip_tos = inp->inp_ip_tos;
>  		if (inp->inp_flags & INP_DONTFRAG)
> -			ip->ip_off = IP_DF;
> +			ip->ip_off = htons(IP_DF);
>  		else
>  			ip->ip_off = 0;
>  		ip->ip_p = inp->inp_ip_p;
> -		ip->ip_len = m->m_pkthdr.len;
> +		ip->ip_len = htons(m->m_pkthdr.len);
>  		ip->ip_src = inp->inp_laddr;
>  		if (jailed(inp->inp_cred)) {
>  			/*
> @@ -504,6 +504,9 @@
>  		if (ip->ip_id == 0)
>  			ip->ip_id = ip_newid();
>  
> +		ip->ip_len = htons(ip->ip_len);
> +		ip->ip_off = htons(ip->ip_off);
> +

So the packet is expected to come into rip_output() from caller 
with ip_len/ip_off in host byte order, right?  As already 
suggested - it would be good to add a comment explaining this.

[...]


-- 
Maxim Dounin