Re: [PATCH] tracing: Wait for preempt irq delay thread to finish

[Xiao Yang <yangx.jy@cn.fujitsu.com>]

Hi Steven,

Thanks for your further investigation.

I used the following ways to test your fix patch on my slow vm and 
didn't see any issue:
1) Insert and remove preemptirq_delay_test in loops.
2) Insert preemptirq_delay_test, write to 
/sys/kernel/preemptirq_delay_test/trigger and remove 
preemptirq_delay_test in loops.
3) Ran irqsoff_tracer.tc in loops.

BTW: For irqsoff_tracer.tc, should we extend code to test the burst 
feature and the sysfs trigger?

Reviewed-by: Xiao Yang <yangx.jy@cn.fujitsu.com>

Thanks,
Xiao Yang
On 2020/5/6 22:30, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)"<rostedt@goodmis.org>
>
> Running on a slower machine, it is possible that the preempt delay kernel
> thread may still be executing if the module was immediately removed after
> added, and this can cause the kernel to crash as the kernel thread might be
> executing after its code has been removed.
>
> There's no reason that the caller of the code shouldn't just wait for the
> delay thread to finish, as the thread can also be created by a trigger in
> the sysfs code, which also has the same issues.
>
> Link: http://lore.kernel.org/r/5EA2B0C8.2080706@cn.fujitsu.com
>
> Cc: stable@vger.kernel.org
> Fixes: 793937236d1ee ("lib: Add module for testing preemptoff/irqsoff latency tracers")
> Reported-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> Signed-off-by: Steven Rostedt (VMware)<rostedt@goodmis.org>
> ---
>   kernel/trace/preemptirq_delay_test.c | 30 ++++++++++++++++++++++------
>   1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/trace/preemptirq_delay_test.c b/kernel/trace/preemptirq_delay_test.c
> index 31c0fad4cb9e..c4c86de63cf9 100644
> --- a/kernel/trace/preemptirq_delay_test.c
> +++ b/kernel/trace/preemptirq_delay_test.c
> @@ -113,22 +113,42 @@ static int preemptirq_delay_run(void *data)
>
>   	for (i = 0; i<  s; i++)
>   		(testfuncs[i])(i);
> +
> +	set_current_state(TASK_INTERRUPTIBLE);
> +	while (!kthread_should_stop()) {
> +		schedule();
> +		set_current_state(TASK_INTERRUPTIBLE);
> +	}
> +
> +	__set_current_state(TASK_RUNNING);
> +
>   	return 0;
>   }
>
> -static struct task_struct *preemptirq_start_test(void)
> +static int preemptirq_run_test(void)
>   {
> +	struct task_struct *task;
> +
>   	char task_name[50];
>
>   	snprintf(task_name, sizeof(task_name), "%s_test", test_mode);
> -	return kthread_run(preemptirq_delay_run, NULL, task_name);
> +	task =  kthread_run(preemptirq_delay_run, NULL, task_name);
> +	if (IS_ERR(task))
> +		return PTR_ERR(task);
> +	if (task)
> +		kthread_stop(task);
> +	return 0;
>   }
>
>
>   static ssize_t trigger_store(struct kobject *kobj, struct kobj_attribute *attr,
>   			 const char *buf, size_t count)
>   {
> -	preemptirq_start_test();
> +	ssize_t ret;
> +
> +	ret = preemptirq_run_test();
> +	if (ret)
> +		return ret;
>   	return count;
>   }
>
> @@ -148,11 +168,9 @@ static struct kobject *preemptirq_delay_kobj;
>
>   static int __init preemptirq_delay_init(void)
>   {
> -	struct task_struct *test_task;
>   	int retval;
>
> -	test_task = preemptirq_start_test();
> -	retval = PTR_ERR_OR_ZERO(test_task);
> +	retval = preemptirq_run_test();
>   	if (retval != 0)
>   		return retval;
>