ARP fix: ``int op'' to ``u_int16_t op'' in sys/netinet/if_ether.c

[Marco Munari <mar_(_dot_)_develops_(_at_)_allerta_(_dot_)_it>]

>Submitter-Id:  net
>Originator:    Marco Munari	http://mm.homeunix.org:8380/marcomunari/
>Organization:	allerta.it
>Synopsis:
	uninitialized int assigned as short possibly inhibit ARP
        to work properly
        (seems a gcc bug, but kernel code facilitate it)
>Severity:
	serious
>Priority:
	low
>Category:
	kernel, system
>Class:
	sw-bug
>Release:
	ALL (cvs current, since 18-Oct-95)
>Environment:
        System      : OpenBSD 3.8
        Architecture: OpenBSD.i386
        Machine     : i386
>Description:

In one of my OpenBSD systems ARP didn't work, the reason -common to OpenBSD-
was the following:

I noticed that op in if_ether.c is more natural of type u_int16_t,
insted of int

file,version:line:
sys/netinet/if_ether.c,1.1:531: 	int op;
                                	===
sys/netinet/if_ether.c,1.1:534: 	op = ntohs(ea->arp_op);

it heppen that op was -741081087 (= D3D40001) instead of 1
(in some 16-bit-also architectures such as AMD64, the highter
half of 32 bit is never initialized, but used in comparisons)


code generated by gcc:

d025e16c:       0f b7 41 06             movzwl 0x6(%ecx),%eax

d025e170:       86 c4                   xchg   %al,%ah
                                        ;rorw $8,%ax in ~i486+ w pipelines
                                        ;(xchg insted of rotate by 8 bits
                                        ; is efficient only in old CPU
                                        ; w/o pipelines)

d025e172:       66 89 45 b0             mov    %ax,0xffffffb0(%ebp)

last mov instruction is inapropriate for signed int,
this seems a compiler(gcc-3.3.5) bug, above instruction should be:
                                        movzwl %ax,0xffffffb0(%ebp)

I'll have the opportunity to verify again more carefully the gcc generated
code (currently I'm physically 600km far from the fixed machine that
didn't work properly),  the ``optimization'' flag -O2 produced
very different code each minimal changment in the source, I have some
dubts for now, except that
(int)op was -741081087  and  ARP didn't work for the wrong op
assignment in that machine.

PS: an explicit assigment op = (int)ntohs(ea->arp_op); seem to produce
    also the wrong code (but I didn't run the code in this second case).
    op = 0x0000ffff & ntohs(ea->arp_op);  // solves (but is undesirable)


to be precise my sys/arch/i386/include/endian.h contains:
#define __swap16md(x) ({                                                \
        u_int16_t __swap16md_x = (x);                                   \
        __asm ("xchgb %b1, %h1" : "+Q" (__swap16md_x)); /* changed */   \
        __swap16md_x;                                                   \
})
but the result is the same with the original line
	__asm ("rorw $8, %w1" : "+r" (__swap16md_x));			\



op nature is u_int16_t as the field ar_op alias arp_op in the structure.

if_ether.h:
/*
 * Ethernet Address Resolution Protocol.
 *
 * See RFC 826 for protocol description.  Structure below is adapted
 * to resolving internet addresses.  Field names used correspond to
 * RFC 826.
 */
struct	ether_arp {
	struct	 arphdr ea_hdr;			/* fixed-size header */
...
};
...
#define	arp_op	ea_hdr.ar_op
...

if_arp.h:
struct	arphdr {
...
	u_int16_t ar_op;	/* one of: */
        =========
#define	ARPOP_REQUEST	1	/* request to resolve address */
#define	ARPOP_REPLY	2	/* response to previous request */
#define	ARPOP_REVREQUEST 3	/* request protocol address given hardware */
#define	ARPOP_REVREPLY	4	/* response giving protocol address */
#define	ARPOP_INVREQUEST 8 	/* request to identify peer */
#define	ARPOP_INVREPLY	9	/* response identifying peer */
};
...



>How-To-Repeat:

ARP didn't work on the following architecture
     compiler_(_at_)_xsile:/usr/src/sys/arch/i386/compile/MM4C
 cpu0: AMD Athlon(tm) 64 Processor 3000+ ("AuthenticAMD" 686-class, 512KB L2 cache) 1.81 GHz
 cpu0: FPU,V86,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,SSE3
 cpu0: AMD Powernow: TS FID VID TTP TM STC
 cpu0: AMD Cool`n'Quiet K8 available states (31100,56000)
 real mem  = 3220742144 (3145256K)
....
 sk0 at skc0 port A, address 00:14:85:22:..:..
 eephy0 at sk0 phy 0: Marvell 88E1011 Gigabit PHY, rev. 5
 pchb1 at pci0 dev 24 function 0 "AMD AMD64 HyperTransport" rev 0x00
 pchb2 at pci0 dev 24 function 1 "AMD AMD64 Address Map" rev 0x00
 pchb3 at pci0 dev 24 function 2 "AMD AMD64 DRAM Cfg" rev 0x00
 pchb4 at pci0 dev 24 function 3 "AMD AMD64 Misc Cfg" rev 0x00

Reading specs from /usr/lib/gcc-lib/i386-unknown-openbsd3.8/3.3.5/specs
Configured with: 
Thread model: single
gcc version 3.3.5 (propolice)

relevant gcc flags used -O2 -os


>Fix:
in function in_arpinput() replace
        int op;
with
	u_int16_t op;

but a possible short int to int assignment bug concerning at least gcc with
amd64 in compatible mode.

my more rich diff for the involved files (including minor fixes for CARP
and a more efficient htonl/ntohl for i486+ architectures in endian.h)
is here:
http://www.allerta.it/pub/improvements/openbsd/kernel/if_ether_plus_endian.diff


All the best,
MARco
-- 
x(t),y(t) = th(3t-34.5)*e^[-(3t-34.5)^2]/2-4.3+e^(-1.8/t^2)/(.8*atg(t-
3)+2)(t-1.8)-.3th(5t-42.5),(1.4e^[-(3t-34.5)^2]+1-sgn[|t-8.5|-.5]*1.5*
|sin(pi*t)|^[2e^(-(t-11.5)^2)+.5+e^(-(.6t-3.3)^2)])/(.5+t)+1  ; 0<t<14
						http://www.allerta.it/