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

[Luis Chamberlain <mcgrof@kernel.org>]

On Tue, May 25, 2021 at 09:41:26AM +0200, Greg Kroah-Hartman wrote:
> On Tue, May 25, 2021 at 01:16:07AM +0000, Luis Chamberlain wrote:
> > 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.

I didn't say it was easy. I meant that we support it and we can consider
its support as well.

> > 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.

I went back to the drawing board with this in mind. But a few things to
note:

The issue of the deadlock does not imply a lock has to be global. So long
as the rmmod path uses a lock which is also used by sysfs files, you can end
up in a deadlock.

Despite this, I tried to remove the global lock on the zram driver, however it
just doesn't seem right to remove it, its being used to help protect a
generic state machine and a global lock seems perfectly reasonable for the
driver.

If you still believe the global is not needed, let me know what you come
up with as an alternative. I just can't find a clean way to do away with
that, *and*, I still also think this pattern might be prevalent in the
kernel in different places. I am not sure we can set as generic rule:

  "Thou Shalt Not use the same lock on rmmod and sysfs files"

I'm moving forward in my v3 series by keeping it and instead now
providing module device attribute wrappers. The idea with this is,
that if we are not yet sure what to do yet, we can at least have
drivers integrate these helpers as well when and if they find they need
a similar solution.

  Luis