Re: tw686x driver (continued)

Re: tw686x driver (continued)

[Ezequiel Garcia]

On Tue, 23 Jul 2019 at 12:55, Mark Balançian <mbalant3@gmail.com> wrote:
>
> On Jul 23, 2019, at 8:17 AM, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>
>
> On Tue, 23 Jul 2019 at 12:02, Mark Balançian <mbalant3@gmail.com> wrote:
>
>
> I see. I guess then my issue would be help in seeing how tw686x_memcpy_dma_free alone does any required interrupt handling, since there are also functions tw686x_irq and tw686x_audio_irq for interrupt handling as well? However, in my analysis, they were called after tw686x_memcpy_dma_free.
>
>
> What are you trying to do? and what is exactly not working?
>
> PS: Don't drop linux-media from the Cc list, and please don't top-post.
>
> Thanks,
> Eze
>
>
> I don’t know what top-posting is, but I take it that it means I should write my email below the previous? Anyways, I’m working with a linux driver verification team to detect race conditions in the kernel. Using our tool, we detected a possible race condition with the tw686x driver.

Can you describe how is this race condition possible ? E.g. what are
the possible code paths and what would be the problem?

Also, is the tool open source?

> Even if there’s the slightest thing I’d like to please patch it as I really need this for my enrolled program.
>

Sure, if there's anything to patch, let's patch it!

> In any case, if interrupt handing isn’t given dedicated functions that are called before tw686x_memcpy_dma_free, I wouldn’t mind writing them and including them in a patch.
>

I can't understand what you mean. Can you describe what is the issue
you are seeing in the driver?

Thanks,
Ezequiel

Re: tw686x driver (continued)

[Mark Balançian]

Hi Ezequiel,

(sorry didn't include linux-media in first email)
I'm not sure yet if I have my supervisor's permission to declare our 
tool as open source, but I'll tell you the possible code paths that I 
think may be leading our tool to think what it's thinking.

First off, it detects a write access to desc->virt without locks inside 
tw686x_memcpy_data_free, after it is called in the calling chain 
tw686x_probe -> allocate an interrupt line -> tw686x_video_init -> 
tw686x_set_format -> tw686x_memcpy_dma_free. Further, 
spin_lock_init(&dev->lock) (line 319 of tw686x-core.c) isn't 
correspondingly closed in the function. Is this intended?

Second, there is a possibility according to how I have traced a call 
chain that tw686x_init is going to the error: label since 
tw686x_memcpy_dma_free is getting called inside another possible calling 
chain, going tw686x_init -> tw686x_video_free (error: label) -> 
dma_ops->free (i.e. tw686x_memcpy_dma_free). I would assume this would 
not be intended either.

In addition, our tool detects a read access without locks to desc->virt 
inside tw686x_audio_irq (line 72 of tw686x-audio.c). Not sure what you 
make of that, but I'd be keen on hearing about that as well from you.

Thank you in advance,

Mark

Re: tw686x driver (continued)

[Mark Balançian]

TBH though, I’m not sure about the tw686x_init  going to the error: label though, so am curious as to your thoughts on this as well. It was just something I thought might be occurring due to the fact that the error trace in our tool ends in tw686x_memcpy_dma_free (i.e. dma_ops->free).

Please see the other indications I made toward the codes of tw686x-core.c and tw686x-audio.c in the message below.

Thank you,
Mark

> On Jul 24, 2019, at 5:25 AM, Mark Balançian <mbalant3@gmail.com> wrote:
> 
> Hi Ezequiel,
> 
> (sorry didn't include linux-media in first email)
> I'm not sure yet if I have my supervisor's permission to declare our tool as open source, but I'll tell you the possible code paths that I think may be leading our tool to think what it's thinking.
> 
> First off, it detects a write access to desc->virt without locks inside tw686x_memcpy_data_free, after it is called in the calling chain tw686x_probe -> allocate an interrupt line -> tw686x_video_init -> tw686x_set_format -> tw686x_memcpy_dma_free. Further, spin_lock_init(&dev->lock) (line 319 of tw686x-core.c) isn't correspondingly closed in the function. Is this intended?
> 
> Second, there is a possibility according to how I have traced a call chain that tw686x_init is going to the error: label since tw686x_memcpy_dma_free is getting called inside another possible calling chain, going tw686x_init -> tw686x_video_free (error: label) -> dma_ops->free (i.e. tw686x_memcpy_dma_free). I would assume this would not be intended either.
> 
> In addition, our tool detects a read access without locks to desc->virt inside tw686x_audio_irq (line 72 of tw686x-audio.c). Not sure what you make of that, but I'd be keen on hearing about that as well from you.
> 
> Thank you in advance,
> 
> Mark

Re: tw686x driver (continued)

[Hans Verkuil]

On 7/24/19 2:25 PM, Mark Balançian wrote:
> Hi Ezequiel,
> 
> (sorry didn't include linux-media in first email)
> I'm not sure yet if I have my supervisor's permission to declare our 
> tool as open source, but I'll tell you the possible code paths that I 
> think may be leading our tool to think what it's thinking.
> 
> First off, it detects a write access to desc->virt without locks inside 
> tw686x_memcpy_data_free, after it is called in the calling chain 
> tw686x_probe -> allocate an interrupt line -> tw686x_video_init -> 
> tw686x_set_format -> tw686x_memcpy_dma_free.

Until the video device has been registered there is no need for locking
since there is no possibility of concurrent access.

 Further,
> spin_lock_init(&dev->lock) (line 319 of tw686x-core.c) isn't 
> correspondingly closed in the function. Is this intended?

Huh? spin_lock_init doesn't take a lock. It just initializes internal
data.

> 
> Second, there is a possibility according to how I have traced a call 
> chain that tw686x_init is going to the error: label since 

tw686x_init? I assume you mean tw686x_video_init?

> tw686x_memcpy_dma_free is getting called inside another possible calling 
> chain, going tw686x_init -> tw686x_video_free (error: label) -> 
> dma_ops->free (i.e. tw686x_memcpy_dma_free). I would assume this would 
> not be intended either.

What tw686x_video_free() does really should be done in the release function
of the video_device: vdev->release is currently set to video_device_release,
but that should be a custom function that calls dev->dma_ops->free.

> In addition, our tool detects a read access without locks to desc->virt 
> inside tw686x_audio_irq (line 72 of tw686x-audio.c). Not sure what you 
> make of that, but I'd be keen on hearing about that as well from you.

No locks should be needed for that.

It is still not clear what your actual problem is: do you have crashes
sometimes? Is there really something wrong?

Regards,

	Hans

Re: tw686x driver (continued)

[Hans Verkuil]

On 7/24/19 3:31 PM, Mark Balançian wrote:
> On Jul 24, 2019, at 6:08 AM, Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>> wrote:
>>
>> What tw686x_video_free() does really should be done in the release function
>> of the video_device: vdev->release is currently set to video_device_release,
>> but that should be a custom function that calls dev->dma_ops->free.
> 
> Hello all,
> 
> There just appears some possible race condition as detected by the tool my supervisor has given me access to. I have to write a patch for
> the linux kernel as an assignment. By the above, does this mean I may please write the custom function that calls dev->dma_ops->free to
> replace the vdev->release = video_device_release line?

Certainly. That's definitely a sensible change.

Regards,

	Hans

Re: tw686x driver (continued)

[Ezequiel Garcia]

Hi Hans,

On Wed, 24 Jul 2019 at 10:08, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 7/24/19 2:25 PM, Mark Balançian wrote:
> > Hi Ezequiel,
> >
> > (sorry didn't include linux-media in first email)
> > I'm not sure yet if I have my supervisor's permission to declare our
> > tool as open source, but I'll tell you the possible code paths that I
> > think may be leading our tool to think what it's thinking.
> >
> > First off, it detects a write access to desc->virt without locks inside
> > tw686x_memcpy_data_free, after it is called in the calling chain
> > tw686x_probe -> allocate an interrupt line -> tw686x_video_init ->
> > tw686x_set_format -> tw686x_memcpy_dma_free.
>
> Until the video device has been registered there is no need for locking
> since there is no possibility of concurrent access.
>
>  Further,
> > spin_lock_init(&dev->lock) (line 319 of tw686x-core.c) isn't
> > correspondingly closed in the function. Is this intended?
>
> Huh? spin_lock_init doesn't take a lock. It just initializes internal
> data.
>
> >
> > Second, there is a possibility according to how I have traced a call
> > chain that tw686x_init is going to the error: label since
>
> tw686x_init? I assume you mean tw686x_video_init?
>
> > tw686x_memcpy_dma_free is getting called inside another possible calling
> > chain, going tw686x_init -> tw686x_video_free (error: label) ->
> > dma_ops->free (i.e. tw686x_memcpy_dma_free). I would assume this would
> > not be intended either.
>
> What tw686x_video_free() does really should be done in the release function
> of the video_device: vdev->release is currently set to video_device_release,
> but that should be a custom function that calls dev->dma_ops->free.
>

IIUC, v4l2_device_release is called when the last reference to the device
goes away. That, in turn, will call v4l2_dev->release(), which
is set to tw686x_dev_release.

Therefore, tw686x_dev_release is called when the last open handle
was closed, and it's where we free the last resources.

If there's any race with the consistent memory allocation/free,
it seems tw686x_dev_release would be an appropriate place
to free them.

Thanks,
Ezequiel

Re: tw686x driver (continued)

[Ezequiel Garcia]

Hi Mark,

On Wed, 24 Jul 2019 at 09:25, Mark Balançian <mbalant3@gmail.com> wrote:
>
> Hi Ezequiel,
>
> (sorry didn't include linux-media in first email)
> I'm not sure yet if I have my supervisor's permission to declare our
> tool as open source, but I'll tell you the possible code paths that I
> think may be leading our tool to think what it's thinking.
>
> First off, it detects a write access to desc->virt without locks inside
> tw686x_memcpy_data_free, after it is called in the calling chain
> tw686x_probe -> allocate an interrupt line -> tw686x_video_init ->
> tw686x_set_format -> tw686x_memcpy_dma_free. Further,
> spin_lock_init(&dev->lock) (line 319 of tw686x-core.c) isn't
> correspondingly closed in the function. Is this intended?
>

Yes, it is intended.

> Second, there is a possibility according to how I have traced a call
> chain that tw686x_init is going to the error: label since
> tw686x_memcpy_dma_free is getting called inside another possible calling
> chain, going tw686x_init -> tw686x_video_free (error: label) ->
> dma_ops->free (i.e. tw686x_memcpy_dma_free). I would assume this would
> not be intended either.
>

I'm not sure I understand what you think it's not intended, sorry.

tw686x driver (continued)

[Mark Balançian]

Hi Ezequiel,

I don’t know what top-posting is and I had to delete the continuation as 
linux-media kept flagging my email as a virus, but I take it that it 
means I should write my email below the previous? Anyways, I’m working 
with a linux driver verification team to detect race conditions in the 
kernel. Using our tool, we detected a possible race condition with the 
tw686x driver. Even if there’s the slightest thing I’d like to please 
patch it as I really need this for my enrolled program.

In any case, if interrupt handing isn’t given dedicated functions that 
are called before tw686x_memcpy_dma_free, I wouldn’t mind writing them 
and including them in a patch.

Thank you,
Mark