From owner-freebsd-stable@FreeBSD.ORG Tue Apr 21 21:08:30 2009 Return-Path: Delivered-To: freebsd-stable@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 128CB106564A for ; Tue, 21 Apr 2009 21:08:30 +0000 (UTC) (envelope-from to.my.trociny@gmail.com) Received: from fk-out-0910.google.com (fk-out-0910.google.com [209.85.128.190]) by mx1.freebsd.org (Postfix) with ESMTP id 849068FC0C for ; Tue, 21 Apr 2009 21:08:29 +0000 (UTC) (envelope-from to.my.trociny@gmail.com) Received: by fk-out-0910.google.com with SMTP id f33so213731fkf.11 for ; Tue, 21 Apr 2009 14:08:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:to:subject:references :organization:from:date:in-reply-to:message-id:user-agent :mime-version:content-type; bh=eSqGlckE2jBVyqOxj/CUL8Hksjj0+yrxIhoika89mC8=; b=JFTlfQv0TicI6s2v8R8/FujZrJkG8hH/QthIkUAc68ldOo67r+pLlS0COg3AkwFyX+ f+PRk8ZknIRabflbPEiPnNEQB529UWb5t4OUlDlTZ8Gnrox+celhKm70MptDGgc+Fz+Z vKYBwZl0KXIuX/quMBv67my4h22ux94k6AOFk= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=to:subject:references:organization:from:date:in-reply-to:message-id :user-agent:mime-version:content-type; b=dWoL37GZMFRGozUUdC6ov3oaY6Nf9xEP/iqEclMpaYxOlLp+67mNOCu4E7kjXvT9mE y3rrqO0gPJYks0TSn/Qzsnw3TzeJPrLI7JUjw+GMlgiwraU2UGJ5Kehtwa39uYim/H1n fOlhGIqYDxO0MLribQLtcZP24UApUxP8pw8ZI= Received: by 10.103.229.12 with SMTP id g12mr4081650mur.16.1240348105054; Tue, 21 Apr 2009 14:08:25 -0700 (PDT) Received: from localhost ([95.69.168.203]) by mx.google.com with ESMTPS id j10sm3569452muh.1.2009.04.21.14.08.24 (version=TLSv1/SSLv3 cipher=RC4-MD5); Tue, 21 Apr 2009 14:08:24 -0700 (PDT) To: freebsd-stable@FreeBSD.org References: <200904210524.n3L5O9YS086865@lava.sentex.ca> <200904211111.57295.jhb@freebsd.org> <200904211519.n3LFJFsk090691@lava.sentex.ca> <20090421153112.GA47589@edoofus.dev.vega.ru> <200904211610.n3LGAYll090970@lava.sentex.ca> Organization: TOA Ukraine From: Mikolaj Golub Date: Wed, 22 Apr 2009 00:08:22 +0300 In-Reply-To: (Robert Watson's message of "Tue\, 21 Apr 2009 18\:53\:54 +0100 \(BST\)") Message-ID: <86k55demux.fsf@kopusha.onet> User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.1 (berkeley-unix) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Cc: Subject: Re: RELENG_7 crash X-BeenThere: freebsd-stable@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Production branch of FreeBSD source code List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 21 Apr 2009 21:08:30 -0000 --=-=-= On Tue, 21 Apr 2009 18:53:54 +0100 (BST) Robert Watson wrote: RW> On Tue, 21 Apr 2009, Mike Tancsa wrote: >> At 11:31 AM 4/21/2009, Ruslan Ermilov wrote: >>> : >>> : Note that these changes simply close races around use of ifindex_table, >>> : and make no attempt to solve the probem of disappearing ifnets. Further >>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>> : refinement of this work, including with respect to ifindex_table >>> : resizing, is still required. >>> : >>> : In a future change, the ifnet lock should be converted from a mutex to an >>> : rwlock in order to reduce contention. >> >> Thanks for the info! In the mean, time, apart from disabling >> snmpwalking, is there anything I can do to mitigate triggering this >> bug ? The box runs ospf/zebra for routing daemons and mpd53 for l2tp >> LNS termination. RW> There are several bugs here, one difficult to fix (lack of RW> refcounting), but also stuff like ifp being derived from an interface RW> number twice, but checked against NULL only the first time (line 85 RW> checked for NULL, re-queried but no check line 88). Fixing the top RW> bit of the function to only query the ifp once and check it for NULL RW> then would be a good idea. More fundamentally, we do need to refcount RW> ifnets when used from the management path, which is not all that hard RW> a change, but preferably to try the easy way first given where we are RW> in the release cycle. I was thinking a bit on this problem (the same issue was reported in kern/132734) and eliminating double call of ifnet_byindex() was the first thing I did. But I decided then that the proper fix would be to wrap all critical code in sysctl_ifdata in IFNET_RLOCK/IFNET_RUNLOCK (the patch is attached). It looks like I am wrong and my idea about how the kernel works is oversimplified? :-) Unfortunately, I didn't manage to reproduce the panic in my environment so I was not able to do some experiments and tests. -- Mikolaj Golub --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=if_mib.c.patch --- sys/net/if_mib.c.orig 2009-04-21 23:19:54.000000000 +0300 +++ sys/net/if_mib.c 2009-04-21 23:20:07.000000000 +0300 @@ -71,7 +71,7 @@ static int sysctl_ifdata(SYSCTL_HANDLER_ARGS) /* XXX bad syntax! */ { int *name = (int *)arg1; - int error; + int error = 0; u_int namelen = arg2; struct ifnet *ifp; struct ifmibdata ifmd; @@ -81,15 +81,21 @@ sysctl_ifdata(SYSCTL_HANDLER_ARGS) /* XX if (namelen != 2) return EINVAL; - if (name[0] <= 0 || name[0] > if_index || - ifnet_byindex(name[0]) == NULL) + if (name[0] <= 0 || name[0] > if_index) return ENOENT; + IFNET_RLOCK(); ifp = ifnet_byindex(name[0]); + if (ifp == NULL) { + IFNET_RUNLOCK(); + return ENOENT; + } + switch(name[1]) { default: - return ENOENT; + error = ENOENT; + break; case IFDATA_GENERAL: bzero(&ifmd, sizeof(ifmd)); @@ -105,12 +111,12 @@ sysctl_ifdata(SYSCTL_HANDLER_ARGS) /* XX ifmd.ifmd_snd_drops = ifp->if_snd.ifq_drops; error = SYSCTL_OUT(req, &ifmd, sizeof ifmd); - if (error || !req->newptr) - return error; + if (error || !req->newptr) + break; error = SYSCTL_IN(req, &ifmd, sizeof ifmd); if (error) - return error; + break; #define DONTCOPY(fld) ifmd.ifmd_data.ifi_##fld = ifp->if_data.ifi_##fld DONTCOPY(type); @@ -130,18 +136,20 @@ sysctl_ifdata(SYSCTL_HANDLER_ARGS) /* XX case IFDATA_LINKSPECIFIC: error = SYSCTL_OUT(req, ifp->if_linkmib, ifp->if_linkmiblen); - if (error || !req->newptr) - return error; + if (error || !req->newptr) + break; error = SYSCTL_IN(req, ifp->if_linkmib, ifp->if_linkmiblen); if (error) - return error; + break; case IFDATA_DRIVERNAME: /* 20 is enough for 64bit ints */ dlen = strlen(ifp->if_dname) + 20 + 1; - if ((dbuf = malloc(dlen, M_TEMP, M_NOWAIT)) == NULL) - return (ENOMEM); + if ((dbuf = malloc(dlen, M_TEMP, M_NOWAIT)) == NULL) { + error = ENOMEM; + break; + } if (ifp->if_dunit == IF_DUNIT_NONE) strcpy(dbuf, ifp->if_dname); else @@ -151,9 +159,10 @@ sysctl_ifdata(SYSCTL_HANDLER_ARGS) /* XX if (error == 0 && req->newptr != NULL) error = EPERM; free(dbuf, M_TEMP); - return (error); + break; } - return 0; + IFNET_RUNLOCK(); + return error; } SYSCTL_NODE(_net_link_generic, IFMIB_IFDATA, ifdata, CTLFLAG_RW, --=-=-=--