Re: [PATCH] hw/net: move allocation to the heap due to very large stack frame

[David Gibson]

On Sun, Oct 11, 2020 at 10:23:49AM +0800, Li Qiang wrote:
> David Gibson <david@gibson.dropbear.id.au> 于2020年10月10日周六 下午2:34写道：
> >
> > On Fri, Oct 09, 2020 at 07:02:56AM -0700, Elena Afanasova wrote:
> > > >From 09905773a00e417d3a37c12350d9e55466fdce8a Mon Sep 17 00:00:00 2001
> > > From: Elena Afanasova <eafanasova@gmail.com>
> > > Date: Fri, 9 Oct 2020 06:41:36 -0700
> > > Subject: [PATCH] hw/net: move allocation to the heap due to very large 
> > > stack
> > >  frame
> >
> > Patch looks fine, but some more details of the motivation would be
> > nice.  I wouldn't have thought that the size of a network packet
> > counted as a "very large" stack frame by userspace standards.
> >
> 
> It is also a best practice to avoid large stack allocation according.
> -->https://wiki.sei.cmu.edu/confluence/display/c/MEM05-C.+Avoid+large+stack+allocations

Hm, yeah, it's not really clear what "large" means in that context.
It mostly seems to be concerned with allocations controlled by an
external attacker, in which case we could be talking up to INT_MAX.
Even with jumbo frames the most we're talking here is ~64kiB.

> 
> Though I don't see any issue here.
> 
> Thanks,
> Li Qiang
> 
> > > Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
> > > ---
> > >  hw/net/spapr_llan.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> > > index 2093f1bad0..581320a0e7 100644
> > > --- a/hw/net/spapr_llan.c
> > > +++ b/hw/net/spapr_llan.c
> > > @@ -688,7 +688,8 @@ static target_ulong h_send_logical_lan(PowerPCCPU 
> > > *cpu,
> > >      SpaprVioDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
> > >      SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
> > >      unsigned total_len;
> > > -    uint8_t *lbuf, *p;
> > > +    uint8_t *p;
> > > +    g_autofree uint8_t *lbuf = NULL;
> > >      int i, nbufs;
> > >      int ret;
> > >
> > > @@ -729,7 +730,7 @@ static target_ulong h_send_logical_lan(PowerPCCPU 
> > > *cpu,
> > >          return H_RESOURCE;
> > >      }
> > >
> > > -    lbuf = alloca(total_len);
> > > +    lbuf = g_malloc(total_len);
> > >      p = lbuf;
> > >      for (i = 0; i < nbufs; i++) {
> > >          ret = spapr_vio_dma_read(sdev, VLAN_BD_ADDR(bufs[i]),
> >
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature