svn commit: r198301 - head/sys/netinet

[Robert Watson]

On Wed, 21 Oct 2009, Qing Li wrote:

> I believe this patch will fix your crash.

Sounds good, thanks!

Robert


>
> -- Qing
>
>
> On Wed, Oct 21, 2009 at 12:58 AM, Robert Watson <rwatson at freebsd.org> wrote:
>> 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);
>>>
>>
>