* [PATCH] atm: dereference of he_dev->rbps_virt in he_init_group() @ 2009-08-29 18:56 Roel Kluin 2009-08-29 18:59 ` Roel Kluin 0 siblings, 1 reply; 14+ messages in thread From: Roel Kluin @ 2009-08-29 18:56 UTC (permalink / raw) To: Chas Williams, linux-atm-general, netdev, Andrew Morton he_dev->rbps_virt allocation may fail. Signed-off-by: Roel Kluin <roel.kluin@gmail.com> --- diff --git a/drivers/atm/he.c b/drivers/atm/he.c index 2de6406..2d766d6 100644 --- a/drivers/atm/he.c +++ b/drivers/atm/he.c @@ -795,6 +795,8 @@ he_init_group(struct he_dev *he_dev, int group) } memset(he_dev->rbps_base, 0, CONFIG_RBPS_SIZE * sizeof(struct he_rbp)); he_dev->rbps_virt = kmalloc(CONFIG_RBPS_SIZE * sizeof(struct he_virt), GFP_KERNEL); + if (he_dev->rbps_virt == NULL) + return -ENOMEM; for (i = 0; i < CONFIG_RBPS_SIZE; ++i) { dma_addr_t dma_handle; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] atm: dereference of he_dev->rbps_virt in he_init_group() 2009-08-29 18:56 [PATCH] atm: dereference of he_dev->rbps_virt in he_init_group() Roel Kluin @ 2009-08-29 18:59 ` Roel Kluin 2009-09-03 6:25 ` David Miller 0 siblings, 1 reply; 14+ messages in thread From: Roel Kluin @ 2009-08-29 18:59 UTC (permalink / raw) To: Roel Kluin; +Cc: Chas Williams, linux-atm-general, netdev, Andrew Morton he_dev->rbps_virt or he_dev->rbpl_virt allocation may fail. Signed-off-by: Roel Kluin <roel.kluin@gmail.com> --- There was another in the same function. diff --git a/drivers/atm/he.c b/drivers/atm/he.c index 2de6406..8af1d3e 100644 --- a/drivers/atm/he.c +++ b/drivers/atm/he.c @@ -795,6 +795,8 @@ he_init_group(struct he_dev *he_dev, int group) } memset(he_dev->rbps_base, 0, CONFIG_RBPS_SIZE * sizeof(struct he_rbp)); he_dev->rbps_virt = kmalloc(CONFIG_RBPS_SIZE * sizeof(struct he_virt), GFP_KERNEL); + if (he_dev->rbps_virt == NULL) + return -ENOMEM; for (i = 0; i < CONFIG_RBPS_SIZE; ++i) { dma_addr_t dma_handle; @@ -838,6 +840,8 @@ he_init_group(struct he_dev *he_dev, int group) } memset(he_dev->rbpl_base, 0, CONFIG_RBPL_SIZE * sizeof(struct he_rbp)); he_dev->rbpl_virt = kmalloc(CONFIG_RBPL_SIZE * sizeof(struct he_virt), GFP_KERNEL); + if (he_dev->rbpl_virt == NULL) + return -ENOMEM; for (i = 0; i < CONFIG_RBPL_SIZE; ++i) { dma_addr_t dma_handle; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] atm: dereference of he_dev->rbps_virt in he_init_group() 2009-08-29 18:59 ` Roel Kluin @ 2009-09-03 6:25 ` David Miller 2009-09-05 12:35 ` Roel Kluin 0 siblings, 1 reply; 14+ messages in thread From: David Miller @ 2009-09-03 6:25 UTC (permalink / raw) To: roel.kluin; +Cc: chas, linux-atm-general, netdev, akpm From: Roel Kluin <roel.kluin@gmail.com> Date: Sat, 29 Aug 2009 20:59:04 +0200 > he_dev->rbps_virt or he_dev->rbpl_virt allocation may fail. > > Signed-off-by: Roel Kluin <roel.kluin@gmail.com> > --- > There was another in the same function. These new return statements will both leak resources allocated earlier. All the caller is going to do is return -ENOMEM as well and it does not cleanup actions at all. Please fix this up. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] atm: dereference of he_dev->rbps_virt in he_init_group() 2009-09-03 6:25 ` David Miller @ 2009-09-05 12:35 ` Roel Kluin 2009-09-11 19:37 ` David Miller 0 siblings, 1 reply; 14+ messages in thread From: Roel Kluin @ 2009-09-05 12:35 UTC (permalink / raw) To: David Miller; +Cc: chas, linux-atm-general, netdev, akpm he_dev->rbps_virt or he_dev->rbpl_virt allocation may fail, so check them. Make sure that he_init_group() cleans up after errors. Signed-off-by: Roel Kluin <roel.kluin@gmail.com> --- > These new return statements will both leak resources allocated > earlier. > > All the caller is going to do is return -ENOMEM as well and > it does not cleanup actions at all. > > Please fix this up. I am new to this api, so please review. diff --git a/drivers/atm/he.c b/drivers/atm/he.c index 2de6406..1d873cb 100644 --- a/drivers/atm/he.c +++ b/drivers/atm/he.c @@ -777,7 +777,7 @@ he_init_cs_block_rcm(struct he_dev *he_dev) static int __devinit he_init_group(struct he_dev *he_dev, int group) { - int i; + int i, ret; /* small buffer pool */ he_dev->rbps_pool = pci_pool_create("rbps", he_dev->pci_dev, @@ -790,19 +790,27 @@ he_init_group(struct he_dev *he_dev, int group) he_dev->rbps_base = pci_alloc_consistent(he_dev->pci_dev, CONFIG_RBPS_SIZE * sizeof(struct he_rbp), &he_dev->rbps_phys); if (he_dev->rbps_base == NULL) { - hprintk("failed to alloc rbps\n"); - return -ENOMEM; + hprintk("failed to alloc rbps_base\n"); + ret = -ENOMEM; + goto out_destroy_rbps_pool; } memset(he_dev->rbps_base, 0, CONFIG_RBPS_SIZE * sizeof(struct he_rbp)); he_dev->rbps_virt = kmalloc(CONFIG_RBPS_SIZE * sizeof(struct he_virt), GFP_KERNEL); + if (he_dev->rbps_virt == NULL) { + hprintk("failed to alloc rbps_virt\n"); + ret = -ENOMEM; + goto out_free_rbps_base; + } for (i = 0; i < CONFIG_RBPS_SIZE; ++i) { dma_addr_t dma_handle; void *cpuaddr; cpuaddr = pci_pool_alloc(he_dev->rbps_pool, GFP_KERNEL|GFP_DMA, &dma_handle); - if (cpuaddr == NULL) - return -ENOMEM; + if (cpuaddr == NULL) { + ret = -ENOMEM; + goto out_free_rbps_virt; + } he_dev->rbps_virt[i].virt = cpuaddr; he_dev->rbps_base[i].status = RBP_LOANED | RBP_SMALLBUF | (i << RBP_INDEX_OFF); @@ -827,25 +835,34 @@ he_init_group(struct he_dev *he_dev, int group) CONFIG_RBPL_BUFSIZE, 8, 0); if (he_dev->rbpl_pool == NULL) { hprintk("unable to create rbpl pool\n"); - return -ENOMEM; + ret = -ENOMEM; + goto out_free_rbps_virt; } he_dev->rbpl_base = pci_alloc_consistent(he_dev->pci_dev, CONFIG_RBPL_SIZE * sizeof(struct he_rbp), &he_dev->rbpl_phys); if (he_dev->rbpl_base == NULL) { - hprintk("failed to alloc rbpl\n"); - return -ENOMEM; + hprintk("failed to alloc rbpl_base\n"); + ret = -ENOMEM; + goto out_destroy_rbpl_pool; } memset(he_dev->rbpl_base, 0, CONFIG_RBPL_SIZE * sizeof(struct he_rbp)); he_dev->rbpl_virt = kmalloc(CONFIG_RBPL_SIZE * sizeof(struct he_virt), GFP_KERNEL); + if (he_dev->rbpl_virt == NULL) { + hprintk("failed to alloc rbpl_virt\n"); + ret = -ENOMEM; + goto out_free_rbpl_base; + } for (i = 0; i < CONFIG_RBPL_SIZE; ++i) { dma_addr_t dma_handle; void *cpuaddr; cpuaddr = pci_pool_alloc(he_dev->rbpl_pool, GFP_KERNEL|GFP_DMA, &dma_handle); - if (cpuaddr == NULL) - return -ENOMEM; + if (cpuaddr == NULL) { + ret = -ENOMEM; + goto out_free_rbpl_virt; + } he_dev->rbpl_virt[i].virt = cpuaddr; he_dev->rbpl_base[i].status = RBP_LOANED | (i << RBP_INDEX_OFF); @@ -870,7 +887,8 @@ he_init_group(struct he_dev *he_dev, int group) CONFIG_RBRQ_SIZE * sizeof(struct he_rbrq), &he_dev->rbrq_phys); if (he_dev->rbrq_base == NULL) { hprintk("failed to allocate rbrq\n"); - return -ENOMEM; + ret = -ENOMEM; + goto out_free_rbpl_virt; } memset(he_dev->rbrq_base, 0, CONFIG_RBRQ_SIZE * sizeof(struct he_rbrq)); @@ -894,7 +912,8 @@ he_init_group(struct he_dev *he_dev, int group) CONFIG_TBRQ_SIZE * sizeof(struct he_tbrq), &he_dev->tbrq_phys); if (he_dev->tbrq_base == NULL) { hprintk("failed to allocate tbrq\n"); - return -ENOMEM; + ret = -ENOMEM; + goto out_free_rbpq_base; } memset(he_dev->tbrq_base, 0, CONFIG_TBRQ_SIZE * sizeof(struct he_tbrq)); @@ -906,6 +925,34 @@ he_init_group(struct he_dev *he_dev, int group) he_writel(he_dev, CONFIG_TBRQ_THRESH, G0_TBRQ_THRESH + (group * 16)); return 0; + +out_free_rbpq_base: + pci_free_consistent(he_dev->pci_dev, CONFIG_RBRQ_SIZE * sizeof(struct he_rbrq), + he_dev->rbrq_base, he_dev->rbrq_phys); + i = CONFIG_RBPL_SIZE; +out_free_rbpl_virt: + while (--i) + pci_pool_free(he_dev->rbpl_virt[i].virt); + kfree(he_dev->rbpl_virt); + +out_free_rbpl_base: + pci_free_consistent(he_dev->pci_dev, CONFIG_RBPL_SIZE * sizeof(struct he_rbp), + he_dev->rbpl_base, he_dev->rbpl_phys); +out_destroy_rbpl_pool: + pci_pool_destroy(he_dev->rbpl_pool); + + i = CONFIG_RBPL_SIZE; +out_free_rbps_virt: + while (--i) + pci_pool_free(he_dev->rbps_virt[i].virt); + kfree(he_dev->rbps_virt); + +out_free_rbps_base: + pci_free_consistent(he_dev->pci_dev, CONFIG_RBPS_SIZE * sizeof(struct he_rbp), + he_dev->rbps_base, he_dev->rbps_phys); +out_destroy_rbps_pool: + pci_pool_destroy(he_dev->rbps_pool); + return ret; } static int __devinit ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] atm: dereference of he_dev->rbps_virt in he_init_group() 2009-09-05 12:35 ` Roel Kluin @ 2009-09-11 19:37 ` David Miller 2009-09-11 19:51 ` David Miller 0 siblings, 1 reply; 14+ messages in thread From: David Miller @ 2009-09-11 19:37 UTC (permalink / raw) To: roel.kluin; +Cc: chas, linux-atm-general, netdev, akpm From: Roel Kluin <roel.kluin@gmail.com> Date: Sat, 05 Sep 2009 14:35:18 +0200 > he_dev->rbps_virt or he_dev->rbpl_virt allocation may fail, so check > them. Make sure that he_init_group() cleans up after errors. > > Signed-off-by: Roel Kluin <roel.kluin@gmail.com> > --- >> These new return statements will both leak resources allocated >> earlier. >> >> All the caller is going to do is return -ENOMEM as well and >> it does not cleanup actions at all. >> >> Please fix this up. > > I am new to this api, so please review. Looks ok, applied, thanks. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] atm: dereference of he_dev->rbps_virt in he_init_group() 2009-09-11 19:37 ` David Miller @ 2009-09-11 19:51 ` David Miller 2009-09-19 18:16 ` Roel Kluin 0 siblings, 1 reply; 14+ messages in thread From: David Miller @ 2009-09-11 19:51 UTC (permalink / raw) To: roel.kluin; +Cc: chas, linux-atm-general, netdev, akpm From: David Miller <davem@davemloft.net> Date: Fri, 11 Sep 2009 12:37:25 -0700 (PDT) > From: Roel Kluin <roel.kluin@gmail.com> > Date: Sat, 05 Sep 2009 14:35:18 +0200 > >> he_dev->rbps_virt or he_dev->rbpl_virt allocation may fail, so check >> them. Make sure that he_init_group() cleans up after errors. >> >> Signed-off-by: Roel Kluin <roel.kluin@gmail.com> >> --- >>> These new return statements will both leak resources allocated >>> earlier. >>> >>> All the caller is going to do is return -ENOMEM as well and >>> it does not cleanup actions at all. >>> >>> Please fix this up. >> >> I am new to this api, so please review. > > Looks ok, applied, thanks. Sorry I have to revert, you didn't even build test this. Roel, I know you want me to take your changes serious, but I absolutely cannot when you do stuff like this. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] atm: dereference of he_dev->rbps_virt in he_init_group() 2009-09-11 19:51 ` David Miller @ 2009-09-19 18:16 ` Roel Kluin 2009-09-19 18:27 ` Joe Perches 0 siblings, 1 reply; 14+ messages in thread From: Roel Kluin @ 2009-09-19 18:16 UTC (permalink / raw) To: David Miller; +Cc: chas, linux-atm-general, netdev, akpm he_dev->rbps_virt or he_dev->rbpl_virt allocation may fail, s them. Make sure that he_init_group() cleans up after errors. Signed-off-by: Roel Kluin <roel.kluin@gmail.com> --- Found with sed: http://kernelnewbies.org/roelkluin This version was build, sparse and checkpatch tested. diff --git a/drivers/atm/he.c b/drivers/atm/he.c index 2de6406..00fd67e 100644 --- a/drivers/atm/he.c +++ b/drivers/atm/he.c @@ -777,7 +777,7 @@ he_init_cs_block_rcm(struct he_dev *he_dev) static int __devinit he_init_group(struct he_dev *he_dev, int group) { - int i; + int i, ret; /* small buffer pool */ he_dev->rbps_pool = pci_pool_create("rbps", he_dev->pci_dev, @@ -790,19 +790,27 @@ he_init_group(struct he_dev *he_dev, int group) he_dev->rbps_base = pci_alloc_consistent(he_dev->pci_dev, CONFIG_RBPS_SIZE * sizeof(struct he_rbp), &he_dev->rbps_phys); if (he_dev->rbps_base == NULL) { - hprintk("failed to alloc rbps\n"); - return -ENOMEM; + hprintk("failed to alloc rbps_base\n"); + ret = -ENOMEM; + goto out_destroy_rbps_pool; } memset(he_dev->rbps_base, 0, CONFIG_RBPS_SIZE * sizeof(struct he_rbp)); he_dev->rbps_virt = kmalloc(CONFIG_RBPS_SIZE * sizeof(struct he_virt), GFP_KERNEL); + if (he_dev->rbps_virt == NULL) { + hprintk("failed to alloc rbps_virt\n"); + ret = -ENOMEM; + goto out_free_rbps_base; + } for (i = 0; i < CONFIG_RBPS_SIZE; ++i) { dma_addr_t dma_handle; void *cpuaddr; cpuaddr = pci_pool_alloc(he_dev->rbps_pool, GFP_KERNEL|GFP_DMA, &dma_handle); - if (cpuaddr == NULL) - return -ENOMEM; + if (cpuaddr == NULL) { + ret = -ENOMEM; + goto out_free_rbps_virt; + } he_dev->rbps_virt[i].virt = cpuaddr; he_dev->rbps_base[i].status = RBP_LOANED | RBP_SMALLBUF | (i << RBP_INDEX_OFF); @@ -827,25 +835,34 @@ he_init_group(struct he_dev *he_dev, int group) CONFIG_RBPL_BUFSIZE, 8, 0); if (he_dev->rbpl_pool == NULL) { hprintk("unable to create rbpl pool\n"); - return -ENOMEM; + ret = -ENOMEM; + goto out_free_rbps_virt; } he_dev->rbpl_base = pci_alloc_consistent(he_dev->pci_dev, CONFIG_RBPL_SIZE * sizeof(struct he_rbp), &he_dev->rbpl_phys); if (he_dev->rbpl_base == NULL) { - hprintk("failed to alloc rbpl\n"); - return -ENOMEM; + hprintk("failed to alloc rbpl_base\n"); + ret = -ENOMEM; + goto out_destroy_rbpl_pool; } memset(he_dev->rbpl_base, 0, CONFIG_RBPL_SIZE * sizeof(struct he_rbp)); he_dev->rbpl_virt = kmalloc(CONFIG_RBPL_SIZE * sizeof(struct he_virt), GFP_KERNEL); + if (he_dev->rbpl_virt == NULL) { + hprintk("failed to alloc rbpl_virt\n"); + ret = -ENOMEM; + goto out_free_rbpl_base; + } for (i = 0; i < CONFIG_RBPL_SIZE; ++i) { dma_addr_t dma_handle; void *cpuaddr; cpuaddr = pci_pool_alloc(he_dev->rbpl_pool, GFP_KERNEL|GFP_DMA, &dma_handle); - if (cpuaddr == NULL) - return -ENOMEM; + if (cpuaddr == NULL) { + ret = -ENOMEM; + goto out_free_rbpl_virt; + } he_dev->rbpl_virt[i].virt = cpuaddr; he_dev->rbpl_base[i].status = RBP_LOANED | (i << RBP_INDEX_OFF); @@ -870,7 +887,8 @@ he_init_group(struct he_dev *he_dev, int group) CONFIG_RBRQ_SIZE * sizeof(struct he_rbrq), &he_dev->rbrq_phys); if (he_dev->rbrq_base == NULL) { hprintk("failed to allocate rbrq\n"); - return -ENOMEM; + ret = -ENOMEM; + goto out_free_rbpl_virt; } memset(he_dev->rbrq_base, 0, CONFIG_RBRQ_SIZE * sizeof(struct he_rbrq)); @@ -894,7 +912,8 @@ he_init_group(struct he_dev *he_dev, int group) CONFIG_TBRQ_SIZE * sizeof(struct he_tbrq), &he_dev->tbrq_phys); if (he_dev->tbrq_base == NULL) { hprintk("failed to allocate tbrq\n"); - return -ENOMEM; + ret = -ENOMEM; + goto out_free_rbpq_base; } memset(he_dev->tbrq_base, 0, CONFIG_TBRQ_SIZE * sizeof(struct he_tbrq)); @@ -906,6 +925,39 @@ he_init_group(struct he_dev *he_dev, int group) he_writel(he_dev, CONFIG_TBRQ_THRESH, G0_TBRQ_THRESH + (group * 16)); return 0; + +out_free_rbpq_base: + pci_free_consistent(he_dev->pci_dev, CONFIG_RBRQ_SIZE * + sizeof(struct he_rbrq), he_dev->rbrq_base, + he_dev->rbrq_phys); + i = CONFIG_RBPL_SIZE; +out_free_rbpl_virt: + while (--i) + pci_pool_free(he_dev->rbps_pool, he_dev->rbpl_virt[i].virt, + he_dev->rbps_base[i].phys); + kfree(he_dev->rbpl_virt); + +out_free_rbpl_base: + pci_free_consistent(he_dev->pci_dev, CONFIG_RBPL_SIZE * + sizeof(struct he_rbp), he_dev->rbpl_base, + he_dev->rbpl_phys); +out_destroy_rbpl_pool: + pci_pool_destroy(he_dev->rbpl_pool); + + i = CONFIG_RBPL_SIZE; +out_free_rbps_virt: + while (--i) + pci_pool_free(he_dev->rbpl_pool, he_dev->rbps_virt[i].virt, + he_dev->rbpl_base[i].phys); + kfree(he_dev->rbps_virt); + +out_free_rbps_base: + pci_free_consistent(he_dev->pci_dev, CONFIG_RBPS_SIZE * + sizeof(struct he_rbp), he_dev->rbps_base, + he_dev->rbps_phys); +out_destroy_rbps_pool: + pci_pool_destroy(he_dev->rbps_pool); + return ret; } static int __devinit ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] atm: dereference of he_dev->rbps_virt in he_init_group() 2009-09-19 18:16 ` Roel Kluin @ 2009-09-19 18:27 ` Joe Perches 2009-09-20 17:11 ` Roel Kluin 0 siblings, 1 reply; 14+ messages in thread From: Joe Perches @ 2009-09-19 18:27 UTC (permalink / raw) To: Roel Kluin; +Cc: David Miller, chas, linux-atm-general, netdev, akpm On Sat, 2009-09-19 at 20:16 +0200, Roel Kluin wrote: > + int i, ret; > + ret = -ENOMEM; > + ret = -ENOMEM; > + ret = -ENOMEM; > + ret = -ENOMEM; > + ret = -ENOMEM; > + ret = -ENOMEM; > + ret = -ENOMEM; > + ret = -ENOMEM; > +out_destroy_rbps_pool: > + pci_pool_destroy(he_dev->rbps_pool); > + return ret; It looks as if it'd be clearer to not use variable ret and simply return -ENOMEM after the out_destroy_rbps_pool label. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] atm: dereference of he_dev->rbps_virt in he_init_group() 2009-09-19 18:27 ` Joe Perches @ 2009-09-20 17:11 ` Roel Kluin 2009-09-22 21:25 ` David Miller 0 siblings, 1 reply; 14+ messages in thread From: Roel Kluin @ 2009-09-20 17:11 UTC (permalink / raw) To: Joe Perches; +Cc: David Miller, chas, linux-atm-general, netdev, akpm he_dev->rbps_virt or he_dev->rbpl_virt allocation may fail, s them. Make sure that he_init_group() cleans up after errors. Signed-off-by: Roel Kluin <roel.kluin@gmail.com> --- > It looks as if it'd be clearer to not use variable ret and > simply return -ENOMEM after the out_destroy_rbps_pool label. Ok, thanks. Again build, sparse and checkpatch tested. diff --git a/drivers/atm/he.c b/drivers/atm/he.c index 2de6406..29e66d6 100644 --- a/drivers/atm/he.c +++ b/drivers/atm/he.c @@ -790,11 +790,15 @@ he_init_group(struct he_dev *he_dev, int group) he_dev->rbps_base = pci_alloc_consistent(he_dev->pci_dev, CONFIG_RBPS_SIZE * sizeof(struct he_rbp), &he_dev->rbps_phys); if (he_dev->rbps_base == NULL) { - hprintk("failed to alloc rbps\n"); - return -ENOMEM; + hprintk("failed to alloc rbps_base\n"); + goto out_destroy_rbps_pool; } memset(he_dev->rbps_base, 0, CONFIG_RBPS_SIZE * sizeof(struct he_rbp)); he_dev->rbps_virt = kmalloc(CONFIG_RBPS_SIZE * sizeof(struct he_virt), GFP_KERNEL); + if (he_dev->rbps_virt == NULL) { + hprintk("failed to alloc rbps_virt\n"); + goto out_free_rbps_base; + } for (i = 0; i < CONFIG_RBPS_SIZE; ++i) { dma_addr_t dma_handle; @@ -802,7 +806,7 @@ he_init_group(struct he_dev *he_dev, int group) cpuaddr = pci_pool_alloc(he_dev->rbps_pool, GFP_KERNEL|GFP_DMA, &dma_handle); if (cpuaddr == NULL) - return -ENOMEM; + goto out_free_rbps_virt; he_dev->rbps_virt[i].virt = cpuaddr; he_dev->rbps_base[i].status = RBP_LOANED | RBP_SMALLBUF | (i << RBP_INDEX_OFF); @@ -827,17 +831,21 @@ he_init_group(struct he_dev *he_dev, int group) CONFIG_RBPL_BUFSIZE, 8, 0); if (he_dev->rbpl_pool == NULL) { hprintk("unable to create rbpl pool\n"); - return -ENOMEM; + goto out_free_rbps_virt; } he_dev->rbpl_base = pci_alloc_consistent(he_dev->pci_dev, CONFIG_RBPL_SIZE * sizeof(struct he_rbp), &he_dev->rbpl_phys); if (he_dev->rbpl_base == NULL) { - hprintk("failed to alloc rbpl\n"); - return -ENOMEM; + hprintk("failed to alloc rbpl_base\n"); + goto out_destroy_rbpl_pool; } memset(he_dev->rbpl_base, 0, CONFIG_RBPL_SIZE * sizeof(struct he_rbp)); he_dev->rbpl_virt = kmalloc(CONFIG_RBPL_SIZE * sizeof(struct he_virt), GFP_KERNEL); + if (he_dev->rbpl_virt == NULL) { + hprintk("failed to alloc rbpl_virt\n"); + goto out_free_rbpl_base; + } for (i = 0; i < CONFIG_RBPL_SIZE; ++i) { dma_addr_t dma_handle; @@ -845,7 +853,7 @@ he_init_group(struct he_dev *he_dev, int group) cpuaddr = pci_pool_alloc(he_dev->rbpl_pool, GFP_KERNEL|GFP_DMA, &dma_handle); if (cpuaddr == NULL) - return -ENOMEM; + goto out_free_rbpl_virt; he_dev->rbpl_virt[i].virt = cpuaddr; he_dev->rbpl_base[i].status = RBP_LOANED | (i << RBP_INDEX_OFF); @@ -870,7 +878,7 @@ he_init_group(struct he_dev *he_dev, int group) CONFIG_RBRQ_SIZE * sizeof(struct he_rbrq), &he_dev->rbrq_phys); if (he_dev->rbrq_base == NULL) { hprintk("failed to allocate rbrq\n"); - return -ENOMEM; + goto out_free_rbpl_virt; } memset(he_dev->rbrq_base, 0, CONFIG_RBRQ_SIZE * sizeof(struct he_rbrq)); @@ -894,7 +902,7 @@ he_init_group(struct he_dev *he_dev, int group) CONFIG_TBRQ_SIZE * sizeof(struct he_tbrq), &he_dev->tbrq_phys); if (he_dev->tbrq_base == NULL) { hprintk("failed to allocate tbrq\n"); - return -ENOMEM; + goto out_free_rbpq_base; } memset(he_dev->tbrq_base, 0, CONFIG_TBRQ_SIZE * sizeof(struct he_tbrq)); @@ -906,6 +914,39 @@ he_init_group(struct he_dev *he_dev, int group) he_writel(he_dev, CONFIG_TBRQ_THRESH, G0_TBRQ_THRESH + (group * 16)); return 0; + +out_free_rbpq_base: + pci_free_consistent(he_dev->pci_dev, CONFIG_RBRQ_SIZE * + sizeof(struct he_rbrq), he_dev->rbrq_base, + he_dev->rbrq_phys); + i = CONFIG_RBPL_SIZE; +out_free_rbpl_virt: + while (--i) + pci_pool_free(he_dev->rbps_pool, he_dev->rbpl_virt[i].virt, + he_dev->rbps_base[i].phys); + kfree(he_dev->rbpl_virt); + +out_free_rbpl_base: + pci_free_consistent(he_dev->pci_dev, CONFIG_RBPL_SIZE * + sizeof(struct he_rbp), he_dev->rbpl_base, + he_dev->rbpl_phys); +out_destroy_rbpl_pool: + pci_pool_destroy(he_dev->rbpl_pool); + + i = CONFIG_RBPL_SIZE; +out_free_rbps_virt: + while (--i) + pci_pool_free(he_dev->rbpl_pool, he_dev->rbps_virt[i].virt, + he_dev->rbpl_base[i].phys); + kfree(he_dev->rbps_virt); + +out_free_rbps_base: + pci_free_consistent(he_dev->pci_dev, CONFIG_RBPS_SIZE * + sizeof(struct he_rbp), he_dev->rbps_base, + he_dev->rbps_phys); +out_destroy_rbps_pool: + pci_pool_destroy(he_dev->rbps_pool); + return -ENOMEM; } static int __devinit ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] atm: dereference of he_dev->rbps_virt in he_init_group() 2009-09-20 17:11 ` Roel Kluin @ 2009-09-22 21:25 ` David Miller 2009-09-26 13:20 ` Roel Kluin 0 siblings, 1 reply; 14+ messages in thread From: David Miller @ 2009-09-22 21:25 UTC (permalink / raw) To: roel.kluin; +Cc: joe, chas, linux-atm-general, netdev, akpm From: Roel Kluin <roel.kluin@gmail.com> Date: Sun, 20 Sep 2009 19:11:28 +0200 > he_dev->rbps_virt or he_dev->rbpl_virt allocation may fail, s > them. Make sure that he_init_group() cleans up after errors. > > Signed-off-by: Roel Kluin <roel.kluin@gmail.com> Applied. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] atm: dereference of he_dev->rbps_virt in he_init_group() 2009-09-22 21:25 ` David Miller @ 2009-09-26 13:20 ` Roel Kluin 2009-09-27 3:26 ` David Miller 0 siblings, 1 reply; 14+ messages in thread From: Roel Kluin @ 2009-09-26 13:20 UTC (permalink / raw) To: David Miller; +Cc: joe, chas, linux-atm-general, netdev, akpm he_dev->rbps_virt or he_dev->rbpl_virt allocation may fail, so check them. Make sure that he_init_group() cleans up after errors. Signed-off-by: Roel Kluin <roel.kluin@gmail.com> Signed-off-by: "Juha Leppanen" <juha_motorsportcom@luukku.com> --- David, I swapped rbps and rbpl arguments in my last patch, and there were some other problems. This was pointed out by Juha Leppanen. Can you please replace the former patch by this one? This was version was build, sparse and checkpatch tested. Sorry for the mess. Roel diff --git a/drivers/atm/he.c b/drivers/atm/he.c index 2de6406..1b0f189 100644 --- a/drivers/atm/he.c +++ b/drivers/atm/he.c @@ -777,7 +777,7 @@ he_init_cs_block_rcm(struct he_dev *he_dev) static int __devinit he_init_group(struct he_dev *he_dev, int group) { - int i; + int i, ret; /* small buffer pool */ he_dev->rbps_pool = pci_pool_create("rbps", he_dev->pci_dev, @@ -790,19 +790,27 @@ he_init_group(struct he_dev *he_dev, int group) he_dev->rbps_base = pci_alloc_consistent(he_dev->pci_dev, CONFIG_RBPS_SIZE * sizeof(struct he_rbp), &he_dev->rbps_phys); if (he_dev->rbps_base == NULL) { - hprintk("failed to alloc rbps\n"); - return -ENOMEM; + hprintk("failed to alloc rbps_base\n"); + ret = -ENOMEM; + goto out_destroy_rbps_pool; } memset(he_dev->rbps_base, 0, CONFIG_RBPS_SIZE * sizeof(struct he_rbp)); he_dev->rbps_virt = kmalloc(CONFIG_RBPS_SIZE * sizeof(struct he_virt), GFP_KERNEL); + if (he_dev->rbps_virt == NULL) { + hprintk("failed to alloc rbps_virt\n"); + ret = -ENOMEM; + goto out_free_rbps_base; + } for (i = 0; i < CONFIG_RBPS_SIZE; ++i) { dma_addr_t dma_handle; void *cpuaddr; cpuaddr = pci_pool_alloc(he_dev->rbps_pool, GFP_KERNEL|GFP_DMA, &dma_handle); - if (cpuaddr == NULL) - return -ENOMEM; + if (cpuaddr == NULL) { + ret = -ENOMEM; + goto out_free_rbps_virt; + } he_dev->rbps_virt[i].virt = cpuaddr; he_dev->rbps_base[i].status = RBP_LOANED | RBP_SMALLBUF | (i << RBP_INDEX_OFF); @@ -827,25 +835,34 @@ he_init_group(struct he_dev *he_dev, int group) CONFIG_RBPL_BUFSIZE, 8, 0); if (he_dev->rbpl_pool == NULL) { hprintk("unable to create rbpl pool\n"); - return -ENOMEM; + ret = -ENOMEM; + goto out_free_rbps_virt; } he_dev->rbpl_base = pci_alloc_consistent(he_dev->pci_dev, CONFIG_RBPL_SIZE * sizeof(struct he_rbp), &he_dev->rbpl_phys); if (he_dev->rbpl_base == NULL) { - hprintk("failed to alloc rbpl\n"); - return -ENOMEM; + hprintk("failed to alloc rbpl_base\n"); + ret = -ENOMEM; + goto out_destroy_rbpl_pool; } memset(he_dev->rbpl_base, 0, CONFIG_RBPL_SIZE * sizeof(struct he_rbp)); he_dev->rbpl_virt = kmalloc(CONFIG_RBPL_SIZE * sizeof(struct he_virt), GFP_KERNEL); + if (he_dev->rbpl_virt == NULL) { + hprintk("failed to alloc rbpl_virt\n"); + ret = -ENOMEM; + goto out_free_rbpl_base; + } for (i = 0; i < CONFIG_RBPL_SIZE; ++i) { dma_addr_t dma_handle; void *cpuaddr; cpuaddr = pci_pool_alloc(he_dev->rbpl_pool, GFP_KERNEL|GFP_DMA, &dma_handle); - if (cpuaddr == NULL) - return -ENOMEM; + if (cpuaddr == NULL) { + ret = -ENOMEM; + goto out_free_rbpl_virt; + } he_dev->rbpl_virt[i].virt = cpuaddr; he_dev->rbpl_base[i].status = RBP_LOANED | (i << RBP_INDEX_OFF); @@ -870,7 +887,8 @@ he_init_group(struct he_dev *he_dev, int group) CONFIG_RBRQ_SIZE * sizeof(struct he_rbrq), &he_dev->rbrq_phys); if (he_dev->rbrq_base == NULL) { hprintk("failed to allocate rbrq\n"); - return -ENOMEM; + ret = -ENOMEM; + goto out_free_rbpl_virt; } memset(he_dev->rbrq_base, 0, CONFIG_RBRQ_SIZE * sizeof(struct he_rbrq)); @@ -894,7 +912,8 @@ he_init_group(struct he_dev *he_dev, int group) CONFIG_TBRQ_SIZE * sizeof(struct he_tbrq), &he_dev->tbrq_phys); if (he_dev->tbrq_base == NULL) { hprintk("failed to allocate tbrq\n"); - return -ENOMEM; + ret = -ENOMEM; + goto out_free_rbpq_base; } memset(he_dev->tbrq_base, 0, CONFIG_TBRQ_SIZE * sizeof(struct he_tbrq)); @@ -906,6 +925,39 @@ he_init_group(struct he_dev *he_dev, int group) he_writel(he_dev, CONFIG_TBRQ_THRESH, G0_TBRQ_THRESH + (group * 16)); return 0; + +out_free_rbpq_base: + pci_free_consistent(he_dev->pci_dev, CONFIG_RBRQ_SIZE * + sizeof(struct he_rbrq), he_dev->rbrq_base, + he_dev->rbrq_phys); + i = CONFIG_RBPL_SIZE; +out_free_rbpl_virt: + while (i--) + pci_pool_free(he_dev->rbpl_pool, he_dev->rbpl_virt[i].virt, + he_dev->rbpl_base[i].phys); + kfree(he_dev->rbpl_virt); + +out_free_rbpl_base: + pci_free_consistent(he_dev->pci_dev, CONFIG_RBPL_SIZE * + sizeof(struct he_rbp), he_dev->rbpl_base, + he_dev->rbpl_phys); +out_destroy_rbpl_pool: + pci_pool_destroy(he_dev->rbpl_pool); + + i = CONFIG_RBPS_SIZE; +out_free_rbps_virt: + while (i--) + pci_pool_free(he_dev->rbps_pool, he_dev->rbps_virt[i].virt, + he_dev->rbps_base[i].phys); + kfree(he_dev->rbps_virt); + +out_free_rbps_base: + pci_free_consistent(he_dev->pci_dev, CONFIG_RBPS_SIZE * + sizeof(struct he_rbp), he_dev->rbps_base, + he_dev->rbps_phys); +out_destroy_rbps_pool: + pci_pool_destroy(he_dev->rbps_pool); + return ret; } static int __devinit ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] atm: dereference of he_dev->rbps_virt in he_init_group() 2009-09-26 13:20 ` Roel Kluin @ 2009-09-27 3:26 ` David Miller 2009-09-28 8:58 ` Roel Kluin 0 siblings, 1 reply; 14+ messages in thread From: David Miller @ 2009-09-27 3:26 UTC (permalink / raw) To: roel.kluin; +Cc: joe, chas, linux-atm-general, netdev, akpm From: Roel Kluin <roel.kluin@gmail.com> Date: Sat, 26 Sep 2009 15:20:47 +0200 > he_dev->rbps_virt or he_dev->rbpl_virt allocation may fail, so > check them. Make sure that he_init_group() cleans up after > errors. > > Signed-off-by: Roel Kluin <roel.kluin@gmail.com> > Signed-off-by: "Juha Leppanen" <juha_motorsportcom@luukku.com> > --- > David, I swapped rbps and rbpl arguments in my last patch, and > there were some other problems. This was pointed out by Juha > Leppanen. Can you please replace the former patch by this one? > > This was version was build, sparse and checkpatch tested. > > Sorry for the mess. I can't just "replace" it, especially since your change is even already in Linus's tree. Please send me a relative fixup rather than a new patch. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] atm: dereference of he_dev->rbps_virt in he_init_group() 2009-09-27 3:26 ` David Miller @ 2009-09-28 8:58 ` Roel Kluin 2009-09-28 19:46 ` David Miller 0 siblings, 1 reply; 14+ messages in thread From: Roel Kluin @ 2009-09-28 8:58 UTC (permalink / raw) To: David Miller; +Cc: joe, chas, linux-atm-general, netdev, akpm From: Juha Leppanen <juha_motorsportcom@luukku.com> Date: Sat, Sep 26, 2009 at 12:34 AM Subject: atm: he: memleak/negative indexing of arrays in he_init_group() The prefix decrement causes a very long loop if pci_pool_alloc() failed in the first iteration. Also I swapped rbps and rbpl arguments. Reported-by: Juha Leppanen <juha_motorsportcom@luukku.com> Signed-off-by: Roel Kluin <roel.kluin@gmail.com> diff --git a/drivers/atm/he.c b/drivers/atm/he.c index 29e66d6..7066703 100644 --- a/drivers/atm/he.c +++ b/drivers/atm/he.c @@ -921,9 +921,9 @@ out_free_rbpq_base: he_dev->rbrq_phys); i = CONFIG_RBPL_SIZE; out_free_rbpl_virt: - while (--i) - pci_pool_free(he_dev->rbps_pool, he_dev->rbpl_virt[i].virt, - he_dev->rbps_base[i].phys); + while (i--) + pci_pool_free(he_dev->rbpl_pool, he_dev->rbpl_virt[i].virt, + he_dev->rbpl_base[i].phys); kfree(he_dev->rbpl_virt); out_free_rbpl_base: @@ -933,11 +933,11 @@ out_free_rbpl_base: out_destroy_rbpl_pool: pci_pool_destroy(he_dev->rbpl_pool); - i = CONFIG_RBPL_SIZE; + i = CONFIG_RBPS_SIZE; out_free_rbps_virt: - while (--i) - pci_pool_free(he_dev->rbpl_pool, he_dev->rbps_virt[i].virt, - he_dev->rbpl_base[i].phys); + while (i--) + pci_pool_free(he_dev->rbps_pool, he_dev->rbps_virt[i].virt, + he_dev->rbps_base[i].phys); kfree(he_dev->rbps_virt); out_free_rbps_base: ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] atm: dereference of he_dev->rbps_virt in he_init_group() 2009-09-28 8:58 ` Roel Kluin @ 2009-09-28 19:46 ` David Miller 0 siblings, 0 replies; 14+ messages in thread From: David Miller @ 2009-09-28 19:46 UTC (permalink / raw) To: roel.kluin; +Cc: joe, chas, linux-atm-general, netdev, akpm From: Roel Kluin <roel.kluin@gmail.com> Date: Mon, 28 Sep 2009 10:58:43 +0200 > From: Juha Leppanen <juha_motorsportcom@luukku.com> > Date: Sat, Sep 26, 2009 at 12:34 AM > Subject: atm: he: memleak/negative indexing of arrays in he_init_group() > > The prefix decrement causes a very long loop if pci_pool_alloc() failed > in the first iteration. Also I swapped rbps and rbpl arguments. > > Reported-by: Juha Leppanen <juha_motorsportcom@luukku.com> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com> Applied, thanks. Note that, by putting that From: line with Juha's info in it, I assume that you want this person to be listed as the author of the patch instead of you. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2009-09-28 19:45 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-08-29 18:56 [PATCH] atm: dereference of he_dev->rbps_virt in he_init_group() Roel Kluin 2009-08-29 18:59 ` Roel Kluin 2009-09-03 6:25 ` David Miller 2009-09-05 12:35 ` Roel Kluin 2009-09-11 19:37 ` David Miller 2009-09-11 19:51 ` David Miller 2009-09-19 18:16 ` Roel Kluin 2009-09-19 18:27 ` Joe Perches 2009-09-20 17:11 ` Roel Kluin 2009-09-22 21:25 ` David Miller 2009-09-26 13:20 ` Roel Kluin 2009-09-27 3:26 ` David Miller 2009-09-28 8:58 ` Roel Kluin 2009-09-28 19:46 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.