Date: Fri, 16 Jun 2006 01:49:44 +0100 From: Ian Dowse <iedowse@iedowse.com> To: John Baldwin <jhb@freebsd.org> 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 Message-ID: <200606160149.aa83404@nowhere.iedowse.com> In-Reply-To: Your message of "Tue, 13 Jun 2006 08:56:54 EDT." <200606130856.55255.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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. I guess I'm just looking at firmware(9) as three relatively independent pieces, so it seems that mixing them only complicate things (e.g. clearing the entire entry when setting fp->file to NULL): o When a firmware image module loads, it registers itself. When it unloads it unregisters itself. o When a driver does "get" on an image, the reference count is incremented, when it does "put" it is decremented. o When firmware(9) holds a reference on a firmware image module, fp->file is non-NULL, and when it doesn't hold a reference, fp->file is NULL >> Shouldn't this race be fixed by your other suggested change of >> having a firmware_unregister() failure preventing the image module >> from unloading? (I didn't realise it wasn't already checking) The >> firmware_unregister() function atomically checks for references and >> clears the full entry, so with your change there is no way for the >> module to be unloaded while a reference exists. > >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. Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200606160149.aa83404>