netdev - Re: [RFC PATCH] workqueue.c: should schedule work serialize work->func

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4B452E11.90404@kernel.org>
Date:	Thu, 07 Jan 2010 09:42:57 +0900
From:	Tejun Heo <tj@...nel.org>
To:	John Fastabend <john.r.fastabend@...el.com>
CC:	netdev@...r.kernel.org, jesse.brandeburg@...el.com,
	tglx@...uxtronix.de
Subject: Re: [RFC PATCH] workqueue.c: should schedule work serialize work->func

Hello,

On 01/07/2010 08:05 AM, John Fastabend wrote:
> codepath
> schedule_work(); ... ; run_workqueue() -> work_clear_pending() -> f(work)
> 
> Although schedule_work() will only schedule the task if it is not already
> on the work queue the WORK_STRUCT_PENDING bits are cleared just before
> calling the work function.  If work is scheduled after this occurs and
> before f(work) completes it is possible to have multiple calls into f().
> 
> Should the kernel protect us from this? With something like the patch
> below.

This is by design.  Multithread workqueue doesn't protect against
works running simultaneously on different cpus.  The callback is
responsible for synchronization.

> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index 9466e86..fa6ffea 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -26,7 +26,8 @@ struct work_struct {
>  	atomic_long_t data;
>  #define WORK_STRUCT_PENDING 0		/* T if work item pending execution */
>  #define WORK_STRUCT_STATIC  1		/* static initializer (debugobjects) */
> -#define WORK_STRUCT_FLAG_MASK (3UL)
> +#define WORK_STRUCT_RUNNING 2
> +#define WORK_STRUCT_FLAG_MASK (7UL)	/* 0111 */
>  #define WORK_STRUCT_WQ_DATA_MASK (~WORK_STRUCT_FLAG_MASK)
>  	struct list_head entry;
>  	work_func_t func;
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index dee4865..b867125 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -397,10 +397,13 @@ static void run_workqueue(struct cpu_workqueue_struct *cwq)
>  		spin_unlock_irq(&cwq->lock);
>  
>  		BUG_ON(get_wq_data(work) != cwq);
> +		while (test_and_set_bit(WORK_STRUCT_RUNNING, work_data_bits(work)))
> +			cpu_relax();
>  		work_clear_pending(work);
>  		lock_map_acquire(&cwq->wq->lockdep_map);
>  		lock_map_acquire(&lockdep_map);
>  		f(work);
> +		clear_bit(WORK_STRUCT_RUNNING, work_data_bits(work));

work is not accessible at this point.  f() may have freed it already.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html