Messages in this thread | | | Date | Tue, 25 May 2021 09:41:26 +0200 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH v2 0/4] zram: fix few sysfs races |
| |
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
| |