Re: [PATCH for v5.2] videobuf2-core.c: always reacquire USERPTR memory

[Hans Verkuil <hverkuil@xs4all.nl>]

On 6/7/19 1:16 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Fri, Jun 07, 2019 at 10:45:31AM +0200, Hans Verkuil wrote:
>> The __prepare_userptr() function made the incorrect assumption that if the
>> same user pointer was used as the last one for which memory was acquired, then
>> there was no need to re-acquire the memory. This assumption was never properly
>> tested, and after doing that it became clear that this was in fact wrong.
> 
> Could you explain in the commit message why the assumption is not
> correct ?

You can free the memory, then allocate it again and you can get the same pointer,
even though it is not necessarily using the same physical pages for the memory
that the kernel is still using for it.

Worse, you can free the memory, then allocate only half the memory you need and
get back the same pointer. vb2 wouldn't notice this. And it seems to work (since
the original mapping still remains), but this can corrupt userspace memory
causing the application to crash. It's not quite clear to me how the memory can
get corrupted. I don't know enough of those low-level mm internals to understand
the sequence of events.

I have test code for v4l2-compliance available if someone wants to test this.

> 
>> Change the behavior to always reacquire memory.
> 
> One more reason to not use USERPTR :-)

I agree.

Regards,

	Hans

> 
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Reported-by: Tomasz Figa <tfiga@chromium.org>
>> Cc: <stable@vger.kernel.org>      # for v5.1 and up
>> ---
>> This should be backported to all stable kernels, unfortunately this patch only
>> applies cleanly to 5.1, so this has to be backported manually.
>> ---
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 4489744fbbd9..a6400391117f 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1013,7 +1013,7 @@ static int __prepare_userptr(struct vb2_buffer *vb)
>>  	void *mem_priv;
>>  	unsigned int plane;
>>  	int ret = 0;
>> -	bool reacquired = vb->planes[0].mem_priv == NULL;
>> +	bool called_cleanup = false;
>>
>>  	memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
>>  	/* Copy relevant information provided by the userspace */
>> @@ -1023,15 +1023,6 @@ static int __prepare_userptr(struct vb2_buffer *vb)
>>  		return ret;
>>
>>  	for (plane = 0; plane < vb->num_planes; ++plane) {
>> -		/* Skip the plane if already verified */
>> -		if (vb->planes[plane].m.userptr &&
>> -			vb->planes[plane].m.userptr == planes[plane].m.userptr
>> -			&& vb->planes[plane].length == planes[plane].length)
>> -			continue;
>> -
>> -		dprintk(3, "userspace address for plane %d changed, reacquiring memory\n",
>> -			plane);
>> -
>>  		/* Check if the provided plane buffer is large enough */
>>  		if (planes[plane].length < vb->planes[plane].min_length) {
>>  			dprintk(1, "provided buffer size %u is less than setup size %u for plane %d\n",
>> @@ -1044,8 +1035,8 @@ static int __prepare_userptr(struct vb2_buffer *vb)
>>
>>  		/* Release previously acquired memory if present */
>>  		if (vb->planes[plane].mem_priv) {
>> -			if (!reacquired) {
>> -				reacquired = true;
>> +			if (!called_cleanup) {
>> +				called_cleanup = true;
>>  				vb->copied_timestamp = 0;
>>  				call_void_vb_qop(vb, buf_cleanup, vb);
>>  			}
> 
> Could we do this unconditionally before the loop ?
> 
>> @@ -1083,17 +1074,14 @@ static int __prepare_userptr(struct vb2_buffer *vb)
>>  		vb->planes[plane].data_offset = planes[plane].data_offset;
>>  	}
>>
>> -	if (reacquired) {
>> -		/*
>> -		 * One or more planes changed, so we must call buf_init to do
>> -		 * the driver-specific initialization on the newly acquired
>> -		 * buffer, if provided.
>> -		 */
>> -		ret = call_vb_qop(vb, buf_init, vb);
>> -		if (ret) {
>> -			dprintk(1, "buffer initialization failed\n");
>> -			goto err;
>> -		}
>> +	/*
>> +	 * Call buf_init to do the driver-specific initialization on
>> +	 * the newly acquired buffer.
>> +	 */
>> +	ret = call_vb_qop(vb, buf_init, vb);
>> +	if (ret) {
>> +		dprintk(1, "buffer initialization failed\n");
>> +		goto err;
>>  	}
>>
>>  	ret = call_vb_qop(vb, buf_prepare, vb);
>