cvs commit: src/sys/sys firmware.h src/sys/kern subr_firmware.c
jhb at freebsd.org
Fri Jun 16 18:57:38 UTC 2006
On Thursday 15 June 2006 20:49, Ian Dowse wrote:
> 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.
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.
More information about the cvs-src