linux-kernel - Re: [PATCH v2] xen-pciback: Consider INTx disabled when MSI/MSI-X is enabled

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] [day] [month] [year] [list]

Message-ID: <df8309e3-5a56-ccf0-fc3d-16fa52f9fa7e@suse.com>
Date:   Fri, 18 Nov 2022 13:27:49 +0100
From:   Jan Beulich <jbeulich@...e.com>
To:     Marek Marczykowski-Górecki 
        <marmarek@...isiblethingslab.com>
Cc:     Juergen Gross <jgross@...e.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>,
        "moderated list:XEN HYPERVISOR INTERFACE" 
        <xen-devel@...ts.xenproject.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] xen-pciback: Consider INTx disabled when MSI/MSI-X is
 enabled

On 18.11.2022 13:06, Marek Marczykowski-Górecki wrote:
> On Fri, Nov 18, 2022 at 08:36:14AM +0100, Jan Beulich wrote:
>> On 18.11.2022 03:35, Marek Marczykowski-Górecki wrote:
>>> Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until
>>> the table is filled. Then it disables INTx just before clearing MASKALL
>>> bit. Currently this approach is rejected by xen-pciback.
>>> According to the PCIe spec, device cannot use INTx when MSI/MSI-X is
>>> enabled.
>>
>> Similarly the spec doesn't allow using MSI and MSI-X at the same time.
>> Before your change xen_pcibk_get_interrupt_type() is consistent for all
>> three forms of interrupt delivery; imo it also wants to be consistent
>> after your change. This effectively would mean setting only one bit at
>> a time (or using an enum right away), but then the question is what
>> order you do the checks in. IOW I think the change to the function is
>> wrong.
> 
> IIUC the difference is that enabling MSI or MSI-X implicitly disables
> INTx, while enabling both MSI and MSI-X is UB. This means that MSI
> active and PCI_COMMAND_INTX_DISABLE bit not set means "only MSI is
> active" - which the function now properly reports.

Hmm, yes, this is perhaps a good way to look at it.

> Both MSI and MSI-X active at the same time means a bug somewhere else
> and the current code allows only to disable one of them in such case. I
> could replace this with BUG_ON, or simply assume such bug doesn't exist
> and ignore this case, if you prefer.

BUG_ON() would imply this state cannot be reached no matter what is
requested from outside of the kernel. I'm not sure that's true here,
so keeping that aspect unchanged is probably fine (and with the above
I then take back my "wrong" from the earlier reply.

Jan