Hello Moudy Ho,
The patch 61890ccaefaf: "media: platform: mtk-mdp3: add MediaTek MDP3
driver" from Aug 23, 2022, leads to the following Smatch static
checker warning:
drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c:292 mdp_probe()
error: we previously assumed 'mdp' could be null (see line 188)
My blog entry gives a good overview of how to write Free the Last Thing
Style error handling. Let's take a look at it as it applies to
mdp_probe().
drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
180 static int mdp_probe(struct platform_device *pdev)
181 {
182 struct device *dev = &pdev->dev;
183 struct mdp_dev *mdp;
184 struct platform_device *mm_pdev;
185 int ret, i;
186
187 mdp = kzalloc(sizeof(*mdp), GFP_KERNEL);
188 if (!mdp) {
189 ret = -ENOMEM;
190 goto err_return;
There is no Last Thing to free. This should be "return -ENOMEM;".
This goto will crash. The Last thing is now "mdp".
191 }
192
193 mdp->pdev = pdev;
194 mdp->mdp_data = of_device_get_match_data(&pdev->dev);
195
196 mm_pdev = __get_pdev_by_id(pdev, MDP_INFRA_MMSYS);
197 if (!mm_pdev) {
198 ret = -ENODEV;
199 goto err_return;
This should be "goto err_free_mdp;". This goto will trigger a series
of WARN_ON() stack traces. The Last Thing is now mdp->mdp_mmsys.
200 }
201 mdp->mdp_mmsys = &mm_pdev->dev;
202
203 mm_pdev = __get_pdev_by_id(pdev, MDP_INFRA_MUTEX);
204 if (WARN_ON(!mm_pdev)) {
205 ret = -ENODEV;
206 goto err_return;
This goto should call put on mdp->mdp_mmsys dev_something. It instead
leaks that. But it does call mtk_mutex_put() and each call will leads
to a series of stack traces. The Last Thing is now mm_pdev. That
doesn't seem to be stored anywhere so it's going to be complicated to
free it... :/
207 }
208 for (i = 0; i < MDP_PIPE_MAX; i++) {
209 mdp->mdp_mutex[i] = mtk_mutex_get(&mm_pdev->dev);
210 if (!mdp->mdp_mutex[i]) {
211 ret = -ENODEV;
212 goto err_return;
If this fails it should unwind the successful allocations:
while (--i >= 0) {
But instead it does unwinds everything. Stack traces.
213 }
214 }
215
216 ret = mdp_comp_config(mdp);
217 if (ret) {
218 dev_err(dev, "Failed to config mdp components\n");
219 goto err_return;
Alright. Good. This will leak the "mm_pdev" references but it will not
print any stack traces. The mdp_comp_config() function uses One Magic
Free Function style error handling so it is buggy. The Last thing is
now comp_config.
220 }
221
222 mdp->job_wq = alloc_workqueue(MDP_MODULE_NAME, WQ_FREEZABLE, 0);
223 if (!mdp->job_wq) {
224 dev_err(dev, "Unable to create job workqueue\n");
225 ret = -ENOMEM;
226 goto err_deinit_comp;
Hooray! This label has a good name and frees the Last Thing. The new
last thing is the ->job_wq.
227 }
228
229 mdp->clock_wq = alloc_workqueue(MDP_MODULE_NAME "-clock", WQ_FREEZABLE,
230 0);
231 if (!mdp->clock_wq) {
232 dev_err(dev, "Unable to create clock workqueue\n");
233 ret = -ENOMEM;
234 goto err_destroy_job_wq;
Hooray!
235 }
236
237 mm_pdev = __get_pdev_by_id(pdev, MDP_INFRA_SCP);
238 if (WARN_ON(!mm_pdev)) {
239 dev_err(&pdev->dev, "Could not get scp device\n");
240 ret = -ENODEV;
241 goto err_destroy_clock_wq;
Hooray!
242 }
243 mdp->scp = platform_get_drvdata(mm_pdev);
244 mdp->rproc_handle = scp_get_rproc(mdp->scp);
245 dev_dbg(&pdev->dev, "MDP rproc_handle: %pK", mdp->rproc_handle);
246
247 mutex_init(&mdp->vpu_lock);
248 mutex_init(&mdp->m2m_lock);
249
250 mdp->cmdq_clt = cmdq_mbox_create(dev, 0);
251 if (IS_ERR(mdp->cmdq_clt)) {
252 ret = PTR_ERR(mdp->cmdq_clt);
253 goto err_put_scp;
Ugh... The mm_pdev name changed to scp. It took a while to figure that
out. This unwinds the __get_pdev_by_id(). Fair enough. The Last thing
is now cmdq_clt.
254 }
255
256 init_waitqueue_head(&mdp->callback_wq);
257 ida_init(&mdp->mdp_ida);
258 platform_set_drvdata(pdev, mdp);
259
260 vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
261
262 ret = v4l2_device_register(dev, &mdp->v4l2_dev);
263 if (ret) {
264 dev_err(dev, "Failed to register v4l2 device\n");
265 ret = -EINVAL;
266 goto err_mbox_destroy;
The cmdq_clt is an mbox. Fair enough. Good name, label does what we
expected.
267 }
268
269 ret = mdp_m2m_device_register(mdp);
270 if (ret) {
271 v4l2_err(&mdp->v4l2_dev, "Failed to register m2m device\n");
272 goto err_unregister_device;
Good.
273 }
274
275 dev_dbg(dev, "mdp-%d registered successfully\n", pdev->id);
276 return 0;
Summary:
BUG1: NULL dereference
BUG2: Stack traces calling mtk_mutex_put().
BUG3&4: Two __get_pdev_by_id() which need a matching put (reference leaks).
277
278 err_unregister_device:
279 v4l2_device_unregister(&mdp->v4l2_dev);
280 err_mbox_destroy:
281 cmdq_mbox_destroy(mdp->cmdq_clt);
282 err_put_scp:
283 scp_put(mdp->scp);
284 err_destroy_clock_wq:
285 destroy_workqueue(mdp->clock_wq);
286 err_destroy_job_wq:
287 destroy_workqueue(mdp->job_wq);
288 err_deinit_comp:
289 mdp_comp_destroy(mdp);
290 err_return:
291 for (i = 0; i < MDP_PIPE_MAX; i++)
--> 292 mtk_mutex_put(mdp->mdp_mutex[i]);
293 kfree(mdp);
294 dev_dbg(dev, "Errno %d\n", ret);
295 return ret;
296 }
regards,
dan carpenter
