linux-kernel - Re: [PATCH v3 2/3] genirq: introduce update_irq

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: <alpine.DEB.2.21.1908191005530.1923@nanos.tec.linutronix.de>
Date:   Mon, 19 Aug 2019 10:18:42 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     luoben <luoben@...ux.alibaba.com>
cc:     alex.williamson@...hat.com, linux-kernel@...r.kernel.org,
        tao.ma@...ux.alibaba.com, gerry@...ux.alibaba.com,
        nanhai.zou@...ux.alibaba.com, linyunsheng@...wei.com
Subject: Re: [PATCH v3 2/3] genirq: introduce update_irq_devid()

Ben,

On Mon, 19 Aug 2019, luoben wrote:
> 在 2019/8/15 下午10:58, Thomas Gleixner 写道:
> > > This narrows the gap for setting up new irq (and irte, if has)
> > What does that mean: "narrows the gap"
> > 
> > What's the gap and why is it only made smaller and not closed?
> 
> Sorry for confusing. The so called 'gap' is a time window between free_irq()
> and request_irq().

And exactly this information wants to be in the changelog.
 
> > function please. Also it does not matter whether the time is short or
> > not. The point is:
> > 
> >       	 Ensure that an interrupt in flight on another CPU which uses
> > the
> >       	 old 'dev_id' has completed because the caller can free the
> > memory
> > 	 to which it points after this function returns.
> > 
> > But this has another twist:
> > 
> >      CPU0				CPU1
> > 
> >      interrupt
> >      	primary_handler(old_dev_id)
> > 	   do_stuff_on(old_dev_id)
> > 	   return WAKE_THREAD;		update_dev_id()
> >          wakeup_thread();
> > 					  action->dev_id = new_dev_id;
> >      irq_thread()
> >          secondary_handler(new_dev_id)
> > 	
> > That's broken and synchronize_irq() does not protect against it.
> 
> Thanks to point it out, I will change to the following in next version, is
> that ok?
> 
> ...
> 
>     /*

  ^^^
Please use a mail client which does not insert random wierd characters.

>      * Ensure that an interrupt in flight on another CPU which uses the
>      * old 'dev_id' has completed because the caller can free the memory
>      * to which it points after this function returns. And also void to

s/void/avoid/

>      * update 'dev_id' in the middle of a threaded interrupt process, it
>      * can lead to a twist that primary handler uses old 'dev_id' but new
>      * 'dev_id' is used by secondary handler.
>      */
>     disable_irq(irq);

Yes, that works.

Thanks,

	tglx