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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 03/24/2017 03:50 PM, Andre Przywara wrote:
> On 24/03/17 11:40, Julien Grall wrote:
> > > +/*
> > > + * Holding struct pending_irq's for each possible virtual LPI in each
> > > domain
> > > + * requires too much Xen memory, also a malicious guest could
> > > potentially
> > > + * spam Xen with LPI map requests. We cannot cover those with (guest
> > > allocated)
> > > + * ITS memory, so we use a dynamic scheme of allocating struct
> > > pending_irq's
> > > + * on demand.
> > > + */
> > I am afraid this will not prevent a guest to use too much Xen memory.
> > Let's imagine the guest is mapping thousands of LPIs but decides to
> > never handle them or is slow. You would allocate pending_irq for each
> > LPI, and never release the memory.
> So what would be the alternative here? In the current GICv2/3 scheme a
> pending_irq for each (SPI) interrupt is allocated up front. This
> approach here tries just to be a bit smarter for LPIs, since the
> expectation is that we by far don't need so many. In the worst case the
> memory allocation would converge to the upper limit (number of mapped
> LPIs), which would probably be used otherwise for the static allocation
> scheme. Which means we would never be worse.
It is getting worse in the current version because you have the list per 
vCPU. So you may get nLPIs * nvCPUs pending_irq allocated.
> What actually is missing is the freeing the memory upon domain
> destruction, which can't be tested for Dom0.
As I already said multiple time, forgetting to add the domain 
destruction is much worse because this is a call to miss something when 
we will need it. I prefer to see the code now so we can review.
> > If we use dynamic allocation, we need a way to limit the number of LPIs
> > received by a guest to avoid memory exhaustion. The only idea I have is
> > an artificial limit, but I don't think it is good. Any other ideas?
> I think if we are concerned about this, we shouldn't allow *DomUs* to
> allocate too many LPIs, which are bounded by the DeviceID/EventID
> combinations. This will be under full control by Dom0, which will
> communicate the number of MSIs (events) for each passthrough-ed device,
> so we can easily impose a policy limit upon creation.
If DOM0 is able to control the number of DeviceID/EventID, then why do 
we need to allocate on the fly?
Also, I don't think the spec prevents to populate Device Table for a 
deviceID that has no Device associated. It could be used by a domain to 
inject fake LPI. For instance this could replace polling mode for the 
command queue.
My main concern is memory allocation can fail. Then what will you do? 
You will penalize a well-behave domain that because someone else was nasty.
And you have no way to report to the guest: "I was not able to allocate 
memory, please try later" because this is an interrupt.
> > > +struct pending_irq *lpi_to_pending(struct vcpu *v, unsigned int lpi,
> > > +                                   bool allocate)
> > > +{
> > > +    struct lpi_pending_irq *lpi_irq, *empty = NULL;
> > > +
> > > +    spin_lock(&v->arch.vgic.pending_lpi_list_lock);
> > > +    list_for_each_entry(lpi_irq, &v->arch.vgic.pending_lpi_list, entry)
> > > +    {
> > > +        if ( lpi_irq->pirq.irq == lpi )
> > > +        {
> > > +            spin_unlock(&v->arch.vgic.pending_lpi_list_lock);
> > > +            return &lpi_irq->pirq;
> > > +        }
> > > +
> > > +        if ( lpi_irq->pirq.irq == 0 && !empty )
> > > +            empty = lpi_irq;
> > > +    }
> > > +
> > > +    if ( !allocate )
> > > +    {
> > > +        spin_unlock(&v->arch.vgic.pending_lpi_list_lock);
> > > +        return NULL;
> > > +    }
> > > +
> > > +    if ( !empty )
> > > +    {
> > > +        empty = xzalloc(struct lpi_pending_irq);
> > xzalloc will return NULL if memory is exhausted. There is a general lack
> > of error checking within this series. Any missing error could be a
> > potential target from a guest, leading to security issue. Stefano and I
> > already spot some, it does not mean we found all. So Before sending the
> > next version, please go through the series and verify *every* return.
> >
> > Also, I can't find the code which release LPIs neither in this patch nor
> > in this series. A general rule is too have allocation and free within
> > the same patch. It is much easier to spot missing free.
> There is no such code, on purpose. We only grow the number, but never
> shrink it (to what?, where to stop?, what if we need more again?). As
> said above, in the worst case this ends up at something where a static
> allocation would have started with from the beginning.
>
> And as mentioned, I only skipped the "free the whole list upon domain
> destruction" part.
A general rule is to explain in the commit message what is done. So you 
avoid argument on the ML.
Cheers,

--
Julien Grall

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