Message-Id: <201212101103.59917.vitas@nppfactor.kiev.ua>
Date:	Mon, 10 Dec 2012 11:03:59 +0200
From:	Vitalii Demianets <vitas@...factor.kiev.ua>
To:	"Hans J. Koch" <hjk@...sjkoch.de>
Cc:	Cong Ding <dinggnu@...il.com>, linux-kernel@...r.kernel.org,
	"Greg Kroah-Hartman" <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

On Saturday 08 December 2012 01:47:21 Hans J. Koch wrote:
> On Fri, Dec 07, 2012 at 05:00:54PM +0200, Vitalii Demianets wrote:
> > > On second thought, we can't call enable_irq()/disable_irq()
> > > unconditionally because of the potential disable counter
> > > (irq_desc->depth) disbalance. That's why we need UIO_IRQ_DISABLED flag,
> > > and that's why we should check it in uio_pdrv_genirq_irqcontrol().
> > > On the other hand, you are right in that we don't need to check it
> > > inside irq handler. Inside irq handler we can disable irq and set the
> > > flag unconditionally, because:
> > > a) We know for sure that irqs are enabled, because we are inside
> > > (not-shared) irq handler;
> > >  and
> > > b) We are guarded from potential race conditions by spin_lock_irqsave()
> > > call in uio_pdrv_genirq_irqcontrol().
> > >
> > > So,yes, we can get rid of costly atomic call to
> > > test_and_set_bit(UIO_IRQ_DISABLED,..) inside irq handler. But I still
> > > don't like the idea of mixing this optimization with bug fixes in a
> > > single patch.
> >
> > On the third thought, we can't ;)
> > Imagine the SMP system where uio_pdrv_genirq_irqcontrol() is being
> > executed on CPU0 and irq handler is running concurrently on CPU1. To
> > protect from disable_irq counter disbalance we must first check current
> > irq status, and in atomic manner. Thus we prevent  double-disable, one
> > from
> > uio_pdrv_genirq_irqcontrol() running on CPU0 and another form irq handler
> > running on CPU1.
> > Above consideration justifies current code.
> >
> > But it seems we have potential concurrency problem here anyway. Here is
> > theoretical scenario:
> > 1) Initial state: irq is enabled, uio_pdrv_genirq_irqcontrol() starts
> > being executed on CPU0 with irq_on=1 and at the same time, concurrently,
> > irq handler starts being executed on CPU1;
> > 2) irq handler executes line
> >    if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
> > as irq was enabled, the condition holds. And now UIO_IRQ_DISABLED is set.
> > 3) uio_pdrv_genirq_irqcontrol() executes line
> >    if (test_and_clear_bit(UIO_IRQ_DISABLED, &priv->flags))
> > as UIO_IRQ_DISABLED was set by CPU1, the condition holds. And now
> > UIO_IRQ_DISABLED is cleared.
> > 4) CPU0 executes enable_irq() . IRQ is enabled. "Unbalanced enable for
> > IRQ %d\n" warning is issued.
> > 5) irq handler on CPU1 executes disable_irq_nosync(). IRQ is disabled.
> >
> > And now we are in situation where IRQ is disabled, but UIO_IRQ_DISABLED
> > is cleared. Bad.
> >
> > The above scenario is purely theoretical, I have no means to check it in
> > hardware.
> > The (theoretical) solution is to guard UIO_IRQ_DISABLED test and actual
> > irq disabling inside irq handler by priv->lock.
> >
> > What do you think about it? Is it worth worrying about?
>
> Hi Vitalii,
> thanks a lot for analyzing the problem so thoroughly. It made me review
> uio_pdrv_genirq.c again, and I noticed several issues and came to the
> following conclusions:
>
> 1.) priv->lock is completely unnecessary. It is only used in one function,
> so there's nothing it could possibly protect.
>
> 2.) All these "test_and_clear_bit" and "test_and_set_bit" calls are also
> unnecessary. We can simply use enable_irq and disable_irq in both the
> irq handler and in uio_pdrv_genirq_irqcontrol.
>
> We should go "back to the roots" and have a look at how UIO works.
> The workflow it is intended for is like this:
>
> 1.) Hardware is in Reset State (e.g. after boot). Any decent hardware
> has its interrupt disabled at that time.
>
> 2.) uio_pdrv_genirq is loaded. Kernel enables the irq.
>
> 3.) Userspace part of the driver comes up. It will initialize the hardware
> (including setting the bits that enable the interrupt).
>
> 4.) Userspace will then issue a blocking read() on /dev/uioX. Typically,
> there'll be a loop or a thread with this blocking read() at the beginning
> (usually using the select() call).
>
> 5.) At some time, a hardware interrupt will occur. The irq handler in
> kernel space will be called, only to disable the irq. This will also cause
> the UIO core to make /dev/uioX readable.
>
> 6.) Userspace's blocking read returns. Userspace does its work by
> reading/writing device memory.
>
> 7.) As the last thing, Userspace writes a "1" to /dev/uioX, which causes
> uio_pdrv_genirq_irqcontrol to be called, re-enabling the interrupt.
>
> 8.) Goto 4.)
>
> We should also remember that uio_pdrv_genirq_handler() is NOT a real
> hardware irq handler. The real handler is in the UIO core, which will
> increment the event number and wake up userspace.
>
> So, although your scenario clearly shows a subtle race condition, there is
> none. If userspace does stupid things, no harm will be done to the kernel.

With all due respect, I disagree here. If userspace does stupid things, 
unintentionally because of poorly written code or intentionally because of a 
malicious purpose, we could have irq depth counter disbalanced which, in 
turn, could lead to the interrupt not enabled when it should be. In which 
case the whole system would stop doing useful things which it was designed to 
do.

> If userspace is designed the way described above (and in the
> documentation), it will always wake up with its interrupt disabled, do its
> work, and then re-enable the interrupt. You can probably think of a few
> things userspace could do to screw things up. But that's not our problem.
>

Disagree again. I think we can not leave a hole in kernel which opens DOS 
possibilities and hope userspace will behave well. There are always errors in 
the code. And security issues, too.

> Could you hack up a patch for that? I think it should start with removing
> uio_pdrv_genirq_platdata->lock and uio_pdrv_genirq_platdata->flags...
>
> Thanks again for your work. What do you think about my theory?
>

I think that we should fix the bug, not ignore it.

Ok, less words, more code. I am posting a new series of two patches. First 
is "Fix memory freeing issues" (with improved wording in the comment which 
you found unclear) and the second is about fixing SMP race condition.
--
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/

Powered by blists - more mailing lists