From owner-svn-src-all@freebsd.org Fri May 13 18:46:22 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id A5689B3A7E6; Fri, 13 May 2016 18:46:22 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 8697E1A21; Fri, 13 May 2016 18:46:22 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 9E4EEB990; Fri, 13 May 2016 14:46:21 -0400 (EDT) From: John Baldwin To: Scott Long Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r299590 - head/sys/dev/bxe Date: Fri, 13 May 2016 11:38:05 -0700 Message-ID: <4407587.3MdJrGvffs@ralph.baldwin.cx> User-Agent: KMail/4.14.3 (FreeBSD/10.2-STABLE; KDE/4.14.3; amd64; ; ) In-Reply-To: <201605130557.u4D5vMBZ015636@repo.freebsd.org> References: <201605130557.u4D5vMBZ015636@repo.freebsd.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Fri, 13 May 2016 14:46:21 -0400 (EDT) X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 May 2016 18:46:22 -0000 On Friday, May 13, 2016 05:57:22 AM Scott Long wrote: > Author: scottl > Date: Fri May 13 05:57:21 2016 > New Revision: 299590 > URL: https://svnweb.freebsd.org/changeset/base/299590 > > Log: > Don't jam the softc in the device_probe routine. The softc isn't owned by > the driver here, so it shouldn't be accessed, let alone written to. Remove > the nearby debug line, it's the only thing that depended on the softc, and > it depended on it in a way that couldn't work in this part of the code. > > This fixes some reports of use-after-free and system instability with > DEBUG_MEMGUARD enabled. Actually, softcs are perfectly valid for use in probe routines. They are just destroyed on exit. The softc is allocated by device_set_driver() (and the previous softc is freed when device_set_driver() is called). You can see the calls to 'device_set_driver' (which also set the device name so that device_printf() works in probe routines) in device_probe_child() (the thing that calls DEVICE_PROBE()): int device_probe_child(device_t dev, device_t child) { ... for (; dc; dc = dc->parent) { for (dl = first_matching_driver(dc, child); dl; dl = next_matching_driver(dc, child, dl)) { ... PDEBUG(("Trying %s", DRIVERNAME(dl->driver))); result = device_set_driver(child, dl->driver); ... result = DEVICE_PROBE(child); ... if (result > 0) { (void)device_set_driver(child, NULL); continue; } ... } ... } ... if (best) { ... result = device_set_driver(child, best->driver); } ... } In the main loop, the 'device_set_driver()' before DEVICE_PROBE() frees the softc from the previous probe attempt. If at least one working driver is found, then best will be non-null and the last 'device_set_driver()' will free the softc from the last probe attempt and allocate a fresh one to be used when DEVICE_ATTACH() is invoked. If no driver was valid, then we would have called the 'device_set_driver(child, NULL)' case after every probe. Looking at the code, it would probably be clearer to just move that case into an 'else' clause at the end for the 'best == NULL' case. However, just touching the softc in a probe routine cannot possibly cause a memory leak. device_get_softc() does not allocate the softc on demand, it just returns whatever is already allocated. While using the softc in probe routines is often dubious, the commit is wrong in that this cannot cause memory leaks or use after free. -- John Baldwin