linux-kernel - Re: Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 10 Jul 2017 09:23:49 +0200
From:   Juergen Gross <jgross@...e.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     jeffy.chen@...k-chips.com,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        xen-devel <xen-devel@...ts.xenproject.org>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
        Marc Zyngier <marc.zyngier@....com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

On 07/07/17 19:11, Thomas Gleixner wrote:
> On Fri, 7 Jul 2017, Thomas Gleixner wrote:
> 
>> On Fri, 7 Jul 2017, Juergen Gross wrote:
>>
>>> Commit bf22ff45bed664aefb5c4e43029057a199b7070c ("genirq: Avoid
>>> unnecessary low level irq function calls") breaks Xen guest
>>> save/restore handling.
>>>
>>> The main problem are the PV devices using Xen event channels as
>>> interrupt sources which are represented as an "irq chip" in the kernel.
>>> When saving the guest the event channels are masked internally. At
>>> restore time event channels are re-established and unmasked via
>>> irq_startup().
> 
> And how exactly gets irq_startup() invoked on those event channels?

[   30.791879] Call Trace:
[   30.791883]  ? irq_get_irq_data+0xe/0x20
[   30.791886]  enable_dynirq+0x23/0x30
[   30.791888]  unmask_irq.part.33+0x26/0x40
[   30.791890]  irq_enable+0x65/0x70
[   30.791891]  irq_startup+0x3c/0x110
[   30.791893]  __enable_irq+0x37/0x60
[   30.791895]  resume_irqs+0xbe/0xe0
[   30.791897]  irq_pm_syscore_resume+0x13/0x20
[   30.791900]  syscore_resume+0x50/0x1b0
[   30.791902]  xen_suspend+0x76/0x140

> 
>>> I have a patch repairing the issue, but I'm not sure if this way to do
>>> it would be accepted. I have exported mask_irq() and I'm doing the
>>> masking now through this function. Would the attached patch be
>>> acceptable? Or is there a better way to solve the problem?
>>
>> Without looking at the patch (too lazy to fiddle with attachments right
>> now), this is definitely wrong. I'll have a look later tonight.
> 
> Not that I'm surprised, but that patch is exactly what I expected. Export a
> random function, which helps to paper over the real problem and run away.
> These functions are internal for a reason and we worked hard on making
> people understand that fiddling with the internals of interrupts is a
> NONO. If there are special requirements for a good reason, then we create
> proper interfaces and infrastructure, if there is no good reason, then the
> problematic code needs to be fixed. There is no exception for XEN.

I'm absolutely on your side here. That was the reason I didn't send
the patch right away, but asked how to solve my issue in a way which
isn't "quick and dirty". The patch was just the easiest way to explain
what should be the result of the proper solution.

> Can you please explain how that save/restore stuff works and which
> functions are involved?

It is based on suspend/resume framework. The main work to be done
additionally is to disconnect from the pv-backends at save time and
connect to the pv-backends again at restore time.

The main function triggering all that is xen_suspend() (as seen in
above backtrace).


Juergen