[PATCH] media input infrastructure:tw686x: Fix of possibleinconsistent memory deallocation and/or race condition by implementation of custom video_device_release function in tw686x driver

[PATCH] media input infrastructure:tw686x: Fix of possibleinconsistent memory deallocation and/or race condition by implementation of custom video_device_release function in tw686x driver

[Mark Balantzyan]

Possible inconsistent memory deallocation and/or race conditions were 
detected specifically with respect to remaining open handles to the video 
device handled by the tw686x driver. This patch addresses this by 
implementing a revised independent instance of the video_device_release 
function to free the remaining resources and memory where the last open 
handle(s) is/were closed.

Signed-off-by: Mark Balantzyan <mbalant3@gmail.com>

---

  drivers/media/pci/tw686x/tw686x-video.c | 15 +++++++++++----
  1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/media/pci/tw686x/tw686x-video.c 
b/drivers/media/pci/tw686x/tw686x-video.c
index 3a06c000..29e10c85 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -1151,18 +1151,25 @@ void tw686x_video_irq(struct tw686x_dev *dev, 
unsigned long requests,
  	}
  }

+void video_device_release(tw686x_dev *dev) {
+	for (int pb = 0; pb < 2; pb++) {
+		dev->dma_ops->free(dev->video_channels,pb);
+	}
+	kfree(dev);
+}
+
  void tw686x_video_free(struct tw686x_dev *dev)
  {
-	unsigned int ch, pb;
+	unsigned int ch;

  	for (ch = 0; ch < max_channels(dev); ch++) {
  		struct tw686x_video_channel *vc = 
&dev->video_channels[ch];

  		video_unregister_device(vc->device);

-		if (dev->dma_ops->free)
-			for (pb = 0; pb < 2; pb++)
-				dev->dma_ops->free(vc, pb);
+		if (dev->dma_ops->free) {
+			video_device_release(dev);
+		}
  	}
  }

-- 
2.17.1

Re: [PATCH] media input infrastructure:tw686x: Fix of possibleinconsistent memory deallocation and/or race condition by implementation of custom video_device_release function in tw686x driver

[Hans Verkuil]

Hi Mark,

Please, please read this first before posting patches:

https://kernelnewbies.org/FirstKernelPatch

And don't use insanely long subject lines in your email.

This patch is nonsense. As said before, you need to override the release() callback
of struct video_device with a tw686x-specific function that frees the dma memory and
calls video_device_release() at the end to free the struct video_device itself.

This release() callback is called by the V4L2 framework when the last user of the
device closes its filehandle, so that's a good point to free all the memory. Doing
it earlier (as the current code does) runs the risk that someone might still access
that memory, and you don't want that.

Regards,

	Hans

On 7/29/19 10:09 PM, Mark Balantzyan wrote:
> 
> Possible inconsistent memory deallocation and/or race conditions were detected specifically with respect to remaining open handles to the video device handled by the tw686x driver. This patch
> addresses this by implementing a revised independent instance of the video_device_release function to free the remaining resources and memory where the last open handle(s) is/were closed.
> 
> Signed-off-by: Mark Balantzyan <mbalant3@gmail.com>
> 
> ---
> 
>  drivers/media/pci/tw686x/tw686x-video.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
> index 3a06c000..29e10c85 100644
> --- a/drivers/media/pci/tw686x/tw686x-video.c
> +++ b/drivers/media/pci/tw686x/tw686x-video.c
> @@ -1151,18 +1151,25 @@ void tw686x_video_irq(struct tw686x_dev *dev, unsigned long requests,
>      }
>  }
> 
> +void video_device_release(tw686x_dev *dev) {
> +    for (int pb = 0; pb < 2; pb++) {
> +        dev->dma_ops->free(dev->video_channels,pb);
> +    }
> +    kfree(dev);
> +}
> +
>  void tw686x_video_free(struct tw686x_dev *dev)
>  {
> -    unsigned int ch, pb;
> +    unsigned int ch;
> 
>      for (ch = 0; ch < max_channels(dev); ch++) {
>          struct tw686x_video_channel *vc = &dev->video_channels[ch];
> 
>          video_unregister_device(vc->device);
> 
> -        if (dev->dma_ops->free)
> -            for (pb = 0; pb < 2; pb++)
> -                dev->dma_ops->free(vc, pb);
> +        if (dev->dma_ops->free) {
> +            video_device_release(dev);
> +        }
>      }
>  }
> 

Re: [PATCH] media input infrastructure:tw686x: Fix of possibleinconsistent memory deallocation and/or race condition by implementation of custom video_device_release function in tw686x driver

[Mark Balantzyan]

Hi Hans, all,

Sorry for the poor patching, I am a student and as you may tell still new 
to this system. At the time of the patching, I wasn't fully informed of 
all the requirements that go into such things, and am still learning.

Would it be alright if I submit a report instead? In order to, I am 
(still, sorry) trying to understand the issue at hand. How in fact may the 
release() callback be overridden (by a tw686x-specific function) to free 
the dma memory and call video_device_release()? To my understanding at the 
time, this was merely a re-implementation of video_device_release with 
said requirements and subtraction of extra features from 
tw686x_video_free()..

 	This release() callback is called by the V4L2 framework when the last user
 	of the device closes its filehandle, so that's a good point to free all
 	the memory. Doing it earlier (as the current code does) runs the risk that someone might
 	still access that memory, and you don't want that.

Yes, I definitely don't want that. :)

Thank you,
Mark

Re: [PATCH] media input infrastructure:tw686x...

[Hans Verkuil]

Hi Mark,

On 7/31/19 6:32 PM, Mark Balantzyan wrote:
> Hi Hans, all,
> 
> Sorry for the poor patching, I am a student and as you may tell still 
> new to this system. At the time of the patching, I wasn't fully informed 
> of all the requirements that go into such things, and am still learning.
> 
> Would it be alright if I submit a report instead? In order to, I am 
> (still, sorry) trying to understand the issue at hand. How in fact may 
> the release() callback be overridden (by a tw686x-specific function) to 
> free the dma memory and call video_device_release()? To my understanding 
> at the time, this was merely a re-implementation of video_device_release 
> with said requirements and subtraction of extra features from 
> tw686x_video_free()..

Sorry, you'll need to discuss this with your mentor. I really don't have 
time to look at reports or anything like that. I'm a media subsystem 
maintainer, not your mentor. And I expect that you spend time trying to 
understand the code by looking at how other drivers do this and look at 
kernel documentation like this:

https://linuxtv.org/downloads/v4l-dvb-apis-new/media_kapi.html

Regards,

	Hans

> 
>      This release() callback is called by the V4L2 framework when the 
> last user
>      of the device closes its filehandle, so that's a good point to free 
> all
>      the memory. Doing it earlier (as the current code does) runs the 
> risk that someone might
>      still access that memory, and you don't want that.
> 
> Yes, I definitely don't want that. :)
> 
> Thank you,
> Mark
>