drivers/serial: Return -ENOMEM on memory allocation failure
in [Kernel]
|
Prev: RFC: iscsi ibft: separate ibft parsing from sysfs interface
Next: usb 2.0 em28xx kernel panic general protection fault: 0000 [#1] SMP RIP: 0010:[<ffffffffa004fbc5>] [<ffffffffa004fbc5>] em28xx_isoc_copy_vbi+0x62e/0x812 [em28xx]
From: Patrick Gefre on 11 Aug 2010 12:10 Julia Lawall wrote: > From: Julia Lawall <julia(a)diku.dk> > > In this code, 0 is returned on memory allocation failure, even though other > failures return -ENOMEM or other similar values. > > A simplified version of the semantic match that finds this problem is as > follows: (http://coccinelle.lip6.fr/) > > // <smpl> > @@ > expression ret; > expression x,e1,e2,e3; > @@ > > ret = 0 > ... when != ret = e1 > *x = \(kmalloc\|kcalloc\|kzalloc\)(...) > ... when != ret = e2 > if (x == NULL) { ... when != ret = e3 > return ret; > } > // </smpl> > > Signed-off-by: Julia Lawall <julia(a)diku.dk> > Signed-off-by: Pat Gefre <pfg(a)sgi.com> > --- > I believe this code also leaks earlier instances of port, which are only > referenced by card_ptr, which is freed in the error handling code at the > end of the function. A lot of operations are done on port on each > iteration, however, so I'm not sure whether it is good enough to just free > them. Perhaps there is some way to call ioc3uart_remove? > Yes you are right, there should be something like this for out4: out4: for (phys_port = 0; phys_port < PORTS_PER_CARD; phys_port++) { port = card_ptr->ic_port[phys_port].icp_port; if (port) { pci_free_consistent(port->ip_idd->pdev, TOTAL_RING_BUF_SIZE, (void *)port->ip_cpu_ringbuf, port->ip_dma_ringbuf); kfree(port); } } kfree(card_ptr); return ret; > drivers/serial/ioc3_serial.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/serial/ioc3_serial.c b/drivers/serial/ioc3_serial.c > index 93de907..800c546 100644 > --- a/drivers/serial/ioc3_serial.c > +++ b/drivers/serial/ioc3_serial.c > @@ -2044,6 +2044,7 @@ ioc3uart_probe(struct ioc3_submodule *is, struct ioc3_driver_data *idd) > if (!port) { > printk(KERN_WARNING > "IOC3 serial memory not available for port\n"); > + ret = -ENOMEM; > goto out4; > } > spin_lock_init(&port->ip_lock); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Julia Lawall on 11 Aug 2010 12:30
> > I believe this code also leaks earlier instances of port, which are only > > referenced by card_ptr, which is freed in the error handling code at the > > end of the function. A lot of operations are done on port on each > > iteration, however, so I'm not sure whether it is good enough to just free > > them. Perhaps there is some way to call ioc3uart_remove? > > > > Yes you are right, there should be something like this for out4: > > out4: > for (phys_port = 0; phys_port < PORTS_PER_CARD; phys_port++) { > port = card_ptr->ic_port[phys_port].icp_port; > if (port) { > pci_free_consistent(port->ip_idd->pdev, > TOTAL_RING_BUF_SIZE, > (void *)port->ip_cpu_ringbuf, > port->ip_dma_ringbuf); > kfree(port); > } > } > kfree(card_ptr); > return ret; Actually, pci_alloc_consistent is only called when phys_port is 0. In the subsequent cases, the ip_dma_ringbuf field is just initialized to the previous value. So it could be: out4: for (phys_port = 0; phys_port < PORTS_PER_CARD; phys_port++) { port = card_ptr->ic_port[phys_port].icp_port; if (port) { if (phys_port == 0) pci_free_consistent(port->ip_idd->pdev, TOTAL_RING_BUF_SIZE, (void *)port->ip_cpu_ringbuf, port->ip_dma_ringbuf); kfree(port); } } kfree(card_ptr); return ret; julia -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |