Re: sleep bug in taskqueue(9)

From: <mdf_at_FreeBSD.org>
Date: Fri, 12 Nov 2010 13:24:51 -0800
On Fri, Nov 12, 2010 at 12:25 PM, Hans Petter Selasky <hselasky_at_c2i.net> wrote:
> On Friday 12 November 2010 17:38:38 mdf_at_freebsd.org wrote:
>> On Fri, Nov 12, 2010 at 6:23 AM, Hans Petter Selasky <hselasky_at_c2i.net>
> wrote:
>> > On Friday 12 November 2010 15:18:46 mdf_at_freebsd.org wrote:
>> >> On Fri, Nov 12, 2010 at 12:56 AM, Hans Petter Selasky <hselasky_at_c2i.net>
>> >
>> > wrote:
>> >> > On Thursday 29 April 2010 01:59:58 Matthew Fleming wrote:
>> >> >> It looks to me like taskqueue_drain(taskqueue_thread, foo) will not
>> >> >> correctly detect whether or not a task is currently running.  The
>> >> >> check is against a field in the taskqueue struct, but for the
>> >> >> taskqueue_thread queue with more than one thread, multiple threads
>> >> >> can simultaneously be running a task, thus stomping over the
>> >> >> tq_running field.
>> >> >>
>> >> >> I have not seen any problem with the code as-is in actual use, so
>> >> >> this is purely an inspection bug.
>> >> >>
>> >> >> The following patch should fix the problem.  Because it changes the
>> >> >> size of struct task I'm not sure if it would be suitable for MFC.
>> >> >
>> >> > 1) The u_char is going to leave a hole in that structure on ARM
>> >> > platforms for example.
>> >> >
>> >> > 2) The existing taskqueue implementation also has a missing check for
>> >> > the pending count wrapping to zero. I.E. it should stick at 0xFFFF
>> >> > and not wrap to 0.
>> >>
>> >> This commit mail is rather old, and this fix was incorrect, because
>> >> the task cannot be referenced after it has been run.  Some task
>> >> handlers will free the task as part of the handler.
>> >
>> > Ok, maybe the e-mail got stuck somewhere. Have you fixed the above
>> > mentioned issues in a newer patch?
>>
>> If you look at the file history for subr_taskqueue.c:
>>
>> http://svn.freebsd.org/viewvc/base/head/sys/kern/subr_taskqueue.c
>>
>> You will see quite a few commits by me.  The most recent relating to
>> detecting if a task is running is being MFC'd today:
>
> Yes, and I see that this code needs an overflow check, which is one of the
> issues still not fixed:

You keep bringing this up.  It is not a new issue.  It is not a bug in
any of the patches.  It is extremely unlikely that a task will be
queued 65536 times before execution.  It is more worthy of an assert
rather than a check, because if a task is enqueued that many times
without being run then there's likely a stuck task in the queue.

The patch you posted will lie as well, so I would not consider it
sufficient if someone wanted to address the issue.

Thanks,
matthew


>
> Before:
>
>        /*
>         * Count multiple enqueues.
>         */
>        if (task->ta_pending) {
>                task->ta_pending++;
>                TQ_UNLOCK(queue);
>                return 0;
>        }
>
>
> After:
>
>        /*
>         * Count multiple enqueues.
>         */
>        if (task->ta_pending) {
>                if (task->ta_pending != 0xFFFF)
>                        task->ta_pending++;
>                TQ_UNLOCK(queue);
>                return 0;
>        }
>
> Else the ta_pending can wrap to zero and the code will not do what it
> announces it does.
>
> --HPS
>
Received on Fri Nov 12 2010 - 20:24:53 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:09 UTC