| Message ID | 20201202161600.23738-2-patrick.havelange@essensium.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/4] net: freescale/fman: Split the main resource region reservation | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | warning | Series does not have a cover letter |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 13 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
> -----Original Message----- > From: Patrick Havelange <patrick.havelange@essensium.com> > Sent: 02 December 2020 18:16 > To: Madalin Bucur <madalin.bucur@nxp.com>; David S. Miller > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org > Cc: Patrick Havelange <patrick.havelange@essensium.com> > 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@essensium.com> > --- > 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
On 2020-12-03 09:44, Madalin Bucur wrote: >> -----Original Message----- >> From: Patrick Havelange <patrick.havelange@essensium.com> >> Sent: 02 December 2020 18:16 >> To: Madalin Bucur <madalin.bucur@nxp.com>; David S. Miller >> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; >> netdev@vger.kernel.org; linux-kernel@vger.kernel.org >> Cc: Patrick Havelange <patrick.havelange@essensium.com> >> 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@essensium.com> >> --- >> 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. Hello, I've resent the series with a cover letter having a bit more explanations. I think all the patches should be applied on net, as they are linked to the same issue/resolution, and are not independent. BR, Patrick H.
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;
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@essensium.com> --- drivers/net/ethernet/freescale/fman/fman_port.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)