Re: [PATCH v2 09/27] ARM: GICv3: introduce separate pending_irq structs for LPIs

[Stefano Stabellini <sstabellini@kernel.org>]

CC'ing Andrew, Jan and George to get more feedback on the security
impact of this patch.

I'll make a quick summary for you: we need to allocate a 56 bytes struct
(called pending_irq) for each potential interrupt injected to guests
(dom0 and domUs). With the new ARM interrupt controller there could be
thousands.

We could do the allocation upfront, which requires more memory, or we
could do the allocation dynamically at run time when each interrupt is
enabled, and deallocate the struct when an interrupt is disabled.

However, there is a concern that doing xmalloc/xfree in response to an
unprivileged DomU request could end up becoming a potential vector of
denial of service attacks. The guest could enable a thousand interrupts,
then disable a thousand interrupts and so on, monopolizing the usage of
one physical cpu. It only takes the write of 1 bit in memory for a guest
to enable/disable an interrupt.

See below.


On Mon, 27 Mar 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 27/03/17 18:44, Stefano Stabellini wrote:
> > On Mon, 27 Mar 2017, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 27/03/17 10:02, Andre Przywara wrote:
> > > > On 24/03/17 17:26, Stefano Stabellini wrote:
> > > > > On Fri, 24 Mar 2017, Andre Przywara wrote:
> > > > I am afraid that this would lead to situations where we needlessly
> > > > allocate and deallocate pending_irqs. Under normal load I'd expect to
> > > > have something like zero to three LPIs pending at any given point in
> > > > time (mostly zero, to be honest).
> > > > So this will lead to a situation where *every* LPI that becomes pending
> > > > triggers a memory allocation - in the hot path. That's why the pool
> > > > idea. So if we are going to shrink the pool, I'd stop at something like
> > > > five entries, to not penalize the common case.
> > > > Does that sound useful?
> > > 
> > > Not answering directly to the question here. I will summarize the face to
> > > face
> > > discussion I had with Andre this morning.
> > > 
> > > So allocating the pending_irq in the IRQ path is not a solution because
> > > memory
> > > allocation should not happen in IRQ context, see ASSERT(!in_irq()) in
> > > _xmalloc.
> > > 
> > > Regardless the ASSERT, it will also increase the time to handle and
> > > forward an
> > > interrupt when there are no pending_irq free because it is necessary to
> > > allocate a new one. Lastly, we have no way to tell the guest: "Try again"
> > > if
> > > it Xen is running out of memory.
> > > 
> > > The outcome of the discussion is to pre-allocate the pending_irq when a
> > > device
> > > is assigned to a domain. We know the maximum number of event supported by
> > > a
> > > device and that 1 event = 1 LPI.
> > > 
> > > This may allocate more memory (a pending_irq is 56 bytes), but at least we
> > > don't need allocation on the fly and can report error.
> > > 
> > > One could argue that we could allocate on MAPTI to limit the allocation.
> > > However, as we are not able to rate-limit/defer the execution of the
> > > command
> > > queue so far, a guest could potentially flood with MAPTI and monopolize
> > > the
> > > pCPU for a long time.
> > 
> > It makes a lot of sense to keep the allocation out of the irq path.
> > However, I am wondering if the allocation/deallocation of pending_irq
> > structs could be done at the point the vLPIs are enabled/disabled,
> > instead of device assignment time.
> 
> I am not sure what you mean by enabling/disabling vLPIS. Do you mean when the
> guest is enabling/disabling them or the guest will assign a vLPI to a
> (deviceID, event) via MAPTI.

I was thinking enable/disable on the LPI Configuration table. However,
it would have the same issues you wrote for MAPTI.


> For both guest could potentially flood us. It would take us a lot of time to
> allocate/free memory for each vLPIs modified. Hence, why I didn't suggest it
> and said: "One could argue that we could allocate on MAPTI to limit the
> allocation...".

Given that Xen wouldn't allocate the same pending_irq twice, at most Xen
would allocate the same amount of memory and the same number of
pending_irq that it would otherwise allocate if we did it at assignment
time, right?

Therefore, we are not concerned about memory utilization. We are
concerned about the CPU time to do the allocation itself, right?

However, the CPU time to run xmalloc/xfree should be small and in any
case the scheduler has always the chance to deschedule the vcpu if it
wants to. This is no different than issuing any of the hypercalls that
require some work on the hypervisor side.

I don't think we have reasons to be concerned. Any opinions?

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