On Tue, 26 Oct 2021, Luis Chamberlain wrote: > On Tue, Oct 26, 2021 at 11:37:30PM +0800, Ming Lei wrote: > > On Tue, Oct 26, 2021 at 10:48:18AM +0200, Petr Mladek wrote: > > > Livepatch code never called kobject_del() under a lock. It would cause > > > the obvious deadlock. > > Never? kobject_put() to be precise. When I started working on the support for module/live patches removal, calling kobject_put() under our klp_mutex lock was the obvious first choice given how the code was structured, but I ran into problems with deadlocks immediately. So it was changed to async approach with the workqueue. Thus the mainline code has never suffered from this, but we knew about the issues. > > > The historic code only waited in the > > > module_exit() callback until the sysfs interface was removed. > > > > OK, then Luis shouldn't consider livepatching as one such issue to solve > > with one generic solution. > > It's not what I was told when the deadlock was found with zram, so I was > informed quite the contrary. >From my perspective, it is quite easy to get it wrong due to either a lack of generic support, or missing rules/documentation. So if this thread leads to "do not share locks between a module removal and a sysfs operation" strict rule, it would be at least something. In the same manner as Luis proposed to document try_module_get() expectations. > I'm working on a generic coccinelle patch which hunts for actual cases > using iteration (a feature of coccinelle for complex searches). The > search is pretty involved, so I don't think I'll have an answer to this > soon. > > Since the question of how generic this deadlock is remains questionable, > I think it makes sense to put the generic deadlock fix off the table for > now, and we address this once we have a more concrete search with > coccinelle. > > But to say we *don't* have drivers which can cause this is obviously > wrong as well, from a cursory search so far. But let's wait and see how > big this list actually is. > > I'll drop the deadlock generic fixes and move on with at least a starter > kernfs / sysfs tests. It makes sense to me. Thanks, Luis, for pursuing it. Miroslav
