videobuf2-vmalloc suspect for corrupted data

videobuf2-vmalloc suspect for corrupted data

[Divneil Wadhawan]

Hi,

I have a V4L2 capture driver accepting a malloc'ed buffer. 
The driver is using vb2_vmalloc_memops (../drivers/media/v4l2-core/videobuf2-vmalloc.c) for user-space to kvaddr translation.
Randomly, corrupted data is received by user-app.

So, the question is regarding the handling of get_userptr, put_userptr by v4l2-core:

const struct vb2_mem_ops vb2_vmalloc_memops = {
         ........
         .get_userptr    = vb2_vmalloc_get_userptr, (get_user_pages() and vm_map_ram())
         .put_userptr    = vb2_vmalloc_put_userptr, (set_page_dirty_lock() and put_page())
          .....
};

The driver prepares for the transaction by virtue of v4l2-core calling get_userptr (QBUF) 
After data is filled, driver puts on a done list (DQBUF)

We never mark the pages as dirty (or put_userptr) after a transaction is complete.
Here, in v4l2 core (videobuf2-core.c) , we conditionally put_userptr - when a QBUF with a different userptr on same index, or releasing buffers.

Is it correct? Probably seems to be the reason for corrupted data.

Regards,
Divneil 		 	   		  

Re: videobuf2-vmalloc suspect for corrupted data

[Pawel Osciak]

Hi Divneil,

On Mon, Apr 7, 2014 at 6:56 PM, Divneil Wadhawan <divneil@outlook.com> wrote:
> Hi,
>
> I have a V4L2 capture driver accepting a malloc'ed buffer.
> The driver is using vb2_vmalloc_memops (../drivers/media/v4l2-core/videobuf2-vmalloc.c) for user-space to kvaddr translation.
> Randomly, corrupted data is received by user-app.
>
> So, the question is regarding the handling of get_userptr, put_userptr by v4l2-core:
>
> const struct vb2_mem_ops vb2_vmalloc_memops = {
>          ........
>          .get_userptr    = vb2_vmalloc_get_userptr, (get_user_pages() and vm_map_ram())
>          .put_userptr    = vb2_vmalloc_put_userptr, (set_page_dirty_lock() and put_page())
>           .....
> };
>
> The driver prepares for the transaction by virtue of v4l2-core calling get_userptr (QBUF)
> After data is filled, driver puts on a done list (DQBUF)
>
> We never mark the pages as dirty (or put_userptr) after a transaction is complete.
> Here, in v4l2 core (videobuf2-core.c) , we conditionally put_userptr - when a QBUF with a different userptr on same index, or releasing buffers.
>
> Is it correct? Probably seems to be the reason for corrupted data.

This is an optimization and requires the mapping of userspace buffer
to v4l2_buffer index to not change.
Is it possible that your userspace is not always queuing the same
userptr memory areas with the same v4l2_buffer index values?
In other words, if you have 2 buffers in use, under userspace mapping
at addr1 and addr2, if you queue addr1 with index=0 and addr2 with
index=1 initially,
you should always keep queuing addr1 with index=0 and never 1, etc.

Also, what architecture are you running this on?

> Regards,
> Divneil                                           --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Best regards,
Pawel Osciak

RE: videobuf2-vmalloc suspect for corrupted data

[Divneil Wadhawan]

Hi Pawel,

Thanks for the quick response.

> Is it possible that your userspace is not always queuing the same
> userptr memory areas with the same v4l2_buffer index values?
No, userptr is always consistent with the index.
In fact, when we dump the captured buffer (Transport Stream) in this case, kernel space data and user-space are different.
When that TS is played, macroblocks are observed from user-space and not from the kernel space dump.
Although, user-space bad data is random, but, I have never seen kernel space dumped TS as bad.

> In other words, if you have 2 buffers in use, under userspace mapping
> at addr1 and addr2, if you queue addr1 with index=0 and addr2 with
> index=1 initially,
> you should always keep queuing addr1 with index=0 and never 1, etc.
Yeah! this is the same rule which is being followed.

> Also, what architecture are you running this on?
ARM Cortex A9 SMP

Regards,
Divneil 		 	   		  

Re: videobuf2-vmalloc suspect for corrupted data

[Hans Verkuil]

On 04/07/2014 12:49 PM, Divneil Wadhawan wrote:
> Hi Pawel,
> 
> Thanks for the quick response.
> 
>> Is it possible that your userspace is not always queuing the same
>> userptr memory areas with the same v4l2_buffer index values?
> No, userptr is always consistent with the index.
> In fact, when we dump the captured buffer (Transport Stream) in this case, kernel space data and user-space are different.
> When that TS is played, macroblocks are observed from user-space and not from the kernel space dump.
> Although, user-space bad data is random, but, I have never seen kernel space dumped TS as bad.
> 
>> In other words, if you have 2 buffers in use, under userspace mapping
>> at addr1 and addr2, if you queue addr1 with index=0 and addr2 with
>> index=1 initially,
>> you should always keep queuing addr1 with index=0 and never 1, etc.
> Yeah! this is the same rule which is being followed.
> 
>> Also, what architecture are you running this on?
> ARM Cortex A9 SMP

Two more questions:

Which kernel version are you using?

Which capture driver are you using?

Regards,

	Hans

RE: videobuf2-vmalloc suspect for corrupted data

[Divneil Wadhawan]

Hi Hans,
> Two more questions:
>
> Which kernel version are you using?
3.4.58

> Which capture driver are you using?
It's a TSMUX driver, written locally.

Regards,
Divneil 		 	   		  

RE: videobuf2-vmalloc suspect for corrupted data

[Divneil Wadhawan]

>> Which capture driver are you using?
> It's a TSMUX driver, written locally.
In complete, it's a Multi-INPUT, single output (MUXER) driver, but, currently, it's the capture side fault here.
Regards,
Divneil 		 	   		  

Re: videobuf2-vmalloc suspect for corrupted data

[Hans Verkuil]

On 04/07/2014 01:20 PM, Divneil Wadhawan wrote:
> Hi Hans,
>> Two more questions:
>>
>> Which kernel version are you using?
> 3.4.58

That should be new enough, I see no important differences between 3.4
and 3.14 in this respect. But really, 3.4? That's over two years old!
If you have control over what kernel you use then I recommend you
upgrade.

>> Which capture driver are you using?
> It's a TSMUX driver, written locally.

I have not seen any reports of problems with vmalloc with arm in a long
time. I know the uvc driver uses vmalloc, and that's used frequently.

Question: if you use MEMORY_MMAP instead of USERPTR, does that work?

Have you tried to stream with v4l2-ctl? It's available here:
http://git.linuxtv.org/cgit.cgi/v4l-utils.git/. It's the reference
implementation of how to stream, so if that fails as well, then at
least its not your application.

Testing whether you see the same when capturing from a usb uvc webcam
(most webcams are uvc these days) would be useful as well. If it works
with a uvc webcam, but not with your driver, then I suspect the driver.

Regards,

	Hans

RE: videobuf2-vmalloc suspect for corrupted data

[Divneil Wadhawan]

Hi Hans,

> That should be new enough, I see no important differences between 3.4
> and 3.14 in this respect. But really, 3.4?
We are moving to 3.10 shortly.

> Question: if you use MEMORY_MMAP instead of USERPTR, does that work?
Unfortunately, this activity is pending (need to get it pushed in plan). I need to port driver and app to do that.

> Have you tried to stream with v4l2-ctl? It's available here:
No, need to see.


Read a bit about vm_map_ram(), it gives a kernel vaddr for user pages in which the user vaddr are residing.
Can the virtually indexed D-cache be an issue here, or am I too paranoid?


I just checked on some webpage probably arm center, that D-caches are physically tagged, so, the above could be ruled out.

What about the architectures?


I will look into the driver more thoroughly, because, for kernel.org code, I am least skeptical.


Regards,
Divneil