[PATCH] kthread_{suspend, resume, suspend_check} locking bugs

[Giovanni Trematerra]

On Fri, Jan 22, 2010 at 05:40:41PM +0100, Attilio Rao wrote:
> 2010/1/16 Jilles Tjoelker <jilles at stack.nl>:
> > On Sun, Jan 10, 2010 at 02:09:43AM +0100, Attilio Rao wrote:
> >> I think that routines kthread_suspend(), kthread_resume() and
> >> kthread_suspend_check() are not adeguately SMP protected.
> >> That is because, in particular, the critical path doesn't protect,
> >> together, TDF_KTH_SUSP and sleeping activity. The right pattern should
> >> be to use the thread lock spinlock as an interlock and use msleep.
> >> Such bugs have not been revealed probabilly because there has been a
> >> lack of testing of such primitives and there are not, currently,
> >> consumers within our stock kernel.
> >
> >> Additively, kthread_suspend_check() seems to require to always pass
> >> curthread, which is silly (as we don't have to conform to any
> >> particular KPI), thus I think it is appropriate for the prototype to
> >> change.
> >> The following patch should fix the issue:
> >> http://www.freebsd.org/~attilio/kthread.diff
> >
> > The analysis and patch look sensible.
> 
> After have digged more with jhb I made a new patch:
> http://www.freebsd.org/~attilio/kthread_races.diff

In kthread_suspend_check you might have written
panic("%s: curthread is not a valid kthread", __func__);

> 
> Gianni, may you test this patch with the modules you made?
> 

With your last patch, no deadlock happens now.
It seems ok.

Hope this help

Just in case someone would make a review of the kernel test module
here it is the code:


/*-
 * Copyright (c) 2010
 *	Giovanni Trematerra <giovanni.trematerra at gmail.com>
 *
 * Permission to use, copy, modify, and distribute this software for any
 * purpose with or without fee is hereby granted, provided that the above
 * copyright notice and this permission notice appear in all copies.
 *
 * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
 * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
 * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
 * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
 * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
 * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
 * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 */
#include <sys/param.h>
#include <sys/kernel.h>
#include <sys/module.h>
#include <sys/param.h>
#include <sys/lock.h>
#include <sys/mutex.h>
#include <sys/systm.h>
#include <sys/kthread.h>
#include <sys/time.h>

#ifdef TESTPAUSE_DEBUG
	#define DPRINTF(x)	do { printf x; } while (0)
#else
	#define DPRINTF(x)	
#endif

static struct mtx test_global_lock;
static int global_condvar;
volatile int QUIT;
int test_thrcnt = 3;

static void 
thr_suspender(void *arg)
{
	struct thread *td = (struct thread *) arg;
	int error;

	for (;;) {
		if (QUIT == 1)
			break;
		error = kthread_suspend(td, 10*hz);
		if (error != 0) {
			if (error == EWOULDBLOCK)
				panic("Ooops: kthread deadlock\n");
			else 
				panic("kthread_suspend error: %d\n", error);
			break;
		}
	}

	mtx_lock(&test_global_lock);
	test_thrcnt--;
	wakeup(&global_condvar);
	mtx_unlock(&test_global_lock);

	kthread_exit();
}

static void 
thr_resumer(void *arg)
{
	struct thread *td = (struct thread *) arg;
	int error;

	for (;;) {
		if (QUIT == 1)
			break;
		error = kthread_resume(td);
		if (error != 0)
			panic("%s: error on kthread_resume. error: %d\n",
				   	__func__, error);
		if (QUIT == 1)
			break;
	}

	mtx_lock(&test_global_lock);
	test_thrcnt--;
	wakeup(&global_condvar);
	mtx_unlock(&test_global_lock);

	kthread_exit();
}

static void
thr_getsuspended(void *arg)
{
	for (;;) {
		if (QUIT == 1)
			break;
		kthread_suspend_check(curthread);
	}

	mtx_lock(&test_global_lock);
	test_thrcnt--;
	wakeup(&global_condvar);
	mtx_unlock(&test_global_lock);

	kthread_exit();
}

static void
kthrdlk_init(void)
{
	struct proc *testproc;
	struct thread *newthr;
	int error;

	QUIT = 0;
	mtx_init(&test_global_lock, "thrdlk_lock", NULL, MTX_DEF);
	testproc = NULL;
	error = kproc_kthread_add(thr_getsuspended, NULL, &testproc, &newthr, 0, 0, 
			"kthrdlk", "thr_getsuspended");
	if (error != 0)
		uprintf("cannot start thr_getsuspended error: %d\n", error);

	error = kproc_kthread_add(thr_resumer, newthr, &testproc, NULL, 0, 0, 
			"testproc", "thr_resumer");
	if (error != 0)
		uprintf("cannot start thr_resumer error: %d\n", error);

	error = kproc_kthread_add(thr_suspender, newthr, &testproc, NULL, 0, 0, 
			"testproc", "thr_suspender");
	if (error != 0)
		uprintf("cannot start thr_suspender error: %d\n", error);
}

static void
kthrdlk_done(void)
{
	int ret;
	/* wait kernel threads end */
	mtx_lock(&test_global_lock);
	QUIT = 1;
	while (test_thrcnt != 0) {
		ret = mtx_sleep(&global_condvar, &test_global_lock, 0, "waiting thrs end", 30 * hz);
		if (ret == EWOULDBLOCK) {
			uprintf("thrpause not die! remaing: %d", test_thrcnt);
			break;
		}
	}
	if (test_thrcnt == 0)
		DPRINTF(("All threads stopped \n"));

	mtx_destroy(&test_global_lock);
}

static int 
kthrdlk_handler(module_t mod, int /*modeventtype_t*/ what,
                            void *arg)
{
	switch (what) {
		case MOD_LOAD:
			kthrdlk_init();
			uprintf("kthrdlk loaded!\n");
			return (0);
		case MOD_UNLOAD:
			kthrdlk_done();
			uprintf("Bye Bye! kthrdlk unloaded!\n");
			return (0);
	}

	return (EOPNOTSUPP);
}

static moduledata_t mod_data= {
             "kthrdlk",
             kthrdlk_handler,
             0
     };

MODULE_VERSION(kthrdlk, 1);

DECLARE_MODULE(kthrdlk, mod_data, SI_SUB_EXEC, SI_ORDER_ANY);


--
Gianni