Make kthread_stop() not oops when passed a bad pointer
Subsystems (2)
PATCH 1/1 Make kthread_stop() not oops when passed a bad pointer
Due to some botched error handling code in a driver I was writing, I recently called kthread_stop(NULL). It's terribly exciting to discover that you've just oopsed while holding a mutex that's required in order to shut down. Make kthread_stop a little more robust against numbskulls like me.
kernel/kthread.c +3 -0
@@ -201,6 +201,9 @@ int kthread_stop(struct task_struct *k) {int ret; +if (!k ||IS_ERR (k)) +return -EINVAL ; +mutex_lock (&kthread_stop_lock);/* It could exit after stop_info.k set, but before wake_up_process. */
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
kernel/kthread.c int kthread_stop(struct task_struct *k) · line 209
Quoted 12 lines · Matthew Wilcox
kernel/kthread.c +3
-0@@ -201,6 +201,9 @@ int kthread_stop(struct task_struct *k) {int ret; +if (!k ||IS_ERR (k)) +return -EINVAL ; +mutex_lock (&kthread_stop_lock);/* It could exit after stop_info.k set, but before wake_up_process. */
Would
if (WARN_ON(!k || IS_ERR(k)))
be in order, or are there valid cases of passing an invalid pointer?
Signature · 3 lines
Stefan Richter -=====-==--- =--- --=-= http://arcgraph.de/sr/
Andrew Morton
maintainerQuoted 14 lines · On Tue, 5 Aug 2008 07:55:59 -0600 Matthew Wilcox <[email protected]> wrote:
Due to some botched error handling code in a driver I was writing, I recently called
kthread_stop(NULL). It's terribly exciting to discover that you've just oopsed while holding a mutex that's required in order to shut down. Makekthread_stopa little more robust against numbskulls like me.Signed-off-by Matthew Wilcox <[email protected]>kernel/kthread.c +0
-0
kernel/kthread.c int kthread_stop(struct task_struct *k) · line 208
Quoted 8 lines
kernel/kthread.c +3
-0@@ -201,6 +201,9 @@ int kthread_stop(struct task_struct *k) {int ret; +if (!k ||IS_ERR (k)) +return -EINVAL ; +mutex_lock (&kthread_stop_lock);
hrm.
a) as this is a programming error, the programmer would like it not to be hidden. A WARN_ON() would address that.
b) there are many instances of
if (p) kthread_stop(p);
which could now be simplified to
kthread_stop(p);
c) a) and b) are incompatible.
PATCH 1/1 Trivial cleanup
This is a trivial cleanup.
Anyone doesn't use it any more.
--- mm/mempolicy.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
mm/mempolicy.c +0 -0
@@ -808,7 +808,6 @@ static int migrate_to_node(struct mm_struct *mm, int
source, int dest,
int do_migrate_pages(struct mm_struct *mm,
const nodemask_t *from_nodes, const nodemask_t *to_nodes, int flags)
{
- LIST_HEAD(pagelist);
int busy = 0;
int err = 0;
nodemask_t tmp;
Signature · 7 lines
1.5.4.3 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH][migration] Trivial cleanup
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 4 lines · On Tuesday 05 August 2008 23:55:59 Matthew Wilcox wrote:
Make
kthread_stopa little more robust against numbskulls like me.Signed-off-by Matthew Wilcox <[email protected]>
Hi Willy,
I really do sympathize with your problem; but the quest is to figure out how to identify it before the code is run, not to put a non-orthogonal bandaid here which can hurt other cases.
How about a more ambitious "we've oopsed so break a mutex every 30 seconds of waiting" patch?
Quoted 2 lines
+ if (!k ||
IS_ERR(k))
+ return-EINVAL;
1) There's no reason that kthread_stop is uniquely difficult to use. Why pick on that one?
2) I know that kfree() handles NULL, but kthread_create/kthread_run never return NULL, unlike kmalloc().
3) If we really want to pass a failed kthread_create() through kthread_stop(), we should return PTR_ERR(k) here. But that should only be done if it made it harder for the callers to screw up, which I don't think it does.
4) After a successful kthread_run(), kthread_stop() will always return the value from the threadfn callback. ie. kthread_stop() doesn't ever fail. A simple semantic, which this patch breaks.
5) Covering up programmer errors is not good policy. I dislike WARN_ON() because an oops is much harder to miss. Painful for you, but The System Works.
Sorry,
Rusty.
Quoted 6 lines · Rusty Russell
I really do sympathize with your problem; but the quest is to figure out how to identify it before the code is run, not to put a non-orthogonal bandaid here which can hurt other cases.
How about a more ambitious "we've oopsed so break a mutex every 30 seconds of waiting" patch?
I was considering something more along the lines of "we've oopsed so find every mutex we own and release it".
Quoted 5 lines
Quoted 2 lines
+?????if (!k ||
IS_ERR(k))
+?????????????return-EINVAL;1) There's no reason that
kthread_stopis uniquely difficult to use. Why pick on that one?
It was the one I hit.
Quoted 2 lines
2) I know that
kfree()handles NULL, but kthread_create/kthread_run never return NULL, unlikekmalloc().
I'd kzalloc'd the memory structure, then rearranged the order of calls initialising it without rearranging the destructor.
Quoted 3 lines
3) If we really want to pass a failed
kthread_create()throughkthread_stop(), we should returnPTR_ERR(k)here. But that should only be done if it made it harder for the callers to screw up, which I don't think it does.
I'm actually really dubious about kthread_stop() returning a value at all. To me, returning an error implies that the function failed to do its job, ie the thread is still running. But that's not true; if it returns -EINVAL, it means the thread never ran. And why should the caller care? Only three callers of kthread_stop do anything with the return value. Two of them just put the value in a debug message, and the third one goes to the effort of passing the return value through three layers of function pointer calls only to have all the callers discard it.
Quoted 3 lines
4) After a successful
kthread_run(),kthread_stop()will always return the value from the threadfn callback. ie.kthread_stop()doesn't ever fail. A simple semantic, which this patch breaks.
Now I'm confused. kthread_stop isn't failing. It preserves the invariant that when it returns, the thread is no longer running.
Quoted 3 lines
5) Covering up programmer errors is not good policy. I dislike
WARN_ON()because an oops is much harder to miss. Painful for you, but The System Works.
I don't understand why we wouldn't want to be more robust here.
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 6 lines · On Wednesday 06 August 2008 22:07:04 Matthew Wilcox wrote:
Quoted 2 lines · Rusty Russell
How about a more ambitious "we've oopsed so break a mutex every 30 seconds of waiting" patch?
I was considering something more along the lines of "we've oopsed so find every mutex we own and release it".
Hmm, I don't think that's possible in general is it?
Quoted 4 lines
Quoted 2 lines
1) There's no reason that
kthread_stopis uniquely difficult to use. Why pick on that one?It was the one I hit.
Yes, I got that :) But if we're not about to sprinkle "if check_ptr(arg)" all through the kernel wherever someone can misuse a function.
Quoted 5 lines
Quoted 2 lines
2) I know that
kfree()handles NULL, but kthread_create/kthread_run never return NULL, unlikekmalloc().I'd kzalloc'd the memory structure, then rearranged the order of calls initialising it without rearranging the destructor.
And if you hadn't used kzalloc you'll still blow up. I dislike zeroing allocs myself because I have dreams of valgrinding the kernel. gcc would warn about this for a stack var, it'd be nice if it did the same here.
Quoted 9 lines
Quoted 4 lines
3) If we really want to pass a failed
kthread_create()throughkthread_stop(), we should returnPTR_ERR(k)here. But that should only be done if it made it harder for the callers to screw up, which I don't think it does.I'm actually really dubious about
kthread_stop()returning a value at all. To me, returning an error implies that the function failed to do its job, ie the thread is still running. But that's not true; if it returns-EINVAL, it means the thread never ran.
You mean -EINTR? Yes, it should probably be left undefined: the caller presumably knows it didn't start the thread.
Quoted 6 lines
And why should the
caller care? Only three callers ofkthread_stopdo anything with the return value. Two of them just put the value in a debug message, and the third one goes to the effort of passing the return value through three layers of function pointer calls only to have all the callers discard it.
Good point. I assumed passing through the value would be useful, but as it's not been after a couple of years, we should make the callback return void. It'd be a painful transition, but I like the simplicity.
Quoted 6 lines
Quoted 3 lines
4) After a successful
kthread_run(),kthread_stop()will always return the value from the threadfn callback. ie.kthread_stop()doesn't ever fail. A simple semantic, which this patch breaks.Now I'm confused.
kthread_stopisn't failing. It preserves the invariant that when it returns, the thread is no longer running.
No, all we know is that they passed the wrong thing into kthread_stop(). So we really don't know if their thread is stopped; maybe it never existed (as in your case), maybe it's still running.
Quoted 5 lines
Quoted 3 lines
5) Covering up programmer errors is not good policy. I dislike
WARN_ON()because an oops is much harder to miss. Painful for you, but The System Works.I don't understand why we wouldn't want to be more robust here.
Because the OOPS made you fix the bug the way silently sucking it up wouldn't have.
Rusty.
Quoted 9 lines · Rusty Russell
Quoted 6 lines · On Wednesday 06 August 2008 22:07:04 Matthew Wilcox wrote:
Quoted 2 lines · Rusty Russell
How about a more ambitious "we've oopsed so break a mutex every 30 seconds of waiting" patch?
I was considering something more along the lines of "we've oopsed so find every mutex we own and release it".
Hmm, I don't think that's possible in general is it?
Maybe not. I've not looked carefully. Though I just had a really good idea that might have the same effect. If you have CONFIG_DEBUG_MUTEXES on, then there's an owner field. So if we don't free the thread_info for an oopsed task, we can tell that it's currently owned by a dead task, and usurp the ownership.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
1) There's no reason that
kthread_stopis uniquely difficult to use. Why pick on that one?It was the one I hit.
Yes, I got that :) But if we're not about to sprinkle "if
check_ptr(arg)" all through the kernel wherever someone can misuse a function.
We are starting to move checks from the callers into the callees -- at least as much for code size improvements as anything else. I'm not sure why the line should be between kthread_stop and kfree rather than on the other side of kthread_stop.
Quoted 9 lines
Quoted 5 lines
Quoted 2 lines
2) I know that
kfree()handles NULL, but kthread_create/kthread_run never return NULL, unlikekmalloc().I'd kzalloc'd the memory structure, then rearranged the order of calls initialising it without rearranging the destructor.
And if you hadn't used kzalloc you'll still blow up. I dislike zeroing allocs myself because I have dreams of valgrinding the kernel. gcc would warn about this for a stack var, it'd be nice if it did the same here.
It's too complex for gcc to be able to see this one, IMO. Valgrind would catch it. I'm not a huge fan of kzalloc either.
Quoted 3 lines
Good point. I assumed passing through the value would be useful, but as it's not been after a couple of years, we should make the callback return void. It'd be a painful transition, but I like the simplicity.
Cool.
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
4) After a successful
kthread_run(),kthread_stop()will always return the value from the threadfn callback. ie.kthread_stop()doesn't ever fail. A simple semantic, which this patch breaks.Now I'm confused.
kthread_stopisn't failing. It preserves the invariant that when it returns, the thread is no longer running.No, all we know is that they passed the wrong thing into
kthread_stop(). So we really don't know if their thread is stopped; maybe it never existed (as in your case), maybe it's still running.
Ah, good point, I hadn't considered the possibility of passing the wrong pointer in.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
5) Covering up programmer errors is not good policy. I dislike
WARN_ON()because an oops is much harder to miss. Painful for you, but The System Works.I don't understand why we wouldn't want to be more robust here.
Because the OOPS made you fix the bug the way silently sucking it up wouldn't have.
I made doing what I did no longer a bug ;-)
But, look, I'm perfectly happy with:
+ BUG_ON(!k || ERR_PTR(k));
as long as it happens before we take the mutex.
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."