Reqbufs(0) need to release queued_list

PATCH In review Angela Wan 5 participants 9 messages Oct 24, 2011 – Nov 7, 2011 Media (V4L/DVB)

Share & tools

Angela Wan

author

2011-10-24 03:25 UTC #0 lore raw

Hi,
As I have used videobuf2+soc_camera architecture on my camera
driver. I find a problem when I use Reqbuf(0), which only release buffer, but not clear queued_list.
Problem description:
That is if upper layer uses qbuf then reqbuf0 directly, not having
stream on/dqbuf/off, then next time when streamon, videobuf2 could still have the buffer from queued_list which having the buffer released in privious reqbuf0, and the camera driver could access the buffer already freed.
The steps that could cause problem for USERPTR:
Qbuf
Qbuf
Reqbuf 0
Reqbuf 20
Qbuf
Qbuf
Streamon (queued_list still has the buffer already freed in the
previous reqbuf0)
.buf_queue (from camera driver, could access the buffer already freed)

My question is if we could use __vb2_queue_release which calls
__vb2_queue_cancle(clear queue_list) and __vb2_queue_free(release buffer) in Reqbuf(0), while not only use __vb2_queue_free.

Thank you

Angela Wan
Best Regards

Guennadi Liakhovetski

2011-10-24 19:39 UTC #1 lore raw

Quoted 4 lines · Angela Wan

Hi,
As I have used videobuf2+soc_camera architecture on my camera
driver. I find a problem when I use Reqbuf(0), which only release buffer, but not clear queued_list.

Indeed, looks like vb2_reqbufs(p->count == 0) fails to clean up the queue?

Thanks
Guennadi

Quoted 21 lines

Problem description:
That is if upper layer uses qbuf then reqbuf0 directly, not having
stream on/dqbuf/off, then next time when streamon, videobuf2 could still have the buffer from queued_list which having the buffer released in privious reqbuf0, and the camera driver could access the buffer already freed.
The steps that could cause problem for USERPTR:
Qbuf
Qbuf
Reqbuf 0
Reqbuf 20
Qbuf
Qbuf
Streamon (queued_list still has the buffer already freed in the
previous reqbuf0)
.buf_queue (from camera driver, could access the buffer already freed)
My question is if we could use __vb2_queue_release which calls
__vb2_queue_cancle(clear queue_list) and __vb2_queue_free(release buffer) in Reqbuf(0), while not only use __vb2_queue_free.

Quoted 9 lines

Thank you
Angela Wan
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Marek Szyprowski

2011-10-24 20:56 UTC #2 lore raw

In reply to Angela Wan

Hello,

Quoted 3 lines · Angela Wan

As I have used videobuf2+soc_camera architecture on my camera
driver. I find a problem when I use Reqbuf(0), which only release buffer, but not clear queued_list.

Thanks for pointing this bug. I will post a fix ASAP.

Best regards

Signature · 2 lines

Marek Szyprowski
Samsung Poland R&D Center

Marek Szyprowski

2011-10-25 07:59 UTC #3 lore raw

PATCH 1/1 media: vb2: reset queued list on REQBUFS(0) call

In reply to Angela Wan

Queued list was not reset on REQBUFS(0) call. This caused enqueuing a freed buffer to the driver.

Reported-by Angela Wan <[email protected]>

Signed-off-by Marek Szyprowski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 drivers/media/video/videobuf2-core.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/media/video/videobuf2-core.c +1 -0

index 3015e60..5722b81 100644
@@ -254,6 +254,7 @@ static void __vb2_queue_free(struct vb2_queue *q)
 
 	q->num_buffers = 0;
 	q->memory = 0;
+	INIT_LIST_HEAD(&q->queued_list);
 }
 
 /**

Signature · 1 line

1.7.1

Angela Wan

author

2011-10-25 08:07 UTC #4 lore raw

Re: [PATCH] media: vb2: reset queued list on REQBUFS(0) call

Hi, Marek
Why not call vb2_queue_release directly in reqbufs(0) instead of
__vb2_queue_free, which could clear queued_count as well?

Angela
Best Regards

Quoted 28 lines · Marek Szyprowski

Queued list was not reset on REQBUFS(0) call. This caused enqueuing a freed buffer to the driver.
Reported-by Angela Wan <[email protected]>
Signed-off-by Marek Szyprowski <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>
---
drivers/media/video/videobuf2-core.c | 1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/media/video/videobuf2-core.c +1 -1
index 3015e60..5722b81 100644
@@ -254,6 +254,7 @@ static void __vb2_queue_free(struct vb2_queue *q)
 
        q->num_buffers = 0;
        q->memory = 0;
+       INIT_LIST_HEAD(&q->queued_list);
  }
 
  /**
--
1.7.1

Pawel Osciak

2011-10-25 08:15 UTC #5 lore raw

Re: [PATCH] media: vb2: reset queued list on REQBUFS(0) call

Hi Angela,

Quoted 4 lines · Angela Wan

Hi, Marek
Why not call vb2_queue_release directly in reqbufs(0) instead of __vb2_queue_free, which could clear queued_count as well?

vb2_queue_release is used to clean up streaming or fileio in progress, neither of those can be active for reqbufs to proceed.

Signature · 2 lines

Best regards,
Pawel Osciak

Pawel Osciak

Acked-by on v1 1/1

2011-10-25 08:11 UTC #6 lore raw

Re: [PATCH] media: vb2: reset queued list on REQBUFS(0) call

In reply to Marek Szyprowski

Quoted 24 lines · Marek Szyprowski

Queued list was not reset on REQBUFS(0) call. This caused enqueuing a freed buffer to the driver.
Reported-by Angela Wan <[email protected]>
Signed-off-by Marek Szyprowski <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>
---
drivers/media/video/videobuf2-core.c | 1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/media/video/videobuf2-core.c +1 -1
index 3015e60..5722b81 100644
@@ -254,6 +254,7 @@ static void __vb2_queue_free(struct vb2_queue *q)
 
        q->num_buffers = 0;
        q->memory = 0;
+       INIT_LIST_HEAD(&q->queued_list);
  }
 
  /**
--

Quoted 4 lines

1.7.1

Acked-by Pawel Osciak <[email protected]>

Signature · 2 lines

Best regards,
Pawel Osciak

Mauro Carvalho Chehab

2011-11-07 13:46 UTC #7 lore raw

Re: [PATCH] media: vb2: reset queued list on REQBUFS(0) call

Em 25-10-2011 06:11, Pawel Osciak escreveu:

Quoted 24 lines

Quoted 22 lines · Marek Szyprowski
Queued list was not reset on REQBUFS(0) call. This caused enqueuing a freed buffer to the driver.
Reported-by Angela Wan <[email protected]>
Signed-off-by Marek Szyprowski <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>
---
 drivers/media/video/videobuf2-core.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/media/video/videobuf2-core.c +1 -0
index 3015e60..5722b81 100644
@@ -254,6 +254,7 @@ static void __vb2_queue_free(struct vb2_queue *q)
 
       q->num_buffers = 0;
       q->memory = 0;
+       INIT_LIST_HEAD(&q->queued_list);
 }
 

Quoted 9 lines

Quoted 6 lines
drivers/media/video/videobuf2-core.c +0 -1
 /**
--
1.7.1
Acked-by Pawel Osciak <[email protected]>

This patch doesn't apply anymore. Is it still needed? If so, please rebase.

Thanks!
Mauro

Marek Szyprowski

2011-11-07 14:15 UTC #8 lore raw

RE: [PATCH] media: vb2: reset queued list on REQBUFS(0) call

Hello,

Quoted 35 lines · Mauro Carvalho Chehab

Em 25-10-2011 06:11, Pawel Osciak escreveu:
Quoted 24 lines
Quoted 22 lines · Marek Szyprowski
Queued list was not reset on REQBUFS(0) call. This caused enqueuing a freed buffer to the driver.
Reported-by Angela Wan <[email protected]>
Signed-off-by Marek Szyprowski <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>
---
 drivers/media/video/videobuf2-core.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/media/video/videobuf2-core.c +1 -0
index 3015e60..5722b81 100644
@@ -254,6 +254,7 @@ static void __vb2_queue_free(struct vb2_queue *q)
 
       q->num_buffers = 0;
       q->memory = 0;
+       INIT_LIST_HEAD(&q->queued_list);
 }
 
Quoted 9 lines
Quoted 6 lines
drivers/media/video/videobuf2-core.c +0 -1
 /**
--
1.7.1
Acked-by Pawel Osciak <[email protected]>

Quoted 1 line

This patch doesn't apply anymore. Is it still needed? If so, please rebase.

Yes, it is needed. This patch, together with other fixes (all rebased) is waiting in the pull request I've sent on 4.11.2011 - "[GIT PULL] More Samsung patches for v3.2 (updated)" thread. Do you want me to resend all of them?

Best regards

Signature · 2 lines

Marek Szyprowski
Samsung Poland R&D Center