On Tue, May 25, 2021 at 01:16:07AM +0000, Luis Chamberlain wrote: > On Sat, May 22, 2021 at 09:48:34AM +0200, Greg Kroah-Hartman wrote: > > On Fri, May 21, 2021 at 09:08:17PM +0000, Luis Chamberlain wrote: > > > On Fri, May 21, 2021 at 10:45:00PM +0200, Greg Kroah-Hartman wrote: > > > > I looked at the last patch here and I really do not see the issue. > > > > > > > > In order for the module to be removed, zram_exit() has to return, right? > > > > > > Yes, but the race is for when a module removal is ongoing, in other > > > words, it has not yet completed, and at the same time we race touching > > > sysfs files. > > > > > > > So how can a show/store function in zram_drv.c be called after > > > > destroy_devices() returns? > > > > > > The issue can come up if we have something poke at the sysfs files *while* a > > > removal is happening. > > > > And have you seen this in the real world? I keep asking this as module > > removal is not an automated process so what triggers this? > > No, its not seen in the real world. It was theoretical, and noted as > possible by Minchan Kim. I reviewed it, and I agree the race is > possible. Ok, then really, it's not a big deal :) > > Why do you feel that block devices are somehow more "special" here? > > I am not saying they are. Your patch made them "special", don't do that. > > They are not, either this is "broken" for everyone, or it works for > > everyone, don't special-case one tiny class of devices for unknown > > reasons. > > The reason dev_type_get() was implemented was precisely to allow for > this to be expanded with the other types as they the *get* is specific to > the type. No, that's the wrong thing to do. > > Your change causes another problem, if a sysfs file has show/store > > happening, the reference count will always be bumped and so the module > > would NOT be able to be freed. That looks like a lovely DoS that any > > user could cause, right? > > Yes true. I think the better way to resolve that is to introduce and use > *try* methods, and so rmmod always trumps a new *get* for these > operations. No, "try" methods suck, as the Yoda quote says. > That would sole the possible "DOS" issue, precisely how I resolved this > same concern for resolving the deadlock with try_module_get(). Should not be needed. > > In sleeping on this > > Sorry, did you mean you thought about this, or you meant sleep as in > the sleep context? I thought about this, sorry for the confusion. > > So in conclusion, the "correct" thing here seems to be two independant > > things: > > - make sure the reference count of the kobject is properly > > incremented during show/store callbacks > > - grab the kobject's type/bus/whatever lock during show/store so > > that it can not race with deleting the device. > > Yup. The above was a proof of concept solution using type, but indeed, > the downside is we'd have to implement try methods when not found, and > likely the list of types is endless. > > Are there places where we cannot use the bus? Not that I know of, all objects live on some sort of "type/bus", that's the way the driver model works. > > No bus/type should be special cased here, block devices are not special > > by any means. > > > > And don't mess with module reference counts, that way lies madness. We > > want to lock data, not code :) > > Live patching needs to lock code ;) and hey it works ;) Live patching is vodoo magic. But it just "adds" code paths, and later, when it feels all is good, then it can remove stuff (if it even does, I do not remember). Adding is easy, removing is hard. > Addressing the kobject refecount here should in theory address most > deadlocks (what my third patch addresses) as well becuase, as you imply, > our protection of the kobject should prevent removal, but that's not > always the case. I think you're failing to consider a shared global > driver lock, which can be used on sysfs files, which in turn have > *nothing* kref'd. And so the module removal can still try to nuke sysfs > files, if those sysfs files like to mess with the shared global driver > lock. If any driver has that kind of crud, they deserve the nightmare that would happen if it interacts this way. Don't worry about that, it's not a pattern that anyone should be using. And again, if the code and data is still there, the lock is ok to grab, there should not be a problem. If so, we can fix the driver. thanks, greg k-h
