Re: [PATCH] Make kthread_stop() not oops when passed a bad pointer

[Rusty Russell <rusty@rustcorp.com.au>]

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.