Re: drm: avoid possible deadlock in kthread_stop

[Sebastien Marie]

On Wed, Oct 14, 2020 at 08:58:04PM +0200, Mark Kettenis wrote:
> > Date: Thu, 1 Oct 2020 09:09:50 +0200
> > From: Sebastien Marie <sema...@online.fr>
> > 
> > Hi,
> > 
> > Currently, when a process is calling kthread_stop(), it sets a flag
> > asking the thread to stop, and enters in sleep mode, but the code
> > doing the stop doesn't wakeup the caller of kthread_stop().
> > 
> > The thread should also be unparked as else it will not seen the
> > KTHREAD_SHOULDSTOP flag. it follows what Linux is doing.
> > 
> > While here, I added some comments in the locking logic for park/unpark
> > and stop.
> > 
> > Comments or OK ?
> 
> I don't think adding all those comments makes a lot of sense.  This
> uses a fairly standard tsleep/wakeup pattern and the some of the
> comments really state the obvious.

it was the way I did to audit the code and understand what it did.

> Can you do a diff that just adds
> the missing wakeup() and kthread_unpark() call?

here a new diff.

diff 4efbe95c75086b3a7b0074651bfa04fd58990a98 /home/semarie/repos/openbsd/src
blob - fd797effc74d6eb4a172c81be8feac0ed168ec5d
file + sys/dev/pci/drm/drm_linux.c
--- sys/dev/pci/drm/drm_linux.c
+++ sys/dev/pci/drm/drm_linux.c
@@ -207,6 +217,7 @@ kthread_func(void *arg)
 
        ret = thread->func(thread->data);
        thread->flags |= KTHREAD_STOPPED;
+       wakeup(thread);
        kthread_exit(ret);
 }
 
@@ -298,8 +327,9 @@ kthread_stop(struct proc *p)
 
        while ((thread->flags & KTHREAD_STOPPED) == 0) {
                thread->flags |= KTHREAD_SHOULDSTOP;
+               kthread_unpark(p);
                wake_up_process(thread->proc);
                tsleep_nsec(thread, PPAUSE, "stop", INFSLP);
        }
        LIST_REMOVE(thread, next);
 

Thanks.
-- 
Sebastien Marie