From owner-cvs-src@FreeBSD.ORG Fri Jun 16 18:57:38 2006 Return-Path: X-Original-To: cvs-src@freebsd.org Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 14F3316A47C; Fri, 16 Jun 2006 18:57:38 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from server.baldwin.cx (66-23-211-162.clients.speedfactory.net [66.23.211.162]) by mx1.FreeBSD.org (Postfix) with ESMTP id 041A343D48; Fri, 16 Jun 2006 18:57:36 +0000 (GMT) (envelope-from jhb@freebsd.org) Received: from localhost.corp.yahoo.com (john@localhost [127.0.0.1]) (authenticated bits=0) by server.baldwin.cx (8.13.4/8.13.4) with ESMTP id k5GIvU8o060257; Fri, 16 Jun 2006 14:57:35 -0400 (EDT) (envelope-from jhb@freebsd.org) From: John Baldwin To: Ian Dowse Date: Fri, 16 Jun 2006 14:57:22 -0400 User-Agent: KMail/1.9.1 References: <200606160149.aa83404@nowhere.iedowse.com> In-Reply-To: <200606160149.aa83404@nowhere.iedowse.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200606161457.23420.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [127.0.0.1]); Fri, 16 Jun 2006 14:57:35 -0400 (EDT) X-Virus-Scanned: ClamAV 0.87.1/1548/Fri Jun 16 13:53:47 2006 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=4.2 tests=ALL_TRUSTED,BAYES_00 autolearn=ham version=3.1.0 X-Spam-Checker-Version: SpamAssassin 3.1.0 (2005-09-13) on server.baldwin.cx Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/sys firmware.h src/sys/kern subr_firmware.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 16 Jun 2006 18:57:38 -0000 On Thursday 15 June 2006 20:49, Ian Dowse wrote: > In message <200606130856.55255.jhb@freebsd.org>, John Baldwin writes: > >On Monday 12 June 2006 20:50, Ian Dowse wrote: > >> That would bring back the original issue where a manually kldloaded > >> firmware image would be removed from the list when a driver calls > >> firmware_put(), even though the kld will remain loaded; there is > >> nothing that a driver can do to get the entry back on the list since > >> calling linker_reference_module() will not result in a call to > >> firmware_register() because the module is already (manually) loaded. > > > >No it wouldn't. :) unloadentry() is only called when we are unloading > >an explicitly loaded module from the taskqueue. That is where I think > >the 'fp->file = NULL' should be changed to 'clearentry()'. Either > >that or don't clear fp->file at all. > > Sorry for the delay in replying. True, but clearing the entry is > still assuming that there are no other linker references, and > firmware(9) doesn't really know this. For example if you manually > load a module that depends on a firmware image module, then the > firmware image module will stay loaded after unloadentry() calls > linker_file_unload(), so clearing the entry then would cause future > firmware_get() calls to fail. But unloadentry() would never unload such a module because fp->file is NULL. unloadentry() would only call clearentry() and then linker_file_unload() on an explicitly loaded firmware module. > >No. You've cleared fp->file. This means that if the other thread gets > >a reference, the firmware_unregister() will fail, but now the kernel will > >never unload this file on a subsequent firmware_put() since it won't see > >that it was explicitly loaded by the kernel since fp->file == NULL. The > >awk script patch fixes a different race where kldunload would succeed > >even though there were open references and drivers would have pointers > >into unmapped memory (or possibly mapped to something else). > > In theory whatever code was attempting to unload the module will > get back an error return and will keep its original reference on > the linker file, so that same code can succeed if it tries to unload > again later. In practice I'm sure there are bugs here ;-) For > example, firmware(9) might want to attempt to avoid clearing fp->file > if its call to linker_file_unload() fails. The problem is that in unloadentry() you are the code trying to unload the module and you ignore the error return. :) I think actually though, the best fix is to just not clear fp->file at all. If the unload succeeds, then the clearentry() in firmware_unregister() will end up clearing fp->file. If the unload fails because another thread grabbed a reference, then fp->file will remain set so that later on when that other thread does a firmware_put() we will try to unload the module in unloadentry() again. -- John Baldwin