* [bug report] drivers/net: support hdlc function for QE-UCC
@ 2016-07-12 23:49 Dan Carpenter
2016-07-13 2:22 ` Qiang Zhao
2016-08-03 14:13 ` Dan Carpenter
0 siblings, 2 replies; 3+ messages in thread
From: Dan Carpenter @ 2016-07-12 23:49 UTC (permalink / raw)
To: kernel-janitors
Hello Zhao Qiang,
The patch c19b6d246a35: "drivers/net: support hdlc function for
QE-UCC" from Jun 6, 2016, leads to the following static checker
warning:
drivers/net/wan/fsl_ucc_hdlc.c:1131 ucc_hdlc_probe()
error: dereferencing freed memory 'uhdlc_priv'
The error handling has quite a few problems. There is an easy way to
write error handling. Let me demonstrate.
drivers/net/wan/fsl_ucc_hdlc.c
1062 ret = of_address_to_resource(np, 0, &res);
1063 if (ret)
1064 return -EINVAL;
1065
1066 ut_info->uf_info.regs = res.start;
1067 ut_info->uf_info.irq = irq_of_parse_and_map(np, 0);
1068
1069 uhdlc_priv = kzalloc(sizeof(*uhdlc_priv), GFP_KERNEL);
1070 if (!uhdlc_priv) {
1071 ret = -ENOMEM;
1072 dev_err(&pdev->dev, "No mem to alloc hdlc private data\n");
No need for this printk(). kzalloc() already prints a warning.
1073 goto err_alloc_priv;
This is a do-nothing goto. Change it to "return -ENOMEM;".
uhdlc_priv = kzalloc(sizeof(*uhdlc_priv), GFP_KERNEL);
if (!uhdlc_priv)
return -ENOMEM;
1074 }
1075
1076 dev_set_drvdata(&pdev->dev, uhdlc_priv);
1077 uhdlc_priv->dev = &pdev->dev;
1078 uhdlc_priv->ut_info = ut_info;
1079
1080 if (of_get_property(np, "fsl,tdm-interface", NULL))
1081 uhdlc_priv->tsa = 1;
1082
1083 if (of_get_property(np, "fsl,ucc-internal-loopback", NULL))
1084 uhdlc_priv->loopback = 1;
1085
1086 if (uhdlc_priv->tsa = 1) {
1087 utdm = kzalloc(sizeof(*utdm), GFP_KERNEL);
1088 if (!utdm) {
1089 ret = -ENOMEM;
1090 dev_err(&pdev->dev, "No mem to alloc ucc tdm data\n");
No need again.
1091 goto err_alloc_utdm;
The goto name should say what the goto does. We need to free uhdlc_priv
so "goto free_uhdlc;" is a good name.
1092 }
1093 uhdlc_priv->utdm = utdm;
1094 ret = ucc_of_parse_tdm(np, utdm, ut_info);
1095 if (ret)
1096 goto err_miss_tsa_property;
Here we need to free utdm so "goto free_utdm;".
1097 }
1098
1099 ret = uhdlc_init(uhdlc_priv);
1100 if (ret) {
1101 dev_err(&pdev->dev, "Failed to init uhdlc\n");
1102 goto err_hdlc_init;
We're still freeing utdm; So goto free_utdm;
1103 }
1104
1105 dev = alloc_hdlcdev(uhdlc_priv);
1106 if (!dev) {
1107 ret = -ENOMEM;
1108 pr_err("ucc_hdlc: unable to allocate memory\n");
1109 goto err_hdlc_init;
Hm... Here we need to undo what uhdlc_init() did. I'm not sure the
function to do that. Currently we leak resources.
goto undo_uhdlc_init;
1110 }
1111
1112 uhdlc_priv->ndev = dev;
1113 hdlc = dev_to_hdlc(dev);
1114 dev->tx_queue_len = 16;
1115 dev->netdev_ops = &uhdlc_ops;
1116 hdlc->attach = ucc_hdlc_attach;
1117 hdlc->xmit = ucc_hdlc_tx;
1118 netif_napi_add(dev, &uhdlc_priv->napi, ucc_hdlc_poll, 32);
1119 if (register_hdlc_device(dev)) {
1120 ret = -ENOBUFS;
Why are we not preserving the error code?
ret = register_hdlc_device(dev);
if (ret) {
1121 pr_err("ucc_hdlc: unable to register hdlc device\n");
No need for this printk.
1122 free_netdev(dev);
1123 goto err_hdlc_init;
Instead of freeing and then a goto just do "goto free_dev;".
1124 }
1125
1126 return 0;
1127
1128 err_hdlc_init:
1129 err_miss_tsa_property:
1130 kfree(uhdlc_priv);
1131 if (uhdlc_priv->tsa)
1132 kfree(utdm);
1133 err_alloc_utdm:
1134 kfree(uhdlc_priv);
1135 err_alloc_priv:
1136 return ret;
1137 }
You can see the problems. We free uhdlc_priv then we dereference it,
then we free it a second time. There is stuff that is missing. The
labels are bad. Here is the new version:
free_dev:
free_netdev(dev);
undo_uhdlc_init:
/* FIXME: impliment this code */
free_utdm:
if (uhdlc_priv->tsa)
kfree(utdm);
free_uhdlc:
kfree(uhdlc);
return ret;
Just follow the steps and then error handling is easy to understand.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [bug report] drivers/net: support hdlc function for QE-UCC
2016-07-12 23:49 [bug report] drivers/net: support hdlc function for QE-UCC Dan Carpenter
@ 2016-07-13 2:22 ` Qiang Zhao
2016-08-03 14:13 ` Dan Carpenter
1 sibling, 0 replies; 3+ messages in thread
From: Qiang Zhao @ 2016-07-13 2:22 UTC (permalink / raw)
To: kernel-janitors
Hello Dan Carpenter,
Thank you for your comments, I will send a patch to fix it.
Best Regards
Zhao Qiang
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Wednesday, July 13, 2016 7:49 AM
> To: Qiang Zhao <qiang.zhao@nxp.com>
> Cc: kernel-janitors@vger.kernel.org
> Subject: [bug report] drivers/net: support hdlc function for QE-UCC
>
> Hello Zhao Qiang,
>
> The patch c19b6d246a35: "drivers/net: support hdlc function for QE-UCC" from
> Jun 6, 2016, leads to the following static checker
> warning:
>
> drivers/net/wan/fsl_ucc_hdlc.c:1131 ucc_hdlc_probe()
> error: dereferencing freed memory 'uhdlc_priv'
>
> The error handling has quite a few problems. There is an easy way to write
> error handling. Let me demonstrate.
>
> drivers/net/wan/fsl_ucc_hdlc.c
> 1062 ret = of_address_to_resource(np, 0, &res);
> 1063 if (ret)
> 1064 return -EINVAL;
> 1065
> 1066 ut_info->uf_info.regs = res.start;
> 1067 ut_info->uf_info.irq = irq_of_parse_and_map(np, 0);
> 1068
> 1069 uhdlc_priv = kzalloc(sizeof(*uhdlc_priv), GFP_KERNEL);
> 1070 if (!uhdlc_priv) {
> 1071 ret = -ENOMEM;
> 1072 dev_err(&pdev->dev, "No mem to alloc hdlc private data\n");
>
> No need for this printk(). kzalloc() already prints a warning.
>
> 1073 goto err_alloc_priv;
>
> This is a do-nothing goto. Change it to "return -ENOMEM;".
>
> uhdlc_priv = kzalloc(sizeof(*uhdlc_priv), GFP_KERNEL);
> if (!uhdlc_priv)
> return -ENOMEM;
>
>
> 1074 }
> 1075
> 1076 dev_set_drvdata(&pdev->dev, uhdlc_priv);
> 1077 uhdlc_priv->dev = &pdev->dev;
> 1078 uhdlc_priv->ut_info = ut_info;
> 1079
> 1080 if (of_get_property(np, "fsl,tdm-interface", NULL))
> 1081 uhdlc_priv->tsa = 1;
> 1082
> 1083 if (of_get_property(np, "fsl,ucc-internal-loopback", NULL))
> 1084 uhdlc_priv->loopback = 1;
> 1085
> 1086 if (uhdlc_priv->tsa = 1) {
> 1087 utdm = kzalloc(sizeof(*utdm), GFP_KERNEL);
> 1088 if (!utdm) {
> 1089 ret = -ENOMEM;
> 1090 dev_err(&pdev->dev, "No mem to alloc ucc tdm data\n");
>
> No need again.
>
> 1091 goto err_alloc_utdm;
>
> The goto name should say what the goto does. We need to free uhdlc_priv so
> "goto free_uhdlc;" is a good name.
>
> 1092 }
> 1093 uhdlc_priv->utdm = utdm;
> 1094 ret = ucc_of_parse_tdm(np, utdm, ut_info);
> 1095 if (ret)
> 1096 goto err_miss_tsa_property;
>
> Here we need to free utdm so "goto free_utdm;".
>
> 1097 }
> 1098
> 1099 ret = uhdlc_init(uhdlc_priv);
> 1100 if (ret) {
> 1101 dev_err(&pdev->dev, "Failed to init uhdlc\n");
> 1102 goto err_hdlc_init;
>
> We're still freeing utdm; So goto free_utdm;
>
> 1103 }
> 1104
> 1105 dev = alloc_hdlcdev(uhdlc_priv);
> 1106 if (!dev) {
> 1107 ret = -ENOMEM;
> 1108 pr_err("ucc_hdlc: unable to allocate memory\n");
> 1109 goto err_hdlc_init;
>
> Hm... Here we need to undo what uhdlc_init() did. I'm not sure the function to
> do that. Currently we leak resources.
>
> goto undo_uhdlc_init;
>
> 1110 }
> 1111
> 1112 uhdlc_priv->ndev = dev;
> 1113 hdlc = dev_to_hdlc(dev);
> 1114 dev->tx_queue_len = 16;
> 1115 dev->netdev_ops = &uhdlc_ops;
> 1116 hdlc->attach = ucc_hdlc_attach;
> 1117 hdlc->xmit = ucc_hdlc_tx;
> 1118 netif_napi_add(dev, &uhdlc_priv->napi, ucc_hdlc_poll, 32);
> 1119 if (register_hdlc_device(dev)) {
> 1120 ret = -ENOBUFS;
>
> Why are we not preserving the error code?
>
> ret = register_hdlc_device(dev);
> if (ret) {
>
> 1121 pr_err("ucc_hdlc: unable to register hdlc device\n");
>
> No need for this printk.
>
> 1122 free_netdev(dev);
> 1123 goto err_hdlc_init;
>
> Instead of freeing and then a goto just do "goto free_dev;".
>
> 1124 }
> 1125
> 1126 return 0;
> 1127
> 1128 err_hdlc_init:
> 1129 err_miss_tsa_property:
> 1130 kfree(uhdlc_priv);
> 1131 if (uhdlc_priv->tsa)
> 1132 kfree(utdm);
> 1133 err_alloc_utdm:
> 1134 kfree(uhdlc_priv);
> 1135 err_alloc_priv:
> 1136 return ret;
> 1137 }
>
> You can see the problems. We free uhdlc_priv then we dereference it, then
> we free it a second time. There is stuff that is missing. The labels are bad.
> Here is the new version:
>
> free_dev:
> free_netdev(dev);
> undo_uhdlc_init:
> /* FIXME: impliment this code */
> free_utdm:
> if (uhdlc_priv->tsa)
> kfree(utdm);
> free_uhdlc:
> kfree(uhdlc);
>
> return ret;
>
> Just follow the steps and then error handling is easy to understand.
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* [bug report] drivers/net: support hdlc function for QE-UCC
2016-07-12 23:49 [bug report] drivers/net: support hdlc function for QE-UCC Dan Carpenter
2016-07-13 2:22 ` Qiang Zhao
@ 2016-08-03 14:13 ` Dan Carpenter
1 sibling, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2016-08-03 14:13 UTC (permalink / raw)
To: kernel-janitors
Hello Zhao Qiang,
The patch c19b6d246a35: "drivers/net: support hdlc function for
QE-UCC" from Jun 6, 2016, leads to the following static checker
warning:
drivers/net/wan/fsl_ucc_hdlc.c:164 uhdlc_init()
warn: unsigned 'priv->ucc_pram_offset' is never less than zero.
drivers/net/wan/fsl_ucc_hdlc.c
159
160 /* Alloc parameter ram for ucc hdlc */
161 priv->ucc_pram_offset = qe_muram_alloc(sizeof(priv->ucc_pram),
162 ALIGNMENT_OF_UCC_HDLC_PRAM);
163
164 if (priv->ucc_pram_offset < 0) {
This is slightly complicated what's happening here. qe_muram_alloc()
returns negatives but casted to an unsigned long, then we assign it to
priv->ucc_pram_offset which is a u32.
165 dev_err(priv->dev, "Can not allocate MURAM for hdlc prameter.\n");
166 ret = -ENOMEM;
167 goto free_tx_bd;
168 }
169
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-08-03 14:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-07-12 23:49 [bug report] drivers/net: support hdlc function for QE-UCC Dan Carpenter
2016-07-13 2:22 ` Qiang Zhao
2016-08-03 14:13 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox