svn commit: r198301 - head/sys/netinet

[Robert Watson]

On Tue, 20 Oct 2009, Qing Li wrote:

>  In the ARP callout timer expiration function, the current time_second
>  is compared against the entry expiration time value (that was set based
>  on time_second) to check if the current time is larger than the set
>  expiration time. Due to the +/- timer granularity value, the comparison
>  returns false, causing the alternative code to be executed. The
>  alternative code path freed the memory without removing that entry
>  from the table list, causing a use-after-free bug.
>
>  Reviewed by:	discussed with kmacy
>  MFC after:	immediately
>  Verified by:	rnoland, yongari

Could this be the same problem I ran into overnight on an 18 October kernel:

Fatal trap 12: page fault while in kernel mode
cpuid = 0; apic id = 00
fault virtual address	= 0x7572749f
fault code		= supervisor read, page not present
instruction pointer	= 0x20:0xc09c0551
stack pointer	        = 0x28:0xc43f6ab4
frame pointer	        = 0x28:0xc43f6adc
code segment		= base 0x0, limit 0xfffff, type 0x1b
 			= DPL 0, pres 1, def32 1, gran 1
processor eflags	= interrupt enabled, resume, IOPL = 0
current process		= 0 (em0 taskq)

#9  0xc0be731b in calltrap () at ../../../i386/i386/exception.s:165
#10 0xc09c0551 in in_lltable_lookup (llt=0xc4955200, flags=8192,
     l3addr=0xc43f6b60) at ../../../netinet/in.c:1361
#11 0xc09b8ea7 in arpintr (m=0xc48dcd00) at if_llatbl.h:202
#12 0xc096f899 in netisr_dispatch_src (proto=7, source=0, m=0xc48dcd00)
     at ../../../net/netisr.c:917
#13 0xc096fb30 in netisr_dispatch (proto=7, m=0xc48dcd00)
     at ../../../net/netisr.c:1004
#14 0xc0967c11 in ether_demux (ifp=0xc470b400, m=0xc48dcd00)
     at ../../../net/if_ethersubr.c:895
#15 0xc0968163 in ether_input (ifp=0xc470b400, m=0xc48dcd00)
     at ../../../net/if_ethersubr.c:754
#16 0xc063bcaa in em_rxeof (adapter=0xc470d000, count=99)
     at ../../../dev/e1000/if_em.c:4610
#17 0xc063dfc7 in em_handle_rxtx (context=0xc470d000, pending=1)
     at ../../../dev/e1000/if_em.c:1763

(kgdb) frame 10
#10 0xc09c0551 in in_lltable_lookup (llt=0xc4955200, flags=8192,
     l3addr=0xc43f6b60) at ../../../netinet/in.c:1361
1361		LIST_FOREACH(lle, lleh, lle_next) {
(kgdb) list
1356		KASSERT(l3addr->sa_family == AF_INET,
1357		    ("sin_family %d", l3addr->sa_family));
1358
1359		hashkey = sin->sin_addr.s_addr;
1360		lleh = &llt->lle_head[LLATBL_HASH(hashkey, LLTBL_HASHMASK)];
1361		LIST_FOREACH(lle, lleh, lle_next) {
1362			struct sockaddr_in *sa2 = (struct sockaddr_in 
*)L3_ADDR(lle);
1363			if (lle->la_flags & LLE_DELETED)
1364				continue;
1365			if (sa2->sin_addr.s_addr == sin->sin_addr.s_addr)
(kgdb) print *llt
$5 = {llt_link = {sle_next = 0x0}, lle_head = {{lh_first = 0xc760d580}, {
       lh_first = 0x0}, {lh_first = 0x0}, {lh_first = 0xc75c3900}, {
       lh_first = 0x0} <repeats 14 times>, {lh_first = 0xc49a8380}, {
       lh_first = 0x0}, {lh_first = 0x0}, {lh_first = 0x0}, {lh_first = 0x0}, {
       lh_first = 0x0}, {lh_first = 0x0}, {lh_first = 0x0}, {lh_first = 0x0}, {
       lh_first = 0x0}, {lh_first = 0xc61be780}, {lh_first = 0x0}, {
       lh_first = 0x0}, {lh_first = 0x0}}, llt_af = 2, llt_ifp = 0xc470b400,
   llt_new = 0xc09bdd60 <in_lltable_new>,
   llt_free = 0xc09c0490 <in_lltable_free>,
   llt_prefix_free = 0xc09c09b0 <in_lltable_prefix_free>,
   llt_lookup = 0xc09c0510 <in_lltable_lookup>,
   llt_rtcheck = 0xc09bdbe0 <in_lltable_rtcheck>,
   llt_dump = 0xc09bd9c0 <in_lltable_dump>}
(kgdb) print *l3addr
$6 = {sa_len = 16 '\020', sa_family = 2 '\002',
   sa_data = "\000\000??*\001\000\000\000\000\000\000\000"}
(kgdb) print lle
$7 = (struct llentry *) 0x75727473
(kgdb) print lleh
$8 = (struct llentries *) 0xc4955210
(kgdb) print *lleh
$9 = {lh_first = 0xc75c3900}

Your commit was after this kernel, so I'd be quite happy with the answer "now 
fixed" but wanted to be sure.

Robert

>
> Modified:
>  head/sys/netinet/if_ether.c
>
> Modified: head/sys/netinet/if_ether.c
> ==============================================================================
> --- head/sys/netinet/if_ether.c	Tue Oct 20 17:50:36 2009	(r198300)
> +++ head/sys/netinet/if_ether.c	Tue Oct 20 17:55:42 2009	(r198301)
> @@ -175,18 +175,18 @@ arptimer(void *arg)
> 	CURVNET_SET(ifp->if_vnet);
> 	IF_AFDATA_LOCK(ifp);
> 	LLE_WLOCK(lle);
> -	if (((lle->la_flags & LLE_DELETED) ||
> -	    (time_second >= lle->la_expire)) &&
> -	    (!callout_pending(&lle->la_timer) &&
> +	if ((!callout_pending(&lle->la_timer) &&
> 	    callout_active(&lle->la_timer))) {
> 		(void) llentry_free(lle);
> 		ARPSTAT_INC(timeouts);
> -	} else {
> -		/*
> -		 * Still valid, just drop our reference
> -		 */
> -		LLE_FREE_LOCKED(lle);
> +	}
> +#ifdef DIAGNOSTICS
> +	else {
> +		struct sockaddr *l3addr = L3_ADDR(lle);
> +		log(LOG_INFO, "arptimer issue: %p, IPv4 address: \"%s\"\n", lle,
> +		    inet_ntoa(((const struct sockaddr_in *)l3addr)->sin_addr));
> 	}
> +#endif
> 	IF_AFDATA_UNLOCK(ifp);
> 	CURVNET_RESTORE();
> }
> @@ -377,7 +377,7 @@ retry:
>
> 	if (renew) {
> 		LLE_ADDREF(la);
> -		la->la_expire = time_second;
> +		la->la_expire = time_second + V_arpt_down;
> 		callout_reset(&la->la_timer, hz * V_arpt_down, arptimer, la);
> 		la->la_asked++;
> 		LLE_WUNLOCK(la);
>