RE: [PATCH 2/4] net: freescale/fman-port: remove direct use of __devm_request_region

[Madalin Bucur]

> -----Original Message-----
> From: Patrick Havelange <patrick.havelange@xxxxxxxxxxxxx>
> Sent: 02 December 2020 18:16
> To: Madalin Bucur <madalin.bucur@xxxxxxx>; David S. Miller
> <davem@xxxxxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>;
> netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Cc: Patrick Havelange <patrick.havelange@xxxxxxxxxxxxx>
> Subject: [PATCH 2/4] net: freescale/fman-port: remove direct use of
> __devm_request_region
> 
> This driver was directly calling __devm_request_region with a specific
> resource on the stack as parameter. This is invalid as
> __devm_request_region expects the given resource passed as parameter
> to live longer than the call itself, as the pointer to the resource
> will be stored inside the internal struct used by the devres
> management.
> 
> In addition to this issue, a related bug has been found by kmemleak
> with this trace :
> unreferenced object 0xc0000001efc01880 (size 64):
>   comm "swapper/0", pid 1, jiffies 4294669078 (age 3620.536s)
>   hex dump (first 32 bytes):
>     00 00 00 0f fe 4a c0 00 00 00 00 0f fe 4a cf ff  .....J.......J..
>     c0 00 00 00 00 ee 9d 98 00 00 00 00 80 00 02 00  ................
>   backtrace:
>     [<c000000000078874>] .alloc_resource+0xb8/0xe0
>     [<c000000000079b50>] .__request_region+0x70/0x1c4
>     [<c00000000007a010>] .__devm_request_region+0x8c/0x138
>     [<c0000000006e0dc8>] .fman_port_probe+0x170/0x420
>     [<c0000000005cecb8>] .platform_drv_probe+0x84/0x108
>     [<c0000000005cc620>] .driver_probe_device+0x2c4/0x394
>     [<c0000000005cc814>] .__driver_attach+0x124/0x128
>     [<c0000000005c9ad4>] .bus_for_each_dev+0xb4/0x110
>     [<c0000000005cca1c>] .driver_attach+0x34/0x4c
>     [<c0000000005ca9b0>] .bus_add_driver+0x264/0x2a4
>     [<c0000000005cd9e0>] .driver_register+0x94/0x160
>     [<c0000000005cfea4>] .__platform_driver_register+0x60/0x7c
>     [<c000000000f86a00>] .fman_port_load+0x28/0x64
>     [<c000000000f4106c>] .do_one_initcall+0xd4/0x1a8
>     [<c000000000f412fc>] .kernel_init_freeable+0x1bc/0x2a4
>     [<c00000000000180c>] .kernel_init+0x24/0x138
> 
> Indeed, the new resource (created in __request_region) will be linked
> to the given resource living on the stack, which will end its lifetime
> after the function calling __devm_request_region has finished.
> Meaning the new resource allocated is no longer reachable.
> 
> Now that the main fman driver is no longer reserving the region
> used by fman-port, this previous hack is no longer needed
> and we can use the regular call to devm_request_mem_region instead,
> solving those bugs at the same time.
> 
> Signed-off-by: Patrick Havelange <patrick.havelange@xxxxxxxxxxxxx>
> ---
>  drivers/net/ethernet/freescale/fman/fman_port.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fman/fman_port.c
> b/drivers/net/ethernet/freescale/fman/fman_port.c
> index d9baac0dbc7d..354974939d9d 100644
> --- a/drivers/net/ethernet/freescale/fman/fman_port.c
> +++ b/drivers/net/ethernet/freescale/fman/fman_port.c
> @@ -1878,10 +1878,10 @@ static int fman_port_probe(struct platform_device
> *of_dev)
> 
>  	of_node_put(port_node);
> 
> -	dev_res = __devm_request_region(port->dev, &res, res.start,
> -					resource_size(&res), "fman-port");
> +	dev_res = devm_request_mem_region(port->dev, res.start,
> +					  resource_size(&res), "fman-port");
>  	if (!dev_res) {
> -		dev_err(port->dev, "%s: __devm_request_region() failed\n",
> +		dev_err(port->dev, "%s: devm_request_mem_region() failed\n",
>  			__func__);
>  		err = -EINVAL;
>  		goto free_port;
> --
> 2.17.1

Hi Patrick,

please send a fix for the issue, targeting the net tree, separate from the
other change you are trying to introduce. We need a better explanation for
the latter and it should go through the net-next tree, if accepted.

Madalin