| Message ID | 1475237495-15030-1-git-send-email-jthumshirn@suse.de |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
> + tmp = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA); > + if (tmp) { > I think that check is inverted? johannes
On Fri, Sep 30, 2016 at 02:29:39PM +0200, Johannes Berg wrote: > > > + tmp = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA); > > + if (tmp) { > > > I think that check is inverted? > > johannes Fu.. you're right, of cause it's !tmp. I'll resend. Thanks, Johannes
Hello. On 9/30/2016 3:11 PM, Johannes Thumshirn wrote: > The call to krealloc() in wsm_buf_reserve() directly assigns the newly > returned memory to buf->begin. This is all fine except when krealloc() > failes we loose the ability to free the old memory pointed to by Fails. > buf->begin. If we just create a temporary variable to assign memory to > and assign the memory to it we can mitigate the memory leak. > > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> > --- > drivers/net/wireless/st/cw1200/wsm.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/wireless/st/cw1200/wsm.c b/drivers/net/wireless/st/cw1200/wsm.c > index 680d60e..12fad99 100644 > --- a/drivers/net/wireless/st/cw1200/wsm.c > +++ b/drivers/net/wireless/st/cw1200/wsm.c > @@ -1807,16 +1807,18 @@ static int wsm_buf_reserve(struct wsm_buf *buf, size_t extra_size) > { > size_t pos = buf->data - buf->begin; > size_t size = pos + extra_size; > + u8 *tmp; > > size = round_up(size, FWLOAD_BLOCK_SIZE); > > - buf->begin = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA); > - if (buf->begin) { > - buf->data = &buf->begin[pos]; > - buf->end = &buf->begin[size]; > - return 0; > - } else { > - buf->end = buf->data = buf->begin; > + tmp = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA); > + if (tmp) { !tmp, you mean? > + wsm_buf_deinit(buf); > return -ENOMEM; > } > + > + buf->begin = tmp; > + buf->data = &buf->begin[pos]; > + buf->end = &buf->begin[size]; > + return 0; > } MBR, Sergei
On Fri, Sep 30, 2016 at 03:56:45PM +0300, Sergei Shtylyov wrote: > Hello. > > On 9/30/2016 3:11 PM, Johannes Thumshirn wrote: > > > The call to krealloc() in wsm_buf_reserve() directly assigns the newly > > returned memory to buf->begin. This is all fine except when krealloc() > > failes we loose the ability to free the old memory pointed to by > > Fails. > > > buf->begin. If we just create a temporary variable to assign memory to > > and assign the memory to it we can mitigate the memory leak. > > > > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> > > --- > > drivers/net/wireless/st/cw1200/wsm.c | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/wireless/st/cw1200/wsm.c b/drivers/net/wireless/st/cw1200/wsm.c > > index 680d60e..12fad99 100644 > > --- a/drivers/net/wireless/st/cw1200/wsm.c > > +++ b/drivers/net/wireless/st/cw1200/wsm.c > > @@ -1807,16 +1807,18 @@ static int wsm_buf_reserve(struct wsm_buf *buf, size_t extra_size) > > { > > size_t pos = buf->data - buf->begin; > > size_t size = pos + extra_size; > > + u8 *tmp; > > > > size = round_up(size, FWLOAD_BLOCK_SIZE); > > > > - buf->begin = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA); > > - if (buf->begin) { > > - buf->data = &buf->begin[pos]; > > - buf->end = &buf->begin[size]; > > - return 0; > > - } else { > > - buf->end = buf->data = buf->begin; > > + tmp = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA); > > + if (tmp) { > > !tmp, you mean? Yes, I've already sent out a v2.
diff --git a/drivers/net/wireless/st/cw1200/wsm.c b/drivers/net/wireless/st/cw1200/wsm.c index 680d60e..12fad99 100644 --- a/drivers/net/wireless/st/cw1200/wsm.c +++ b/drivers/net/wireless/st/cw1200/wsm.c @@ -1807,16 +1807,18 @@ static int wsm_buf_reserve(struct wsm_buf *buf, size_t extra_size) { size_t pos = buf->data - buf->begin; size_t size = pos + extra_size; + u8 *tmp; size = round_up(size, FWLOAD_BLOCK_SIZE); - buf->begin = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA); - if (buf->begin) { - buf->data = &buf->begin[pos]; - buf->end = &buf->begin[size]; - return 0; - } else { - buf->end = buf->data = buf->begin; + tmp = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA); + if (tmp) { + wsm_buf_deinit(buf); return -ENOMEM; } + + buf->begin = tmp; + buf->data = &buf->begin[pos]; + buf->end = &buf->begin[size]; + return 0; }
The call to krealloc() in wsm_buf_reserve() directly assigns the newly returned memory to buf->begin. This is all fine except when krealloc() failes we loose the ability to free the old memory pointed to by buf->begin. If we just create a temporary variable to assign memory to and assign the memory to it we can mitigate the memory leak. Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> --- drivers/net/wireless/st/cw1200/wsm.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)