Re: [PATCH v4] gnttab: defer allocation of status frame tracking array

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 04/05/2021 09:42, Jan Beulich wrote:
> This array can be large when many grant frames are permitted; avoid
> allocating it when it's not going to be used anyway, by doing this only
> in gnttab_populate_status_frames(). While the delaying of the respective
> memory allocation adds possible reasons for failure of the respective
> enclosing operations, there are other memory allocations there already,
> so callers can't expect these operations to always succeed anyway.
>
> As to the re-ordering at the end of gnttab_unpopulate_status_frames(),
> this is merely to represent intended order of actions (shrink array
> bound, then free higher array entries). If there were racing accesses,
> suitable barriers would need adding in addition.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Nack.

The argument you make says that the grant status frames is "large
enough" to care about not allocating.  (I frankly disagree, but that
isn't relevant to my to my nack).

The change in logic here moves a failure in DOMCTL_createdomain, to
GNTTABOP_setversion.  We know, from the last minute screwups in XSA-226,
that there are versions of Windows and Linux in the field, used by
customers, which will BUG()/BSOD when GNTTABOP_setversion doesn't succeed.

You're literally adding even more complexity to the grant table, to also
increase the chance of regressing VMs in the wild.  This is not ok.

The only form of this patch which is in any way acceptable, is to avoid
the allocation when you know *in DOMCTL_createdomain* that it will never
be needed by the VM.  So far, that is from Kconfig and/or the command
line options.

~Andrew