Re: [PATCH v2 0/4] zram: fix few sysfs races

[Greg Kroah-Hartman]

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