linux-kernel - Re: [RFC][PATCH] IRQ: Fix oneshot irq race between irq_finalize_oneshot and handle_level

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]

Date:	Thu, 11 Mar 2010 10:55:33 +0800
From:	Yong Zhang <yong.zhang@...driver.com>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Lars-Peter Clausen <lars@...afoo.de>, Ingo Molnar <mingo@...e.hu>,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCH] IRQ: Fix oneshot irq race between
 irq_finalize_oneshot and handle_level_irq

On Wed, Mar 10, 2010 at 08:56:22AM +0100, Thomas Gleixner wrote:
> > How about the following patch(maybe a little ugly). I think it will
> > resolve your concerns.
> 
> No it does not, but you are right that it's ugly. And it is patently
> wrong as well.
>  
> > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> > index d70394f..23b79c6 100644
> > --- a/kernel/irq/chip.c
> > +++ b/kernel/irq/chip.c
> > @@ -461,9 +461,24 @@ handle_level_irq(unsigned int irq, struct irq_desc *desc)
> >  	raw_spin_lock(&desc->lock);
> >  	mask_ack_irq(desc, irq);

I must say this patch isn't based on your previous one in which
mask_ack_irq() is modified to flag IRQ_MASKED.
I let IRQ_MASKED serialise interrupt-handler and irq-thread in oneshot
mode. 

> >  
> > -	if (unlikely(desc->status & IRQ_INPROGRESS))
> > -		goto out_unlock;
> > +	/*
> > +	 * if we are in oneshot mode and the irq thread is running on
> > +	 * another cpu, just return because the irq thread will unmask
> > +	 * the irq
> > +	 */
> > +	if (unlikely(desc->status & IRQ_ONESHOT)) {
> > +		if (unlikely(desc->status & (IRQ_INPROGRESS | IRQ_MASKED)
> > +					  == IRQ_INPROGRESS | IRQ_MASKED))
> > +			goto out_unlock;
> > +	}
> > +	else {
> > +		if (unlikely(desc->status & IRQ_INPROGRESS))
> > +			goto out_unlock;
> > +	}
> 
>   In case of IRQ_SHOT and IRQ_INPROGRESS and the other CPU having
>   unmasked the interrupt already you are reentering the handler which
>   is a nono.

I have thought of that kind of reentering you point above,
if IRQ_MASKED is cleared by irq_finalize_oneshot(), the thread_fn()
is done as well as the hardware opration. If another irq comes in,
the reentering does happen, but I think it's harmless because at
this time we just set IRQTF_RUNTHREAD and IRQ_MASKED and wake
irq thread up and then the irq thread loops for another time.
So irq will not return unmask in case of oneshot.

If I miss something, correct me please.

Thanks,
Yong


--
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/