if of_find_device_by_node() succeed, fman_port_probe() doesn't have a
corresponding put_device(). Thus add jump target to fix the exception
handling for this function implementation.
Fixes: 0572054617f3 ("fsl/fman: fix dereference null return value")
Signed-off-by: Yu Kuai <[email protected]>
---
drivers/net/ethernet/freescale/fman/fman_port.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fman/fman_port.c b/drivers/net/ethernet/freescale/fman/fman_port.c
index d9baac0dbc7d..576ce6df3fce 100644
--- a/drivers/net/ethernet/freescale/fman/fman_port.c
+++ b/drivers/net/ethernet/freescale/fman/fman_port.c
@@ -1799,13 +1799,13 @@ static int fman_port_probe(struct platform_device *of_dev)
of_node_put(fm_node);
if (!fm_pdev) {
err = -EINVAL;
- goto return_err;
+ goto put_device;
}
fman = dev_get_drvdata(&fm_pdev->dev);
if (!fman) {
err = -EINVAL;
- goto return_err;
+ goto put_device;
}
err = of_property_read_u32(port_node, "cell-index", &val);
@@ -1813,7 +1813,7 @@ static int fman_port_probe(struct platform_device *of_dev)
dev_err(port->dev, "%s: reading cell-index for %pOF failed\n",
__func__, port_node);
err = -EINVAL;
- goto return_err;
+ goto put_device;
}
port_id = (u8)val;
port->dts_params.id = port_id;
@@ -1847,7 +1847,7 @@ static int fman_port_probe(struct platform_device *of_dev)
} else {
dev_err(port->dev, "%s: Illegal port type\n", __func__);
err = -EINVAL;
- goto return_err;
+ goto put_device;
}
port->dts_params.type = port_type;
@@ -1861,7 +1861,7 @@ static int fman_port_probe(struct platform_device *of_dev)
dev_err(port->dev, "%s: incorrect qman-channel-id\n",
__func__);
err = -EINVAL;
- goto return_err;
+ goto put_device;
}
port->dts_params.qman_channel_id = qman_channel_id;
}
@@ -1871,7 +1871,7 @@ static int fman_port_probe(struct platform_device *of_dev)
dev_err(port->dev, "%s: of_address_to_resource() failed\n",
__func__);
err = -ENOMEM;
- goto return_err;
+ goto put_device;
}
port->dts_params.fman = fman;
@@ -1898,6 +1898,8 @@ static int fman_port_probe(struct platform_device *of_dev)
return_err:
of_node_put(port_node);
+put_device:
+ put_device(&fm_pdev->dev);
free_port:
kfree(port);
return err;
--
2.25.4
On Sat, 31 Oct 2020 18:54:18 +0800 Yu Kuai wrote:
> if of_find_device_by_node() succeed, fman_port_probe() doesn't have a
> corresponding put_device(). Thus add jump target to fix the exception
> handling for this function implementation.
>
> Fixes: 0572054617f3 ("fsl/fman: fix dereference null return value")
> Signed-off-by: Yu Kuai <[email protected]>
> diff --git a/drivers/net/ethernet/freescale/fman/fman_port.c b/drivers/net/ethernet/freescale/fman/fman_port.c
> index d9baac0dbc7d..576ce6df3fce 100644
> --- a/drivers/net/ethernet/freescale/fman/fman_port.c
> +++ b/drivers/net/ethernet/freescale/fman/fman_port.c
> @@ -1799,13 +1799,13 @@ static int fman_port_probe(struct platform_device *of_dev)
> of_node_put(fm_node);
> if (!fm_pdev) {
> err = -EINVAL;
> - goto return_err;
> + goto put_device;
> }
> @@ -1898,6 +1898,8 @@ static int fman_port_probe(struct platform_device *of_dev)
>
> return_err:
> of_node_put(port_node);
> +put_device:
> + put_device(&fm_pdev->dev);
> free_port:
> kfree(port);
> return err;
This does not look right. You're jumping to put_device() when fm_pdev
is NULL?
The order of error handling should be the reverse of the order of
execution of the function.
On 2020/11/03 9:30, Jakub Kicinski wrote:
> On Sat, 31 Oct 2020 18:54:18 +0800 Yu Kuai wrote:
>> if of_find_device_by_node() succeed, fman_port_probe() doesn't have a
>> corresponding put_device(). Thus add jump target to fix the exception
>> handling for this function implementation.
>>
>> Fixes: 0572054617f3 ("fsl/fman: fix dereference null return value")
>> Signed-off-by: Yu Kuai <[email protected]>
>
>> diff --git a/drivers/net/ethernet/freescale/fman/fman_port.c b/drivers/net/ethernet/freescale/fman/fman_port.c
>> index d9baac0dbc7d..576ce6df3fce 100644
>> --- a/drivers/net/ethernet/freescale/fman/fman_port.c
>> +++ b/drivers/net/ethernet/freescale/fman/fman_port.c
>> @@ -1799,13 +1799,13 @@ static int fman_port_probe(struct platform_device *of_dev)
>> of_node_put(fm_node);
>> if (!fm_pdev) {
>> err = -EINVAL;
>> - goto return_err;
>> + goto put_device;
>> }
>
>> @@ -1898,6 +1898,8 @@ static int fman_port_probe(struct platform_device *of_dev)
>>
>> return_err:
>> of_node_put(port_node);
>> +put_device:
>> + put_device(&fm_pdev->dev);
>> free_port:
>> kfree(port);
>> return err;
>
> This does not look right. You're jumping to put_device() when fm_pdev
> is NULL?
>
Hi,
oops, it's a silly mistake. Will fix it in V2 patch.
Thanks,
Yu Kuai
> The order of error handling should be the reverse of the order of
> execution of the function.
> .
>
if of_find_device_by_node() succeed, fman_port_probe() doesn't have a
corresponding put_device(). Thus add jump target to fix the exception
handling for this function implementation.
Fixes: 0572054617f3 ("fsl/fman: fix dereference null return value")
Signed-off-by: Yu Kuai <[email protected]>
---
.../net/ethernet/freescale/fman/fman_port.c | 23 +++++++++----------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fman/fman_port.c b/drivers/net/ethernet/freescale/fman/fman_port.c
index d9baac0dbc7d..4ae5d844d1f5 100644
--- a/drivers/net/ethernet/freescale/fman/fman_port.c
+++ b/drivers/net/ethernet/freescale/fman/fman_port.c
@@ -1792,20 +1792,21 @@ static int fman_port_probe(struct platform_device *of_dev)
if (!fm_node) {
dev_err(port->dev, "%s: of_get_parent() failed\n", __func__);
err = -ENODEV;
- goto return_err;
+ goto free_port;
}
+ of_node_put(port_node);
fm_pdev = of_find_device_by_node(fm_node);
of_node_put(fm_node);
if (!fm_pdev) {
err = -EINVAL;
- goto return_err;
+ goto free_port;
}
fman = dev_get_drvdata(&fm_pdev->dev);
if (!fman) {
err = -EINVAL;
- goto return_err;
+ goto put_device;
}
err = of_property_read_u32(port_node, "cell-index", &val);
@@ -1813,7 +1814,7 @@ static int fman_port_probe(struct platform_device *of_dev)
dev_err(port->dev, "%s: reading cell-index for %pOF failed\n",
__func__, port_node);
err = -EINVAL;
- goto return_err;
+ goto put_device;
}
port_id = (u8)val;
port->dts_params.id = port_id;
@@ -1847,7 +1848,7 @@ static int fman_port_probe(struct platform_device *of_dev)
} else {
dev_err(port->dev, "%s: Illegal port type\n", __func__);
err = -EINVAL;
- goto return_err;
+ goto put_device;
}
port->dts_params.type = port_type;
@@ -1861,7 +1862,7 @@ static int fman_port_probe(struct platform_device *of_dev)
dev_err(port->dev, "%s: incorrect qman-channel-id\n",
__func__);
err = -EINVAL;
- goto return_err;
+ goto put_device;
}
port->dts_params.qman_channel_id = qman_channel_id;
}
@@ -1871,20 +1872,18 @@ static int fman_port_probe(struct platform_device *of_dev)
dev_err(port->dev, "%s: of_address_to_resource() failed\n",
__func__);
err = -ENOMEM;
- goto return_err;
+ goto put_device;
}
port->dts_params.fman = fman;
- of_node_put(port_node);
-
dev_res = __devm_request_region(port->dev, &res, res.start,
resource_size(&res), "fman-port");
if (!dev_res) {
dev_err(port->dev, "%s: __devm_request_region() failed\n",
__func__);
err = -EINVAL;
- goto free_port;
+ goto put_device;
}
port->dts_params.base_addr = devm_ioremap(port->dev, res.start,
@@ -1896,8 +1895,8 @@ static int fman_port_probe(struct platform_device *of_dev)
return 0;
-return_err:
- of_node_put(port_node);
+put_device:
+ put_device(&fm_pdev->dev);
free_port:
kfree(port);
return err;
--
2.25.4
On Tue, 3 Nov 2020 19:23:23 +0800 Yu Kuai wrote:
> --- a/drivers/net/ethernet/freescale/fman/fman_port.c
> +++ b/drivers/net/ethernet/freescale/fman/fman_port.c
> @@ -1792,20 +1792,21 @@ static int fman_port_probe(struct platform_device *of_dev)
> if (!fm_node) {
> dev_err(port->dev, "%s: of_get_parent() failed\n", __func__);
> err = -ENODEV;
> - goto return_err;
> + goto free_port;
> }
>
> + of_node_put(port_node);
> fm_pdev = of_find_device_by_node(fm_node);
> of_node_put(fm_node);
> if (!fm_pdev) {
> err = -EINVAL;
> - goto return_err;
> + goto free_port;
> }
This is not right either. I just asked you fix up the order of the
error path, not move the of_node_put() in the body of the function.
Now you're releasing the reference on the object and still use it after.
> -----Original Message-----
> From: Jakub Kicinski <[email protected]>
> Sent: 05 November 2020 03:31
> To: Yu Kuai <[email protected]>
> Cc: Madalin Bucur <[email protected]>; [email protected]; Florinel
> Iordache <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH V2] fsl/fman: add missing put_devcie() call in
> fman_port_probe()
>
> On Tue, 3 Nov 2020 19:23:23 +0800 Yu Kuai wrote:
> > --- a/drivers/net/ethernet/freescale/fman/fman_port.c
> > +++ b/drivers/net/ethernet/freescale/fman/fman_port.c
> > @@ -1792,20 +1792,21 @@ static int fman_port_probe(struct
> platform_device *of_dev)
> > if (!fm_node) {
> > dev_err(port->dev, "%s: of_get_parent() failed\n", __func__);
> > err = -ENODEV;
> > - goto return_err;
> > + goto free_port;
> > }
> >
> > + of_node_put(port_node);
> > fm_pdev = of_find_device_by_node(fm_node);
> > of_node_put(fm_node);
> > if (!fm_pdev) {
> > err = -EINVAL;
> > - goto return_err;
> > + goto free_port;
> > }
>
> This is not right either. I just asked you fix up the order of the
> error path, not move the of_node_put() in the body of the function.
>
> Now you're releasing the reference on the object and still use it after.
If you manage to put together a v3, please also address the typo in the
subject (put_devcie).
Madalin
if of_find_device_by_node() succeed, fman_port_probe() doesn't have a
corresponding put_device(). Thus add jump target to fix the exception
handling for this function implementation.
Fixes: 0572054617f3 ("fsl/fman: fix dereference null return value")
Signed-off-by: Yu Kuai <[email protected]>
---
Changes in V3:
- move of_node_put(port_node) backward, so that error handling can be the
reverse of the order of execution.
- rename retur_err to put_node
.../net/ethernet/freescale/fman/fman_port.c | 23 ++++++++++---------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fman/fman_port.c b/drivers/net/ethernet/freescale/fman/fman_port.c
index d9baac0dbc7d..9790e483241b 100644
--- a/drivers/net/ethernet/freescale/fman/fman_port.c
+++ b/drivers/net/ethernet/freescale/fman/fman_port.c
@@ -1792,20 +1792,20 @@ static int fman_port_probe(struct platform_device *of_dev)
if (!fm_node) {
dev_err(port->dev, "%s: of_get_parent() failed\n", __func__);
err = -ENODEV;
- goto return_err;
+ goto free_port;
}
fm_pdev = of_find_device_by_node(fm_node);
of_node_put(fm_node);
if (!fm_pdev) {
err = -EINVAL;
- goto return_err;
+ goto put_node;
}
fman = dev_get_drvdata(&fm_pdev->dev);
if (!fman) {
err = -EINVAL;
- goto return_err;
+ goto put_device;
}
err = of_property_read_u32(port_node, "cell-index", &val);
@@ -1813,7 +1813,7 @@ static int fman_port_probe(struct platform_device *of_dev)
dev_err(port->dev, "%s: reading cell-index for %pOF failed\n",
__func__, port_node);
err = -EINVAL;
- goto return_err;
+ goto put_device;
}
port_id = (u8)val;
port->dts_params.id = port_id;
@@ -1847,7 +1847,7 @@ static int fman_port_probe(struct platform_device *of_dev)
} else {
dev_err(port->dev, "%s: Illegal port type\n", __func__);
err = -EINVAL;
- goto return_err;
+ goto put_device;
}
port->dts_params.type = port_type;
@@ -1861,7 +1861,7 @@ static int fman_port_probe(struct platform_device *of_dev)
dev_err(port->dev, "%s: incorrect qman-channel-id\n",
__func__);
err = -EINVAL;
- goto return_err;
+ goto put_device;
}
port->dts_params.qman_channel_id = qman_channel_id;
}
@@ -1871,22 +1871,21 @@ static int fman_port_probe(struct platform_device *of_dev)
dev_err(port->dev, "%s: of_address_to_resource() failed\n",
__func__);
err = -ENOMEM;
- goto return_err;
+ goto put_device;
}
port->dts_params.fman = fman;
- of_node_put(port_node);
-
dev_res = __devm_request_region(port->dev, &res, res.start,
resource_size(&res), "fman-port");
if (!dev_res) {
dev_err(port->dev, "%s: __devm_request_region() failed\n",
__func__);
err = -EINVAL;
- goto free_port;
+ goto put_device;
}
+ of_node_put(port_node);
port->dts_params.base_addr = devm_ioremap(port->dev, res.start,
resource_size(&res));
if (!port->dts_params.base_addr)
@@ -1896,7 +1895,9 @@ static int fman_port_probe(struct platform_device *of_dev)
return 0;
-return_err:
+put_device:
+ put_device(&fm_pdev->dev);
+put_node:
of_node_put(port_node);
free_port:
kfree(port);
--
2.25.4
On Sat, 7 Nov 2020 17:09:25 +0800 Yu Kuai wrote:
> if of_find_device_by_node() succeed, fman_port_probe() doesn't have a
> corresponding put_device(). Thus add jump target to fix the exception
> handling for this function implementation.
>
> Fixes: 0572054617f3 ("fsl/fman: fix dereference null return value")
> Signed-off-by: Yu Kuai <[email protected]>
> @@ -1792,20 +1792,20 @@ static int fman_port_probe(struct platform_device *of_dev)
> if (!fm_node) {
> dev_err(port->dev, "%s: of_get_parent() failed\n", __func__);
> err = -ENODEV;
> - goto return_err;
> + goto free_port;
And now you no longer put port_node if jumping from here...
Also does the reference to put_device() not have to be released when
this function succeeds?
> }
> @@ -1896,7 +1895,9 @@ static int fman_port_probe(struct platform_device *of_dev)
>
> return 0;
>
> -return_err:
> +put_device:
> + put_device(&fm_pdev->dev);
> +put_node:
> of_node_put(port_node);
> free_port:
> kfree(port);
?? 2020/11/08 6:09, Jakub Kicinski ะด??:
> On Sat, 7 Nov 2020 17:09:25 +0800 Yu Kuai wrote:
>> if of_find_device_by_node() succeed, fman_port_probe() doesn't have a
>> corresponding put_device(). Thus add jump target to fix the exception
>> handling for this function implementation.
>>
>> Fixes: 0572054617f3 ("fsl/fman: fix dereference null return value")
>> Signed-off-by: Yu Kuai <[email protected]>
>
>> @@ -1792,20 +1792,20 @@ static int fman_port_probe(struct platform_device *of_dev)
>> if (!fm_node) {
>> dev_err(port->dev, "%s: of_get_parent() failed\n", __func__);
>> err = -ENODEV;
>> - goto return_err;
>> + goto free_port;
>
> And now you no longer put port_node if jumping from here...
Sincerely apologize for that stupid mistake...
>
> Also does the reference to put_device() not have to be released when
> this function succeeds?
>
I'm not sure about that, since fman_port_driver doesn't define other
interface, maybe it reasonable to release it here.
>> }
>
>> @@ -1896,7 +1895,9 @@ static int fman_port_probe(struct platform_device *of_dev)
>>
>> return 0;
>>
>> -return_err:
>> +put_device:
>> + put_device(&fm_pdev->dev);
>> +put_node:
>> of_node_put(port_node);
>> free_port:
>> kfree(port);
>
> .
>