Re: [PATCH 4.19 047/125] media: davinci: vpif_capture: fix potential double free

[Evgeny Novikov]

Hi Pavel,

Maybe I miss something, but it seems that in 4.19 vpif_probe() ignores
error codes from vpif_probe_complete() and returns 0 even if it fails.
That was fixed by commit 64f883cd98c6, but it was not backported to 4.19.
In addition, this commit contains a fix of one more bug.

Regarding your second note. I investigated other drivers that use the same
mechanism and did not find out any driver that performs clean up in the
complete handler. In particular, this is the case for very similar driver
vpif_display.c. I am not an expert in this subsystem, so, I can not reason
why this is so.

-- 
Best regards,
Evgeny Novikov



01.09.2020, 21:43, "Pavel Machek" <pavel@xxxxxxx>:
> Hi!
>
>>  [ Upstream commit 602649eadaa0c977e362e641f51ec306bc1d365d ]
>>
>>  In case of errors vpif_probe_complete() releases memory for vpif_obj.sd
>>  and unregisters the V4L2 device. But then this is done again by
>>  vpif_probe() itself. The patch removes the cleaning from
>>  vpif_probe_complete().
>
>>  Signed-off-by: Evgeny Novikov <novikov@xxxxxxxxx>
>>  Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>>  Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
>>  Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
>>  ---
>>   drivers/media/platform/davinci/vpif_capture.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>>  diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
>>  index a96f53ce80886..cf1d11e6dd8c4 100644
>>  --- a/drivers/media/platform/davinci/vpif_capture.c
>>  +++ b/drivers/media/platform/davinci/vpif_capture.c
>>  @@ -1489,8 +1489,6 @@ probe_out:
>>                   /* Unregister video device */
>>                   video_unregister_device(&ch->video_dev);
>>           }
>>  - kfree(vpif_obj.sd);
>>  - v4l2_device_unregister(&vpif_obj.v4l2_dev);
>>
>>           return err;
>>   }
>
> This one is wrong. Unlike mainline, 4.19 does check return value of
> vpif_probe_complete(), and thus it will lead to memory leak in 4.19.
>
> Furthermore, I believe mainline still has a problems after this
> patch. There is sync and async path where vpif_probe_complete(), and
> while this fixes the sync path in mainline, I believe it will cause
> memory leak on the async path.
>
> Best regards,
>                                                                         Pavel
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html