Re: [Qemu-devel] [PATCH 4/8] usb: fix unbounded stack for xhci_dma_write_u32s

[Peter Maydell]

On 8 March 2016 at 14:00, Peter Xu <address@hidden> wrote:
> First of all, this function cannot be inlined even with always_inline,
> so removing inline.

Please don't mix two different changes in one patch.

> After that, make its stack bounded.
>
> Suggested-by: Paolo Bonzini <address@hidden>
> CC: Gerd Hoffmann <address@hidden>
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  hw/usb/hcd-xhci.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 44b6f8c..3dd5a02 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -694,18 +694,22 @@ static inline void xhci_dma_read_u32s(XHCIState *xhci, 
> dma_addr_t addr,
>      }
>  }
>
> -static inline void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr,
> -                                       uint32_t *buf, size_t len)
> +static void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr,
> +                                uint32_t *buf, size_t len)
>  {
>      int i;
> -    uint32_t tmp[len / sizeof(uint32_t)];
> +    uint32_t n = len / sizeof(uint32_t);
> +#define __BUF_SIZE (12)
> +    uint32_t tmp[__BUF_SIZE];
>
> +    assert(__BUF_SIZE >= n);
>      assert((len % sizeof(uint32_t)) == 0);
>
> -    for (i = 0; i < (len / sizeof(uint32_t)); i++) {
> +    for (i = 0; i < n; i++) {
>          tmp[i] = cpu_to_le32(buf[i]);
>      }
>      pci_dma_write(PCI_DEVICE(xhci), addr, tmp, len);
> +#undef __BUF_SIZE

All the patches in this series seem to be following the
same pattern of #defining an arbitrary fixed length for
the array. This does not at all seem to me to be an
improvement. We should be avoiding unbounded stack
allocations, but we need to do this by either changing
the code to work correctly without an unbounded allocation
or by using a heap allocation instead of a stack allocation.

In some cases, like this one, the original code isn't even
unbounded -- we always call this function with a length
parameter which is a small compile time constant, so the
stack usage is definitely bounded. So this change is making
the code uglier and less flexible for no benefit that I
can see.

thanks
-- PMM