Re: [Xen-devel] [PATCH v2] interrupts: allow guest to set and clear MSI-X mask bit

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/08/13 12:49, Jan Beulich wrote:
>>>> On 05.08.13 at 13:01, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 05/08/13 11:44, Jan Beulich wrote:
>>>>>> On 23.07.13 at 19:59, Joby Poriyath <joby.poriyath@xxxxxxxxxx> wrote:
>>>> On Tue, Jul 23, 2013 at 09:28:42AM -0400, Konrad Rzeszutek Wilk wrote:
>>>>> On Tue, Jul 23, 2013 at 11:54:41AM +0100, Joby Poriyath wrote:
>>>>>> Ping...
>>>>>>
>>>>>> Could you kindly review this?
>>>>> I believe Malcom reviewed and sent you an email with his response.
>>>> Andrew and Malcolm has reviewed the code.
>>>>
>>>>> Are you waiting for Andrew to look at it as well? Or Jan? Jan is
>>>>> out on vacation for a couple of weeks.
>>>> I was hoping that Jan would also review this, since this patch 
>>>> partly reverts his code. 
>>>>
>>>> This patch makes an assumption that, for a passed through device, 
>>>> xen needs to mask the MSI-X control bit only when it does vcpu 
>>>> migration. Otherwise xen leaves it in the state guest has set it.
>>> Exactly. And hence the patch is wrong, 
>> Unfortunately it is not.
>>
>> The whole problem is that the VF can (and does) request that the PF
>> resets itself (using an out-of-band mailbox system in the hardware
>> itself), causing the mask bit to change (i.e. get set) without Xen's
>> knowledge.
> The at the very least such a code change should be done for VFs
> only. But I'd object to that too.
>
>> At this point, it is only the guest which can rectify the situation and
>> clear the mask bit.
>>
>> I would agree that the solution is not perfect, but SRIOV virtual
>> functions rely on the ability to clear the mask bit, whether or not it
>> is a security issue.  This means that your patch to disable writing to
>> the mask bit has caused a functional regression in all SRIOV operations
>> in Xen.
> Only in the special case that was described in the patch header
> afaict (or else I'm sure we would have known much earlier).

No - this unconditionally breaks any attempt to use an SRIOV virtual
function by a VF-aware driver.  This includes FreeBSD and SLES VMs, and
results in a complete loss of interrupts, because the mask bit has been
set without Xen's knowledge, and nothing ever clears it, as the guests
attempt to clear it gets intercepted and discarded.

This was discovered as a regression from XenServer 6.1 to 6.2, and was
identified specifically this change which was backported into Xen 4.1.5

>
>> I would say that the less bad alternative is to reintroduce the ability
>> to change the mask bit, and fix the regression, then work out how to fix
>> the security aspect.
> No, my position is pretty clear here: No re-introduction of security
> issues when fixing secondary problems.

Then we have no option but to state that SRIOV is completely broken
under Xen (including all stable trees) until we have a fix.

>
> The more that the immediate (but only partial) solution would be quite
> clear: If the guest wants to clear the flag and Xen thinks the flag is
> clear, it could certainly allow the write. The security aspect here is
> when the guest wants to clear the flag while Xen expects it to be set.
>
> But a proper solution would of course be preferred. That may involve
> notifying Xen of the reset from the PF driver.
>
> Jan
>

That is making the assumption that the PF driver is even aware that the
reset has occurred.  If the PF driver can get at the reset information,
I still cant see Xen specific hooks being accepted upstream.

Fundamentally, given that only the VM is in a position to actually know
about the state of the mask bit (As we cant possibly have Xen polling
for the state), then the guest has to have access to the mask bit.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel