cvs commit: src/sys/sys firmware.h src/sys/kern subr_firmware.c
iedowse at iedowse.com
Fri Jun 16 00:49:47 UTC 2006
In message <200606130856.55255.jhb at 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.
More information about the cvs-src