Re: [PATCH] net: allwinner: Fix some resources leak in the error handling path of the probe and in the remove function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Le 15/12/2020 à 20:35, Dan Carpenter a écrit :
On Tue, Dec 15, 2020 at 08:08:15PM +0100, Maxime Ripard wrote:
On Tue, Dec 15, 2020 at 07:18:48PM +0100, Christophe JAILLET wrote:
Le 15/12/2020 à 12:37, Maxime Ripard a écrit :
On Tue, Dec 15, 2020 at 12:11:53PM +0300, Dan Carpenter wrote:
On Tue, Dec 15, 2020 at 09:56:55AM +0100, Maxime Ripard wrote:
Hi,

On Mon, Dec 14, 2020 at 09:21:17PM +0100, Christophe JAILLET wrote:
'irq_of_parse_and_map()' should be balanced by a corresponding
'irq_dispose_mapping()' call. Otherwise, there is some resources leaks.

Do you have a source to back that? It's not clear at all from the
documentation for those functions, and couldn't find any user calling it
from the ten-or-so random picks I took.

It looks like irq_create_of_mapping() needs to be freed with
irq_dispose_mapping() so this is correct.

The doc should be updated first to make that clear then, otherwise we're
going to fix one user while multiples will have poped up

Maxime


Hi,

as Dan explained, I think that 'irq_dispose_mapping()' is needed because of
the 'irq_create_of_mapping()" within 'irq_of_parse_and_map()'.

As you suggest, I'll propose a doc update to make it clear and more future
proof.

Thanks :)

And if you feel like it, a coccinelle script would be awesome too so
that other users get fixed over time

Maxime

Smatch has a new check for resource leaks which hopefully people will
find useful.

https://github.com/error27/smatch/blob/master/check_unwind.c

Nice :)
I wasn't aware of it.


To check for these I would need to add the following lines to the table:

         { "irq_of_parse_and_map", ALLOC, -1, "$", &int_one, &int_max},
         { "irq_create_of_mapping", ALLOC, -1, "$", &int_one, &int_max},
         { "irq_dispose_mapping", RELEASE, 0, "$"},

The '-1, "$"' means the returned value.  irq_of_parse_and_map() and
irq_create_of_mapping() return positive int on success.

The irq_dispose_mapping() frees its zeroth parameter so it's listed as
'0, "$"'.  We don't care about the returns from irq_dispose_mapping().

It doesn't apply in this case but if a function frees a struct member
then that's listed as '0, "$->member_name"'.

regards,
dan carpenter


The script I use to try to spot missing release function is:
///
@@
expression  x, y;
identifier f, l;
@@

*       x = irq_of_parse_and_map(...);
        ... when any
*       y = f(...);
        ... when any
*       if (<+... y ...+>)
        {
                ...
(
*               goto l;
|
*               return ...;
)
                ...
        }
        ... when any
*l:
        ... when != irq_dispose_mapping(...);
*       return ...;
///

It is likely that some improvement can be made, but it works pretty well for what I want.

And I have a collection of alloc/free functions that I manually put in place of irq_of_parse_and_map and irq_dispose_mapping.

Up to know, this list is:

// alloc_etherdev/alloc_etherdev_mq/alloc_etherdev_mqs - free_netdev
// alloc_workqueue - destroy_workqueue
// class_register - class_unregister
// clk_get - clk_put
// clk_prepare - clk_unprepare
// create_workqueue - destroy_workqueue
// create_singlethread_workqueue - destroy_workqueue
// dev_pm_domain_attach/dev_pm_domain_attach_by_id/dev_pm_domain_attach_by_name - dev_pm_domain_detach
// devres_alloc - devres_free
// dma_alloc_coherent - dma_free_coherent
// dma_map_resource - dma_unmap_resource
// dma_map_single - dma_unmap_single
// dma_request_slave_channel - dma_release_channel
// dma_request_chan - dma_release_channel
// framebuffer_alloc - framebuffer_release
// get_device - put_device
// iio_channel_get - iio_channel_release
// ioremap - iounmap
// input_allocate_device - input_free_device
// input_register_handle - input_unregister_handle
// irq_of_parse_and_map / irq_create_of_mapping - irq_dispose_mapping
// kmalloc - kfree
// mempool_alloc - mempool_free
// of_node_get - of_node_put
// of_reserved_mem_device_init - of_reserved_mem_device_release
// pinctrl_register - pinctrl_unregister
// request_irq - free_irq
// request_region - release_region
// request_mem_region - release_mem_region
// reset_control_assert - reset_control_deassert
// scsi_host_alloc - scsi_host_put

// pci_alloc_irq_vectors - pci_free_irq_vectors
// pci_dev_get - pci_dev_put
// pci_enable_device - pci_disable_device
// pci_iomap - pci_iounmap
// pci_request_region - pci_release_region
// pci_request_regions - pci_release_regions

// alloc_skb/__alloc_skb - kfree_skb/__kfree_skb
// dev_alloc_skb - dev_kfree_skb

// spi_dev_get - spi_dev_put
// spi_message_alloc - spi_message_free
// spi_register_master - spi_unregister_master



[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux