Re: sleep bug in taskqueue(9)

On Friday 12 November 2010 22:24:51 mdf_at_freebsd.org wrote:
> 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.

In the USB world, many of the taskqueue enqueue calls result directly from 
IOCTL's, so I consider this a real issue!

--HPS