media: videobuf2-core: copy vb planes unconditionally

[PATCH] media: videobuf2-core: copy vb planes unconditionally

Posted by Tudor Ambarus 2 weeks, 3 days ago

Copy the relevant data from userspace to the vb->planes unconditionally
as it's possible some of the fields may have changed after the buffer
has been validated.

Keep the dma_buf_put(planes[plane].dbuf) calls in the first
`if (!reacquired)` case, in order to be close to the plane validation code
where the buffers were got in the first place.

Cc: stable@vger.kernel.org
Fixes: 95af7c00f35b ("media: videobuf2-core: release all planes first in __prepare_dmabuf()")
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 .../media/common/videobuf2/videobuf2-core.c   | 28 ++++++++++---------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index f07dc53a9d06..c0cc441b5164 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1482,18 +1482,23 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
 			}
 			vb->planes[plane].dbuf_mapped = 1;
 		}
+	} else {
+		for (plane = 0; plane < vb->num_planes; ++plane)
+			dma_buf_put(planes[plane].dbuf);
+	}
 
-		/*
-		 * Now that everything is in order, copy relevant information
-		 * provided by userspace.
-		 */
-		for (plane = 0; plane < vb->num_planes; ++plane) {
-			vb->planes[plane].bytesused = planes[plane].bytesused;
-			vb->planes[plane].length = planes[plane].length;
-			vb->planes[plane].m.fd = planes[plane].m.fd;
-			vb->planes[plane].data_offset = planes[plane].data_offset;
-		}
+	/*
+	 * Now that everything is in order, copy relevant information
+	 * provided by userspace.
+	 */
+	for (plane = 0; plane < vb->num_planes; ++plane) {
+		vb->planes[plane].bytesused = planes[plane].bytesused;
+		vb->planes[plane].length = planes[plane].length;
+		vb->planes[plane].m.fd = planes[plane].m.fd;
+		vb->planes[plane].data_offset = planes[plane].data_offset;
+	}
 
+	if (reacquired) {
 		/*
 		 * Call driver-specific initialization on the newly acquired buffer,
 		 * if provided.
@@ -1503,9 +1508,6 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
 			dprintk(q, 1, "buffer initialization failed\n");
 			goto err_put_vb2_buf;
 		}
-	} else {
-		for (plane = 0; plane < vb->num_planes; ++plane)
-			dma_buf_put(planes[plane].dbuf);
 	}
 
 	ret = call_vb_qop(vb, buf_prepare, vb);
-- 
2.47.0.199.ga7371fff76-goog

Re: [PATCH] media: videobuf2-core: copy vb planes unconditionally

Posted by Tomasz Figa 2 weeks, 3 days ago

On Wed, Nov 6, 2024 at 9:18 PM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Copy the relevant data from userspace to the vb->planes unconditionally
> as it's possible some of the fields may have changed after the buffer
> has been validated.
>
> Keep the dma_buf_put(planes[plane].dbuf) calls in the first
> `if (!reacquired)` case, in order to be close to the plane validation code
> where the buffers were got in the first place.
>
> Cc: stable@vger.kernel.org
> Fixes: 95af7c00f35b ("media: videobuf2-core: release all planes first in __prepare_dmabuf()")
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 28 ++++++++++---------
>  1 file changed, 15 insertions(+), 13 deletions(-)
>

Thanks for the fix.

Acked-by: Tomasz Figa <tfiga@chromium.org>

(We probably need some tests to verify this behavior... It seems like
the way v4l2-compliance is implemented [1] would only trigger the
!reacquired case on most drivers.)

[1] https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-test-buffers.cpp#n2071
(just queuing all imported buffers in order and re-queuing them
exactly as they are dequeued [2])
[2] https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-test-buffers.cpp#n1299

Best regards,
Tomasz

> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index f07dc53a9d06..c0cc441b5164 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1482,18 +1482,23 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>                         }
>                         vb->planes[plane].dbuf_mapped = 1;
>                 }
> +       } else {
> +               for (plane = 0; plane < vb->num_planes; ++plane)
> +                       dma_buf_put(planes[plane].dbuf);
> +       }
>
> -               /*
> -                * Now that everything is in order, copy relevant information
> -                * provided by userspace.
> -                */
> -               for (plane = 0; plane < vb->num_planes; ++plane) {
> -                       vb->planes[plane].bytesused = planes[plane].bytesused;
> -                       vb->planes[plane].length = planes[plane].length;
> -                       vb->planes[plane].m.fd = planes[plane].m.fd;
> -                       vb->planes[plane].data_offset = planes[plane].data_offset;
> -               }
> +       /*
> +        * Now that everything is in order, copy relevant information
> +        * provided by userspace.
> +        */
> +       for (plane = 0; plane < vb->num_planes; ++plane) {
> +               vb->planes[plane].bytesused = planes[plane].bytesused;
> +               vb->planes[plane].length = planes[plane].length;
> +               vb->planes[plane].m.fd = planes[plane].m.fd;
> +               vb->planes[plane].data_offset = planes[plane].data_offset;
> +       }
>
> +       if (reacquired) {
>                 /*
>                  * Call driver-specific initialization on the newly acquired buffer,
>                  * if provided.
> @@ -1503,9 +1508,6 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>                         dprintk(q, 1, "buffer initialization failed\n");
>                         goto err_put_vb2_buf;
>                 }
> -       } else {
> -               for (plane = 0; plane < vb->num_planes; ++plane)
> -                       dma_buf_put(planes[plane].dbuf);
>         }
>
>         ret = call_vb_qop(vb, buf_prepare, vb);
> --
> 2.47.0.199.ga7371fff76-goog
>

Re: [PATCH] media: videobuf2-core: copy vb planes unconditionally

Posted by Hans Verkuil 2 weeks, 2 days ago

On 07/11/2024 00:41, Tomasz Figa wrote:
> On Wed, Nov 6, 2024 at 9:18 PM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> Copy the relevant data from userspace to the vb->planes unconditionally
>> as it's possible some of the fields may have changed after the buffer
>> has been validated.
>>
>> Keep the dma_buf_put(planes[plane].dbuf) calls in the first
>> `if (!reacquired)` case, in order to be close to the plane validation code
>> where the buffers were got in the first place.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 95af7c00f35b ("media: videobuf2-core: release all planes first in __prepare_dmabuf()")
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
>>  .../media/common/videobuf2/videobuf2-core.c   | 28 ++++++++++---------
>>  1 file changed, 15 insertions(+), 13 deletions(-)
>>
> 
> Thanks for the fix.
> 
> Acked-by: Tomasz Figa <tfiga@chromium.org>
> 
> (We probably need some tests to verify this behavior... It seems like
> the way v4l2-compliance is implemented [1] would only trigger the
> !reacquired case on most drivers.)
> 
> [1] https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-test-buffers.cpp#n2071
> (just queuing all imported buffers in order and re-queuing them
> exactly as they are dequeued [2])
> [2] https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-test-buffers.cpp#n1299

I'll see if I can improve that test.

Regards,

	Hans

> 
> Best regards,
> Tomasz
> 
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index f07dc53a9d06..c0cc441b5164 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1482,18 +1482,23 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>>                         }
>>                         vb->planes[plane].dbuf_mapped = 1;
>>                 }
>> +       } else {
>> +               for (plane = 0; plane < vb->num_planes; ++plane)
>> +                       dma_buf_put(planes[plane].dbuf);
>> +       }
>>
>> -               /*
>> -                * Now that everything is in order, copy relevant information
>> -                * provided by userspace.
>> -                */
>> -               for (plane = 0; plane < vb->num_planes; ++plane) {
>> -                       vb->planes[plane].bytesused = planes[plane].bytesused;
>> -                       vb->planes[plane].length = planes[plane].length;
>> -                       vb->planes[plane].m.fd = planes[plane].m.fd;
>> -                       vb->planes[plane].data_offset = planes[plane].data_offset;
>> -               }
>> +       /*
>> +        * Now that everything is in order, copy relevant information
>> +        * provided by userspace.
>> +        */
>> +       for (plane = 0; plane < vb->num_planes; ++plane) {
>> +               vb->planes[plane].bytesused = planes[plane].bytesused;
>> +               vb->planes[plane].length = planes[plane].length;
>> +               vb->planes[plane].m.fd = planes[plane].m.fd;
>> +               vb->planes[plane].data_offset = planes[plane].data_offset;
>> +       }
>>
>> +       if (reacquired) {
>>                 /*
>>                  * Call driver-specific initialization on the newly acquired buffer,
>>                  * if provided.
>> @@ -1503,9 +1508,6 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>>                         dprintk(q, 1, "buffer initialization failed\n");
>>                         goto err_put_vb2_buf;
>>                 }
>> -       } else {
>> -               for (plane = 0; plane < vb->num_planes; ++plane)
>> -                       dma_buf_put(planes[plane].dbuf);
>>         }
>>
>>         ret = call_vb_qop(vb, buf_prepare, vb);
>> --
>> 2.47.0.199.ga7371fff76-goog
>>

Re: [PATCH] media: videobuf2-core: copy vb planes unconditionally

Posted by William McVicker 2 weeks, 3 days ago

Hi Tudor,

On 11/06/2024, Tudor Ambarus wrote:
> Copy the relevant data from userspace to the vb->planes unconditionally
> as it's possible some of the fields may have changed after the buffer
> has been validated.
> 
> Keep the dma_buf_put(planes[plane].dbuf) calls in the first
> `if (!reacquired)` case, in order to be close to the plane validation code
> where the buffers were got in the first place.
> 
> Cc: stable@vger.kernel.org
> Fixes: 95af7c00f35b ("media: videobuf2-core: release all planes first in __prepare_dmabuf()")
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>

Thanks for sending this fix! I have tested that this fixes the video playback
issues I was seeing on my Pixel 6 device. Feel free to include my

Tested-by: Will McVicker <willmcvicker@google.com>

Thanks!
--Will