From: Luis Chamberlain <email@example.com>
To: Minchan Kim <firstname.lastname@example.org>
Cc: email@example.com, firstname.lastname@example.org, email@example.com,
firstname.lastname@example.org, email@example.com, firstname.lastname@example.org,
Subject: Re: [PATCH 1/2] zram: fix crashes due to use of cpu hotplug multistate
Date: Wed, 7 Apr 2021 14:50:11 +0000 [thread overview]
Message-ID: <20210407145011.GC4332@42.do-not-panic.com> (raw)
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
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
> 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.
next prev parent reply other threads:[~2021-04-07 14:50 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-06 2:20 [PATCH 0/2] zram: fix few ltp zram02.sh crashes Luis Chamberlain
2021-03-06 2:20 ` [PATCH 1/2] zram: fix crashes due to use of cpu hotplug multistate Luis Chamberlain
2021-03-09 2:55 ` Minchan Kim
2021-03-10 13:11 ` Luis Chamberlain
2021-03-10 21:25 ` Luis Chamberlain
2021-03-12 2:08 ` Minchan Kim
2021-03-10 21:21 ` Luis Chamberlain
2021-03-12 2:14 ` Minchan Kim
2021-03-12 18:32 ` Luis Chamberlain
2021-03-12 19:28 ` Minchan Kim
2021-03-19 19:09 ` Luis Chamberlain
2021-03-22 16:37 ` Minchan Kim
2021-03-22 20:41 ` Luis Chamberlain
2021-03-22 22:12 ` Minchan Kim
2021-04-01 23:59 ` Luis Chamberlain
2021-04-02 7:54 ` Greg KH
2021-04-02 18:30 ` Luis Chamberlain
2021-04-03 6:13 ` Greg KH
[not found] ` <20210406003152.GZ4332@42.do-not-panic.com>
2021-04-06 12:00 ` Miroslav Benes
2021-04-06 15:54 ` Josh Poimboeuf
2021-04-07 14:09 ` Peter Zijlstra
2021-04-07 15:30 ` Josh Poimboeuf
2021-04-07 16:48 ` Peter Zijlstra
2021-04-07 20:17 ` Josh Poimboeuf
2021-04-08 6:18 ` Greg KH
2021-04-08 13:16 ` Steven Rostedt
2021-04-08 13:37 ` Josh Poimboeuf
2021-04-08 1:37 ` Thomas Gleixner
2021-04-08 6:16 ` Greg KH
2021-04-08 8:01 ` Jiri Kosina
2021-04-08 8:09 ` Greg KH
2021-04-08 8:35 ` Jiri Kosina
2021-04-08 8:55 ` Greg KH
2021-04-08 18:40 ` Luis Chamberlain
2021-04-09 3:01 ` Kees Cook
2021-04-05 17:07 ` Minchan Kim
2021-04-05 19:00 ` Luis Chamberlain
2021-04-05 19:58 ` Minchan Kim
2021-04-06 0:29 ` Luis Chamberlain
2021-04-07 1:23 ` Minchan Kim
2021-04-07 1:38 ` Minchan Kim
2021-04-07 14:52 ` Luis Chamberlain
2021-04-07 14:50 ` Luis Chamberlain [this message]
2021-03-06 2:20 ` [PATCH 2/2] zram: fix races of sysfs attribute removal and usage Luis Chamberlain
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).