cvs commit: src/sys/sys firmware.h src/sys/kern subr_firmware.c

[John Baldwin]

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.

-- 
John Baldwin