Re: [PATCH 1/2] zram: fix crashes due to use of cpu hotplug multistate

[Luis Chamberlain]

On Tue, Apr 06, 2021 at 06:23:46PM -0700, Minchan Kim wrote:
> On Tue, Apr 06, 2021 at 12:29:09AM +0000, Luis Chamberlain wrote:
> > On Mon, Apr 05, 2021 at 12:58:05PM -0700, Minchan Kim wrote:
> > > On Mon, Apr 05, 2021 at 07:00:23PM +0000, Luis Chamberlain wrote:
> > > > Which is why the *try_module_get()* I think is much more suitable, as
> > > > it will always fails if we're already going down.
> > > 
> > > How does the try_module_get solved the problem?
> > 
> > The try stuff only resolves the deadlock. The bget() / bdput() resolves
> > the race to access to the gendisk private_data.
> 
> That's the one I missed in this discussion. Now I am reading your [2/2]
> in original patch. I thought it was just zram instance was destroyed
> by sysfs race problem so you had seen the deadlock.

Patch [2/2] indeed dealt with a zram instance race but the issue was not
a crash due to indirection, it was because of a deadlock. The deadlock
happens because a shared mutex is used both at sysfs and also on __exit.
I'll expand on the example as you request so that both issues are
clearly understood.

The zram race you spotted which could lead to a derefence and crash is
a separate one, which the bget() / bdput() on sysfs knobs resolves. That
race happens because zram's sysfs knobs don't prevent del_gendisk() from
completing currently, and so a dereference can happen.

> Hmm, we are discussing several problems all at once. I feel it's time
> to jump v2 with your way in this point. You said three different
> problems. As I asked, please write it down with more detail with
> code sequence as we discussed other thread. If you mean a deadlock,
> please write what specific locks was deadlock with it.
> It would make discussion much easier. Let's discuss the issue
> one by one in each thread.

Sure. Will post a v2.

> > But so far Greg does not see enough justification for a), so we can either
> > show how wider this issue is (which I can do using coccinelle), or we
> > just open code the try_module_get() / put on each driver that needs it
> > for now. Either way it would resolve the issue.
> 
> I second if it's general problem for drivers, I agree it's worth to
> addresss in the core unless the driver introduce the mess. I have
> no idea here since I didn't understand the problem, yet.

I suspect these issue are generic, however hard to reproduce, but this
just means busying out sysfs knobs and doing module removal can likely
cause crashes to some kernel drivers.

Since it seems the position to take is module removal is best effort,
if crashes on module removal are important to module maintainers, the
position to take is that such races are best addressed on the driver
side, not core.

> > As for b), given that I think even you had missed my attempt to
> > generialzie the bdget/bdput solution for any attribute type (did you see
> > my dev_type_get() and dev_type_put() proposed changes?), I don't think
> > this problem is yet well defined in a generic way for us to rule it out.
> > It is however easier to simply open code this per driver that needs it
> > for now given that I don't think Greg is yet convinced the deadlock is
> > yet a widespread issue. I however am pretty sure both races races *do*
> > exist outside of zram in many places.
> 
> It would be good sign to propose general solution.

Same situation here, as the race is with module removal.

Even in the case where module removal is possible today and likely
"supported" -- on livepatching, it seems we're shying away slowly from
that situation, and will likely expose a knob to ensure removing a
livepatch (and do module removal) will require a knob to be flipped
to say, "Yes, I know what I am doing").

> > They try_module_get() approach resolves the deadlock race, but it does
> > so in a lazy way. I mean lazy in that then rmmod wins over sysfs knobs.
> > So touching sysfs knobs won't make an rmmod fail. I think that's more
> > typical expected behaviour. Why? Because I find it odd that looping
> > forever touching sysfs attributes should prevent a module removal. But
> > that's a personal preference.
> 
> I agree with you that would be better but let's see how it could go
> clean.

OK great.

> FYI, please look at hot_remove_store which also can remove zram
> instance on demand. I am looking forward to seeing your v2.

Sure, I'll address all sysfs knobs in v2.

  Luis