Re: kern/38535: locking bug in ifmedia_ioctl

[Andrew Doran <ad%netbsd.org@localhost>]

The following reply was made to PR kern/38535; it has been noted by GNATS.

From: Andrew Doran <ad%netbsd.org@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: 
Subject: Re: kern/38535: locking bug in ifmedia_ioctl
Date: Mon, 28 Apr 2008 22:54:14 +0100

 On Mon, Apr 28, 2008 at 06:50:00PM +0000, martin%duskware.de@localhost wrote:
 
 > Stopped in pid 3.1 (ifconfig) at        netbsd:cpu_Debugger+0x4:        nop
 > db> bt                                                                     
 > lockdebug_abort1(1476780, 1476780, 120cbf0, 1241308, 1, 800) at 
 > netbsd:lockdebug
 > _abort1+0x7c                                                                 
 >   
 > lockdebug_barrier(145ea00, 1, 3, 1, 0, 0) at netbsd:lockdebug_barrier+0x118
 > assert_sleepable(2755c50, 122aac0, 24, 122aae8, ffffffffffffba78, 40a0a800) 
 > at n
 > etbsd:assert_sleepable+0x20                                                  
 >   
 > malloc(0, 140d620, 0, b95f8c0, 2755d10, badcafe) at netbsd:malloc+0x230
 > ifmedia_ioctl(18, c8fdc80, 2755c50, 6, 2755d10, 1) at 
 > netbsd:ifmedia_ioctl+0x150
 >                                                                              
 >   
 > hme_ioctl(27558f0, c0306936, c8fdc80, 0, 1, badcafe) at netbsd:hme_ioctl+0x3c
 > ifioctl(2d, c0306936, c8fdc80, b95f8c0, 100a740, 40a0003c) at 
 > netbsd:ifioctl+0x4
 > 08                                                                           
 >   
 > sys_ioctl(0, c8fddc0, c8fde00, 407472e0, 4091f8e0, 80000000) at 
 > netbsd:sys_ioctl
 > +0xdc                                                                        
 >   
 > syscall_plain(c8fded0, 3, 4073e6e4, 2, 4073e6e4, 800) at 
 > netbsd:syscall_plain+0x
 > 120                                                                          
 >   
 > ?(3, c0306936, ffffffffffffb978, 401, 0, 0) at 0x10092fc
 
 I guess hme_ioctl shouldn't hold a spinlock across ifmedia_ioctl, becuse it 
does an
 M_WAITOK allocation.
 
     331                /*
     332                 * Count them so we know a-priori how much is the max 
we'll
     333                 * need.
     334                 */
     335                ep = TAILQ_FIRST(&ifm->ifm_list);
     336                for (nwords = 0; ep != NULL; ep = TAILQ_NEXT(ep, 
ifm_list))
     337                        nwords++;
     338 
     339                if (ifmr->ifm_count != 0) {
     340                        size_t count;
     341                        size_t minwords = nwords > 
(size_t)ifmr->ifm_count
     342                            ? (size_t)ifmr->ifm_count
     343                            : nwords;
     344                        int *kptr = (int *)malloc(minwords * 
sizeof(int),
     345                            M_TEMP, M_WAITOK);
     346                        /*
     347                         * Get the media words from the interface's 
list.
     348                         */
     349                        ep = TAILQ_FIRST(&ifm->ifm_list);
     350                        for (count = 0; ep != NULL && count < minwords;
     351                            ep = TAILQ_NEXT(ep, ifm_list), count++)
     352                                kptr[count] = ep->ifm_media;
 
 That seems broken, presumably "nwords" can change if malloc() sleeps.
 
 Andrew