lkml.org 
[lkml]   [2008]   [Aug]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] Make kthread_stop() not oops when passed a bad pointer
Date
On Wednesday 06 August 2008 22:07:04 Matthew Wilcox wrote:
> On Wed, Aug 06, 2008 at 11:22:58AM +1000, Rusty Russell wrote:
> > 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?

> > 1) There's no reason that kthread_stop is 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.

> > 2) I know that kfree() handles NULL, but kthread_create/kthread_run never
> > return NULL, unlike kmalloc().
>
> 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.

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

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

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.

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

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.

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


\
 
 \ /
  Last update: 2008-08-08 08:23    [W:0.059 / U:27.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site