From: Evgeny Novikov <[email protected]>
[ 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().
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Evgeny Novikov <[email protected]>
Signed-off-by: Hans Verkuil <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
Signed-off-by: Sasha Levin <[email protected]>
---
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;
}
--
2.25.1
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 <[email protected]>
> Signed-off-by: Hans Verkuil <[email protected]>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> Signed-off-by: Sasha Levin <[email protected]>
> ---
> 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
On Tue, Sep 01, 2020 at 08:42:58PM +0200, Pavel Machek wrote:
> 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 <[email protected]>
> > Signed-off-by: Hans Verkuil <[email protected]>
> > Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> > Signed-off-by: Sasha Levin <[email protected]>
> > ---
> > 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.
Thanks, now dropped.
greg k-h
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" <[email protected]>:
> 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 <[email protected]>
>> Signed-off-by: Hans Verkuil <[email protected]>
>> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
>> Signed-off-by: Sasha Levin <[email protected]>
>> ---
>> 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