On Fri, Nov 17, 2023 at 05:06:16PM -0500, Greg Kroah-Hartman wrote: > On Fri, Nov 17, 2023 at 10:58:59PM +0100, Marco Pagani wrote: > > > > > > On 2023-11-14 07:53, Xu Yilun wrote: > > > On Fri, Nov 10, 2023 at 11:58:37PM +0100, Marco Pagani wrote: > > >> > > >> > > >> On 2023-11-08 16:52, Xu Yilun wrote: > > >>> On Fri, Nov 03, 2023 at 09:31:02PM +0100, Marco Pagani wrote: > > >>>> > > >>>> > > >>>> On 2023-10-30 09:32, Xu Yilun wrote: > > >>>>> On Fri, Oct 27, 2023 at 05:29:27PM +0200, Marco Pagani wrote: > > >>>>>> Remove unnecessary module reference counting from the core components > > >>>>>> of the subsystem. Low-level driver modules cannot be removed before > > >>>>>> core modules since they use their exported symbols. > > >>>>> > > >>>>> Could you help show the code for this conclusion? > > >>>>> > > >>>>> This is different from what I remember, a module cannot be removed when > > >>>>> its exported symbols are being used by other modules. IOW, the core > > >>>>> modules cannot be removed when there exist related low-level driver > > >>>>> modules. But the low-level driver modules could be removed freely > > >>>>> without other protecting mechanism. > > >>>>> > > >>>> > > >>>> My understanding was that we wanted to remove module reference counting > > >>>> from the fpga core and ease it from the responsibility of preventing > > >>>> low-level driver modules from being unloaded. > > >>> > > >>> FPGA core needs to prevent low-level driver module unloading sometimes, > > >>> e.g. when region reprograming is in progress. That's why we get fpga > > >>> region driver modules & bridge modules in fpga_region_program_fpga(). > > >>> > > >>> But we try best to get them only necessary. Blindly geting them all the > > >>> time results in no way to unload all modules (core & low level modules). > > >>> > > >>>> > > >>>> If we want to keep reference counting in the fpga core, we could add a > > >>>> struct module *owner field in the struct fpga_manager_ops (and others > > >>>> core *_ops) so that the low-level driver can set it to THIS_MODULE. > > >>>> In this way, we can later use it in fpga_mgr_register() to bump up the > > >>> > > >>> Yes, we should pass the module owner in fpga_mgr_register(), but could > > >>> not bump up its refcount at once. > > >>> > > >>>> refcount of the low-level driver module by calling > > >>>> try_module_get(mgr->mops->owner) directly when it registers the manager. > > >>>> Finally, fpga_mgr_unregister() would call module_put(mgr->mops->owner) > > >>>> to allow unloading the low-level driver module. > > >>> > > >>> As mentioned above, that makes problem. Most of the low level driver > > >>> modules call fpga_mgr_unregister() on module_exit(), but bumping up > > >>> their module refcount prevents module_exit() been executed. That came > > >>> out to be a dead lock. > > >>> > > >> > > >> Initially, I considered calling try_module_get(mgr->mops->owner) > > >> in fpga_mgr_get(). But then, the new kernel-doc description of > > >> try_module_get() (1) made me question the safety of that approach. > > >> My concern is that the low-level driver could be removed right when > > >> someone is calling fpga_mgr_get() and hasn't yet reached > > >> try_module_get(mgr->mops->owner). In that case, the struct mops > > >> (along with the entire low-level driver module) and the manager dev > > >> would "disappear" under the feet of fpga_mgr_get(). > > > > > > I don't get what's the problem. fpga_mgr_get() would first of all > > > look for mgr_dev via class_find_device(), if low-level module is > > > unloaded, then you cannot find the mgr_dev and gracefully error out. > > > > > > If class_find_device() succeed, mgr_dev got a reference and won't > > > disappear. Finally we may still found module removed when > > > try_module_get(), but should be another graceful error out. > > > > > > Am I missing anything? > > > > > > > My concern is: suppose that you successfully got the mgr dev from > > class_find_device(), and now you are in __fpga_mgr_get(), right before > > try_module_get(mgr->mops->owner). At that point, you get descheduled, > > and while you are not running, someone unloads the low-level driver > > module that ends its life by calling fpga_mgr_unregister(). When you > > wake up, you find yourself with a reference to a device that does not > > exist anymore, trying to get a module that does not exist anymore I may get the problem. The mgr device is still exists, but the module is removed, so the mgr->mops & owner pointers are invalid.. > > through one of its symbols (module *owner in mops). > > Then the user gets to keep the multiple pieces that their kernel is now > in :) > > Seriously, as module unload can never happen except by explicit ask, > this should only possibly be an issue that a developer who is working on > the code would hit, so don't work too hard to resolve something that > isn't anything an actual user can hit. > > > Greg suggested checking if this can really happen and eventually > > protecting fpga_mgr_get() and fpga_mgr_unregister() with a lock for > > mops (if I understood correctly). In that case, considering the same > > scenario described above: > > > > fpga_mgr_get() gets the mops lock and the mgr dev but is suspended > > before calling try_module_get(). > > > > Someone unloads the low-level driver, delete_modules progresses > > (the module's recount hasn't yet been incremented) but blocks while > > calling fpga_mgr_unregister() since fpga_mgr_get() is holding the lock. > > > > fpga_mgr_get() resumes and tries to get the module through one of its > > symbols (mgr->mops->owner). The module's memory hasn't yet been freed > > (delete_modules is blocked), and the refcount is zero, so > > try_module_get() fails safely, and we can put the mgr dev that is > > still present since fpga_mgr_unregister() is blocked. > > > > fpga_mgr_unregister() resumes and unregisters the mgr dev. > > That seems a bit reasonable, try it and see! It also looks good to me. Thanks, Yilun > > > I'm still thinking about the possible implications. On the one hand, > > it looks safe in this case, but on the other hand, it feels brittle. > > In my understanding, the root problem is that there will always be a > > critical window (when you have taken a reference to the device but > > not yet to the low-level driver module) when unloading the module > > could be potentially unsafe depending on the current implementation > > and the preemption model. > > > > I still feel that it would be simpler and safer if we could bump > > up the refcount during fpga_mgr_register() and maybe have a sysfs > > attribute to unlock the low-level driver (if no one has taken the > > mgr dev refcount). > > Ick, no, that shouldn't be needed. > > > That way, it would be safer by design since the > > refcount will be bumped up right during the module load procedure, > > and we could guarantee that the lifetime of the mgr device is > > entirely contained in the lifetime of the low-level driver module. > > Remember, there are two different things here, code and data. Trying to > tie one ref count to the other is almost always going to cause problems, > try to keep them independent if at all possible. > > Or better yet, just don't use module reference counts at all and > properly drop the device when the specific module is unloaded, like > network drivers do. That might take more work to restructure things, > which might be useless work given that again, this is something that no > user will ever hit, only developers if at all. > > thanks, > > greg k-h
