From: Linus Torvalds <torvalds@linux-foundation.org>
To: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Rusty Russell <rusty@rustcorp.com.au>,
LKML <linux-kernel@vger.kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
Brandon Philips <brandon@ifup.org>
Subject: Re: [Regression] Crash in load_module() while freeing args
Date: Tue, 25 May 2010 16:47:32 -0700 (PDT) [thread overview]
Message-ID: <alpine.LFD.2.00.1005251639420.3689@i5.linux-foundation.org> (raw)
In-Reply-To: <201005260054.52122.rjw@sisk.pl>
On Wed, 26 May 2010, Rafael J. Wysocki wrote:
>
> I'm not able to reproduce the issue with the following commit reverted:
>
> commit 480b02df3aa9f07d1c7df0cd8be7a5ca73893455
> Author: Rusty Russell <rusty@rustcorp.com.au>
> Date: Wed May 19 17:33:39 2010 -0600
>
> module: drop the lock while waiting for module to complete initialization.
Hmm. That does seem to be buggy. We can't just drop and re-take the lock:
that may make sense _internally_ as far as resolve_symbol() itself is
concerned, but the caller will its own local variables, and some of those
will no longer be valid if the lock was dropped.
That commit also changes the return value semantics of "use_module()",
which is an exported interface where the only users seem to be
out-of-kernel (the only in-kernel use is in kernel/module.c itself). That
seems like a really really bad idea too.
So I think reverting it is definitely the right thing to do. The commit
seems fundamentally broken. And having modules do request_module() in
their init functions has always been invalid anyway, so that excuse
doesn't really seem to be a reason to do anything crazy like this either.
Rewriting the logic to
- not drop the lock
- not change the return semantics of an exported interface
- just make 'resolve_symbol()' fail if the module isn't fully loaded
would seem to be a more reasonable approach, no?
Linus
next prev parent reply other threads:[~2010-05-25 23:51 UTC|newest]
Thread overview: 71+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-05-25 21:00 Rafael J. Wysocki
2010-05-25 22:54 ` Rafael J. Wysocki
2010-05-25 23:47 ` Linus Torvalds [this message]
2010-05-26 8:00 ` Rusty Russell
2010-05-26 11:57 ` Rusty Russell
2010-05-26 22:56 ` Rafael J. Wysocki
2010-05-26 23:07 ` Linus Torvalds
2010-05-27 5:26 ` Rusty Russell
2010-05-27 18:46 ` Brandon Philips
2010-05-31 9:40 ` Rusty Russell
2010-05-31 12:00 ` [PATCH 0/2] kernel/module.c locking changes Rusty Russell
2010-05-31 12:01 ` [PATCH 1/2] module: make locking more fine-grained Rusty Russell
2010-05-31 12:02 ` [PATCH 2/2] module: fix bne2 "gave up waiting for init of module libcrc32c" Rusty Russell
2010-05-31 16:48 ` Andrew Morton
2010-05-31 18:19 ` Linus Torvalds
2010-05-31 20:15 ` Linus Torvalds
2010-05-31 20:16 ` [PATCH 1/2] Make the module 'usage' lists be two-way Linus Torvalds
2010-05-31 20:17 ` [PATCH 2/2] module: wait for other modules after dropping the module_mutex Linus Torvalds
2010-06-01 1:37 ` [PATCH 1/2] Make the module 'usage' lists be two-way Rusty Russell
2010-06-01 3:42 ` Rusty Russell
2010-06-01 4:00 ` Linus Torvalds
2010-06-01 4:05 ` Linus Torvalds
2010-06-01 2:44 ` Américo Wang
2010-06-01 3:51 ` Linus Torvalds
2010-06-01 1:57 ` [PATCH 2/2] module: fix bne2 "gave up waiting for init of module libcrc32c" Rusty Russell
2010-06-01 3:40 ` Linus Torvalds
2010-06-01 4:27 ` Linus Torvalds
2010-06-01 5:19 ` Rusty Russell
2010-06-02 3:15 ` Rusty Russell
2010-06-01 1:21 ` Rusty Russell
2010-06-01 3:24 ` Linus Torvalds
2010-06-01 5:22 ` Rusty Russell
2010-06-01 14:58 ` Linus Torvalds
2010-06-01 17:53 ` Linus Torvalds
2010-06-01 23:24 ` Brandon Philips
2010-06-01 23:51 ` Linus Torvalds
2010-06-02 2:10 ` Brandon Philips
2010-06-02 3:03 ` Rusty Russell
2010-06-02 4:35 ` Linus Torvalds
2010-06-02 4:44 ` Linus Torvalds
2010-06-02 6:35 ` Rusty Russell
2010-06-02 7:45 ` Linus Torvalds
2010-06-02 8:12 ` Linus Torvalds
2010-06-02 9:07 ` Rusty Russell
2010-06-02 5:52 ` Rusty Russell
2010-06-02 7:21 ` Linus Torvalds
2010-06-02 14:06 ` Rusty Russell
2010-06-02 14:50 ` Linus Torvalds
2010-06-03 13:06 ` Rusty Russell
2010-06-02 16:53 ` Brandon Philips
2010-06-02 18:01 ` Linus Torvalds
2010-06-03 5:20 ` Rusty Russell
2010-06-03 16:24 ` Linus Torvalds
2010-06-04 1:02 ` Rusty Russell
2010-06-04 1:55 ` Linus Torvalds
2010-06-04 5:20 ` Rusty Russell
2010-06-04 22:48 ` Linus Torvalds
2010-06-05 1:49 ` Rusty Russell
2010-06-02 3:09 ` Rusty Russell
2010-06-02 4:32 ` Linus Torvalds
2010-06-02 4:56 ` Linus Torvalds
2010-06-02 5:52 ` Rusty Russell
2010-06-02 6:59 ` Linus Torvalds
2010-06-01 1:04 ` Rusty Russell
2010-06-01 5:38 ` [PATCH 1/2] module: make locking more fine-grained Américo Wang
2010-06-01 5:55 ` Rusty Russell
2010-05-27 21:57 ` [Regression] Crash in load_module() while freeing args Rafael J. Wysocki
2010-05-31 7:54 ` Rusty Russell
2010-05-31 10:23 ` [PATCH] module: fix reference to mod->percpu after freeing module Rusty Russell
2010-05-31 10:25 ` Tejun Heo
2010-05-26 15:41 ` [Regression] Crash in load_module() while freeing args Linus Torvalds
Reply instructions:
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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LFD.2.00.1005251639420.3689@i5.linux-foundation.org \
--to=torvalds@linux-foundation.org \
--cc=akpm@linux-foundation.org \
--cc=brandon@ifup.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rjw@sisk.pl \
--cc=rusty@rustcorp.com.au \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* 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
all inboxes | Powered by JetHome®