From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48027) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adWQH-0005rN-Qr for qemu-devel@nongnu.org; Wed, 09 Mar 2016 00:12:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adWQE-0000EU-Kj for qemu-devel@nongnu.org; Wed, 09 Mar 2016 00:12:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47351) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adWQE-0000EK-Fq for qemu-devel@nongnu.org; Wed, 09 Mar 2016 00:12:34 -0500 Date: Wed, 9 Mar 2016 13:12:21 +0800 From: Peter Xu Message-ID: <20160309051221.GL2377@pxdev.xzpeter.org> References: <1457420446-25276-1-git-send-email-peterx@redhat.com> <1457420446-25276-5-git-send-email-peterx@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 4/8] usb: fix unbounded stack for xhci_dma_write_u32s List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Paolo Bonzini , QEMU Developers , Gerd Hoffmann On Tue, Mar 08, 2016 at 02:26:36PM +0700, Peter Maydell wrote: > On 8 March 2016 at 14:00, Peter Xu 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. Sorry. Will follow this. > > > After that, make its stack bounded. > > > > Suggested-by: Paolo Bonzini > > CC: Gerd Hoffmann > > Signed-off-by: Peter Xu > > --- > > 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. I was trying to avoid the "stack unlimited" warning. There will be a warning generated before applying this patch. The patch solved this. Though I'd say this patch might not be good enough. :( Then, will drop this one too if I have no further better idea. Thanks for review. Peter