On Tue, Dec 1, 2020 at 9:21 PM Arnd Bergmann <[email protected]> wrote: > > On Mon, Nov 23, 2020 at 10:01 PM Norbert Slusarek <[email protected]> wrote: > > > > From: Norbert Slusarek <[email protected]> > > Date: Mon, 23 Nov 2020 21:53:41 +0100 > > Subject: [PATCH RESEND] misc/vmw_vmci: bail out earlier on too big queue > > allocation > > > > For the allocation of a queue pair in qp_host_alloc_queue() an arbitrary > > value > > can be passed for produce_size, which can lead to miscalculation of memory > > we'd > > like to allocate in one take. A warning is triggered at > > __alloc_pages_nodemask() > > in mm/page_alloc.c:4737 which aborts the false allocation, but it results > > in a > > VMware machine freezing for an infinite time. > > > > Signed-off-by: Norbert Slusarek <[email protected]> > > Thank you for sending a patch with such a detailed analysis and even > including a test case!
Yeah agree, very good details! > > > diff --git a/drivers/misc/vmw_vmci/vmci_queue_pair.c > > b/drivers/misc/vmw_vmci/vmci_queue_pair.c > > index c49065887e8f..997ff32475b2 100644 > > --- a/drivers/misc/vmw_vmci/vmci_queue_pair.c > > +++ b/drivers/misc/vmw_vmci/vmci_queue_pair.c > > @@ -526,6 +526,7 @@ static struct vmci_queue *qp_host_alloc_queue(u64 size) > > struct vmci_queue *queue; > > size_t queue_page_size; > > u64 num_pages; > > + unsigned int order; > > const size_t queue_size = sizeof(*queue) + > > sizeof(*(queue->kernel_if)); > > > > if (size > SIZE_MAX - PAGE_SIZE) > > @@ -537,6 +538,10 @@ static struct vmci_queue *qp_host_alloc_queue(u64 size) > > > > queue_page_size = num_pages * sizeof(*queue->kernel_if->u.h.page); > > > > + order = get_order(queue_size + queue_page_size); > > + if (order >= MAX_ORDER) > > + return NULL; > > + > > queue = kzalloc(queue_size + queue_page_size, GFP_KERNEL); > > Calling kzalloc() with that user-provided size may still not be a great > idea, and MAX_ORDER is probably more than anyone ever needs here > (I don't know what the interface is needed for, but usually it is). > > If there is a reasonable upper bound smaller than MAX_ORDER, I > would suggest using that. It might also be helpful to use kvzalloc()/kvfree() > in this case, which can return an arbitrary number of pages > and suffers less from fragmentation. I don't know well vmw_vmci, but I took a brief look, and I saw that there is a macro (VMCI_MAX_GUEST_QP_MEMORY) used in vmci_qpair_alloc(), I'm not sure if we can use the same macro, but maybe something similar. Honestly I don't know where that limit comes from, maybe it was chosen as an arbitrary and reasonable value but I suggest to check if we can reuse the same macro here with some adjustments. I think in some way that limit is related to the max memory that the host should allocate. @Jorgen any thought? Thanks, Stefano > > Note that both kzalloc() and kvzalloc() will fail for much smaller > sizes if the kernel is low on memory, so that might have the same > problem. > > Arnd >
