linux-kernel - Re: Is adding requeue_delayed

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090825093906.GA3020@redhat.com>
Date:	Tue, 25 Aug 2009 11:39:06 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Roland Dreier <rdreier@...co.com>
Cc:	linux-kernel@...r.kernel.org,
	Dmitry Torokhov <dmitry.torokhov@...il.com>
Subject: Re: Is adding requeue_delayed_work() a good idea

Now I noticed I forgot to CC Dmitry yesterday...

On 08/24, Roland Dreier wrote:
>
>  > > In my particular case it doesn't really matter.  In the queued case it
>  > > could leave it to run whenever it gets to the head of the workqueue.  In
>  > > the already running case then I think the timer should be reset.  The
>  > > main point is that if I do requeue_delayed_work() I want to make sure
>  > > the work runs all the way through from the beginning at some point in
>  > > the future.  The pattern I have in mind is something like:
>  > >
>  > > 	spin_lock_irqsave(&mydata_lock);
>  > > 	new_timeout = add_item_to_timeout_list();
>  > > 	requeue_delayed_work(wq, &process_timeout_list_work, new_timeout);
>  > > 	spin_unlock_irqsave(&mydata_lock);
>  > >
>  > > so if the process_timeout_list_work runs early or twice it doesn't
>  > > matter; I just want to make sure that the work runs from the beginning
>  > > and sees the new item I added to the list at some point after the
>  > > requeue.
>  >
>  > Hmm. But, asuming that process_timeout_list_work->func() takes mydata_lock
>  > too, you can just use queue_delayed_work() ?
>  >
>  > process_timeout_list_work can't miss new items, queue_delayed_work()
>  > can only fail if dwork is pending and its ->func has not started yet.
>
> Maybe I misunderstand the code or misunderstand you,

No, sorry. I misunderstood you (and sorry for delays btw).

I have read "I just want to make sure" above but forgot you also need
to shorten the timeout.

OK, in this case I think we have a simple solution,

	// like cancel_delayed_work, but uses del_timer().
	// this means, if it returns 0 the timer function may be
	// running and the queueing is in progress. The caller
	// can't rely on flush_workqueue/etc
	static inline int __cancel_delayed_work(struct delayed_work *work)
	{
		int ret;

		ret = del_timer(&work->timer);
		if (ret)
			work_clear_pending(&work->work);
		return ret;
	}

Now, you can do

	spin_lock_irqsave(&mydata_lock);
	new_timeout = add_item_to_timeout_list();

	__cancel_delayed_work(&process_timeout_list_work);
	queue_delayed_work(wq, &process_timeout_list_work, new_timeout);

	spin_unlock_irqsave(&mydata_lock);

If queue_delayed_work() fails, this means that WORK_STRUCT_PENDING is set,
dwork->work is already queued or the queueing is in progress. In both
cases it will run "soon" as if we just called queue_work(&dwork->work).

But this assumes nobody else does queue_delayed_work(dwork, HUGE_DELAY) in
parallel, otherwise we can lose the race and another caller can setup
HUGE_DELAY timeout.

In particular, if process_timeout_list_work->func() itself uses
queue_delay_work() to re-arm itself we can race. Bu t I think it is always
possible to do something to synchronize with work->func, for example
work->func() can re-arm itself _before_ it scans timeout_list (under the
same lock). This way, if re-queue code above fails because work->func()
wins, work->func() must see the new additions to timeout_list.

Can this work for you?

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/