[linux-dvb] [PATCH] 1/3: BUG FIX in dvb_ringbuffer_flush

[linux-dvb] [PATCH] 1/3: BUG FIX in dvb_ringbuffer_flush

[Andrea]

[-- Attachment #1: Type: text/plain, Size: 501 bytes --]

I've been playing with DMX_SET_BUFFER_SIZE and I've had a problem when I tried to resize the buffer
to a smaller size.

dvb_dmxdev_set_buffer_size flushes the ringbuffer and then replaces it with a new one.
What happens if the current pointer is on a position that would be invalid in the new buffer? An
access violation.

This because dvb_ringbuffer_flush resets the 2 pointers to the vaule of pwrite (which could be after
the end of the new buffer).
I think it is safer to reset them to 0.

Andrea


[-- Attachment #2: ring.diff --]
[-- Type: text/x-patch, Size: 454 bytes --]

diff -r 1886a5ea2f84 linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c
--- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Fri Mar 21 08:04:55 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Sat Mar 22 00:07:53 2008 +0000
@@ -86,7 +86,7 @@ ssize_t dvb_ringbuffer_avail(struct dvb_
 
 void dvb_ringbuffer_flush(struct dvb_ringbuffer *rbuf)
 {
-	rbuf->pread = rbuf->pwrite;
+	rbuf->pread = rbuf->pwrite = 0;
 	rbuf->error = 0;
 }
 


[-- Attachment #3: Type: text/plain, Size: 150 bytes --]

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] 1/3: BUG FIX in dvb_ringbuffer_flush

[Oliver Endriss]

Andrea wrote:
> I've been playing with DMX_SET_BUFFER_SIZE and I've had a problem when I tried to resize the buffer
> to a smaller size.
> 
> dvb_dmxdev_set_buffer_size flushes the ringbuffer and then replaces it with a new one.
> What happens if the current pointer is on a position that would be invalid in the new buffer? An
> access violation.
> 
> This because dvb_ringbuffer_flush resets the 2 pointers to the vaule of pwrite (which could be after
> the end of the new buffer).

Ack, this _is_ a bug. :-(

> I think it is safer to reset them to 0.

Nak. At the first glance one might think that this patch is correct.
Unfortunately, it introduces a subtle bug.

Citing the comments in dvb_ringbuffer.h:
| (2) If there is exactly one reader and one writer, there is no need
|     to lock read or write operations.
|     Two or more readers must be locked against each other.
|     Flushing the buffer counts as a read operation.
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|     Two or more writers must be locked against each other.

With your patch all dvb_ringbuffer_flush() calls must also be considered
as write operations. As a consequence, you would have to add spinlock
protection at many places in the code...

So I suggest to leave dvb_ringbuffer_flush() as is and zero the read and
write pointers only where it is really required...

CU
Oliver

-- 
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
----------------------------------------------------------------

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

[linux-dvb] [PATCH] 1/3: BUG FIX in dvb_ringbuffer_flush

[Andrea]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

linux-dvb-request@linuxtv.org wrote:
> Date: Sat, 22 Mar 2008 04:56:43 +0100
> From: Oliver Endriss <o.endriss@gmx.de>
> 
  > Nak. At the first glance one might think that this patch is correct.
  > Unfortunately, it introduces a subtle bug.

> So I suggest to leave dvb_ringbuffer_flush() as is and zero the read and
> write pointers only where it is really required...

Thanks for your feedback.
You are right and I have changed the code.

I've added a new function in the ringbuffer that resets the pointers to 0 and clears the error flag.
There might be some more factoring and one could move into that function 2 more lines

	buf->data = NULL;
	buf->size = size;
	dvb_ringbuffer_reset(buf);


Andrea


[-- Attachment #2: ring2.diff --]
[-- Type: text/x-patch, Size: 1661 bytes --]

diff -r 1886a5ea2f84 linux/drivers/media/dvb/dvb-core/dmxdev.c
--- a/linux/drivers/media/dvb/dvb-core/dmxdev.c	Fri Mar 21 08:04:55 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dmxdev.c	Sat Mar 22 10:12:59 2008 +0000
@@ -281,7 +313,7 @@ static int dvb_dmxdev_set_buffer_size(st
 	mem = buf->data;
 	buf->data = NULL;
 	buf->size = size;
-	dvb_ringbuffer_flush(buf);
+	dvb_ringbuffer_reset(buf);
 	spin_unlock_irq(&dmxdevfilter->dev->lock);
 	vfree(mem);
 
diff -r 1886a5ea2f84 linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c
--- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Fri Mar 21 08:04:55 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Sat Mar 22 10:12:59 2008 +0000
@@ -90,6 +90,11 @@ void dvb_ringbuffer_flush(struct dvb_rin
 	rbuf->error = 0;
 }
 
+void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf)
+{
+	rbuf->pread = rbuf->pwrite = 0;
+	rbuf->error = 0;
+}
 
 
 void dvb_ringbuffer_flush_spinlock_wakeup(struct dvb_ringbuffer *rbuf)
diff -r 1886a5ea2f84 linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h
--- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h	Fri Mar 21 08:04:55 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h	Sat Mar 22 10:12:59 2008 +0000
@@ -84,6 +84,12 @@ extern ssize_t dvb_ringbuffer_free(struc
 /* return the number of bytes waiting in the buffer */
 extern ssize_t dvb_ringbuffer_avail(struct dvb_ringbuffer *rbuf);
 
+/* 
+** Reset the read and write pointers to zero and flush the buffer
+** This counts as a read and write operation
+*/
+
+extern void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf);
 
 /* read routines & macros */
 /* ---------------------- */


[-- Attachment #3: Type: text/plain, Size: 150 bytes --]

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] 1/3: BUG FIX in dvb_ringbuffer_flush

[Andrea]

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]

Andrea wrote:
> linux-dvb-request@linuxtv.org wrote:
> I've added a new function in the ringbuffer that resets the pointers to 
> 0 and clears the error flag.
> There might be some more factoring and one could move into that function 
> 2 more lines
> 
>     buf->data = NULL;
>     buf->size = size;
>     dvb_ringbuffer_reset(buf);

I've just added a comment to patch 1/3.
I post it here again.

This patch fixes the bug in DMX_SET_BUFFER_SIZE for the demux.
Basically it resets read and write pointers to 0 in case they are beyond the new size of the buffer.

In the next patch (2/3) I rewrite this function to behave the same as the new DMX_SET_BUFFER_SIZE 
for the dvr.
I thought it is a good idea for the 2 very similar ioctl to be implemented in the same way.

Andrea

[-- Attachment #2: patch.1 --]
[-- Type: text/plain, Size: 1715 bytes --]

diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dmxdev.c
--- a/linux/drivers/media/dvb/dvb-core/dmxdev.c	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dmxdev.c	Sun Apr 13 09:50:59 2008 +0100
@@ -281,7 +281,9 @@ static int dvb_dmxdev_set_buffer_size(st
 	mem = buf->data;
 	buf->data = NULL;
 	buf->size = size;
-	dvb_ringbuffer_flush(buf);
+	
+	// reset and not flush in case the buffer shrinks
+	dvb_ringbuffer_reset(buf);
 	spin_unlock_irq(&dmxdevfilter->dev->lock);
 	vfree(mem);
 
diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c
--- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Sun Apr 13 09:50:59 2008 +0100
@@ -90,6 +90,11 @@ void dvb_ringbuffer_flush(struct dvb_rin
 	rbuf->error = 0;
 }
 
+void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf)
+{
+	rbuf->pread = rbuf->pwrite = 0;
+	rbuf->error = 0;
+}
 
 
 void dvb_ringbuffer_flush_spinlock_wakeup(struct dvb_ringbuffer *rbuf)
diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h
--- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h	Sun Apr 13 09:50:59 2008 +0100
@@ -84,6 +84,12 @@ extern ssize_t dvb_ringbuffer_free(struc
 /* return the number of bytes waiting in the buffer */
 extern ssize_t dvb_ringbuffer_avail(struct dvb_ringbuffer *rbuf);
 
+/* 
+** Reset the read and write pointers to zero and flush the buffer
+** This counts as a read and write operation
+*/
+
+extern void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf);
 
 /* read routines & macros */
 /* ---------------------- */

[-- Attachment #3: Type: text/plain, Size: 150 bytes --]

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] 1/3: BUG FIX in dvb_ringbuffer_flush

[Andrea]

[-- Attachment #1: Type: text/plain, Size: 556 bytes --]

Andrea wrote:
> I've just added a comment to patch 1/3.
> I post it here again.
> 
> This patch fixes the bug in DMX_SET_BUFFER_SIZE for the demux.
> Basically it resets read and write pointers to 0 in case they are beyond 
> the new size of the buffer.
> 
> In the next patch (2/3) I rewrite this function to behave the same as 
> the new DMX_SET_BUFFER_SIZE for the dvr.
> I thought it is a good idea for the 2 very similar ioctl to be 
> implemented in the same way.
> 
> Andrea

I've fixed some formatting errors reported by "make checkpatch".

Andrea

[-- Attachment #2: patch.1 --]
[-- Type: text/plain, Size: 1716 bytes --]

diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dmxdev.c
--- a/linux/drivers/media/dvb/dvb-core/dmxdev.c	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dmxdev.c	Sun Apr 13 10:46:37 2008 +0100
@@ -281,7 +281,9 @@ static int dvb_dmxdev_set_buffer_size(st
 	mem = buf->data;
 	buf->data = NULL;
 	buf->size = size;
-	dvb_ringbuffer_flush(buf);
+
+	/* reset and not flush in case the buffer shrinks */
+	dvb_ringbuffer_reset(buf);
 	spin_unlock_irq(&dmxdevfilter->dev->lock);
 	vfree(mem);
 
diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c
--- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Sun Apr 13 10:46:37 2008 +0100
@@ -90,6 +90,11 @@ void dvb_ringbuffer_flush(struct dvb_rin
 	rbuf->error = 0;
 }
 
+void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf)
+{
+	rbuf->pread = rbuf->pwrite = 0;
+	rbuf->error = 0;
+}
 
 
 void dvb_ringbuffer_flush_spinlock_wakeup(struct dvb_ringbuffer *rbuf)
diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h
--- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h	Sun Apr 13 10:46:37 2008 +0100
@@ -84,6 +84,12 @@ extern ssize_t dvb_ringbuffer_free(struc
 /* return the number of bytes waiting in the buffer */
 extern ssize_t dvb_ringbuffer_avail(struct dvb_ringbuffer *rbuf);
 
+/*
+** Reset the read and write pointers to zero and flush the buffer
+** This counts as a read and write operation
+*/
+
+extern void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf);
 
 /* read routines & macros */
 /* ---------------------- */

[-- Attachment #3: Type: text/plain, Size: 150 bytes --]

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] 1/3: BUG FIX in dvb_ringbuffer_flush

[Andrea]

Andrea wrote:
> Andrea wrote:
>> I've just added a comment to patch 1/3.
>> I post it here again.
>>
>> This patch fixes the bug in DMX_SET_BUFFER_SIZE for the demux.
>> Basically it resets read and write pointers to 0 in case they are 
>> beyond the new size of the buffer.
>>
>> In the next patch (2/3) I rewrite this function to behave the same as 
>> the new DMX_SET_BUFFER_SIZE for the dvr.
>> I thought it is a good idea for the 2 very similar ioctl to be 
>> implemented in the same way.
>>
>> Andrea
> 
> I've fixed some formatting errors reported by "make checkpatch".
> 
> Andrea


This patch fixes a bug in DMX_SET_BUFFER_SIZE in case the buffer shrinks.

Signed-off-by: Andrea Odetti <mariofutire@gmail.com>

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] 1/3: BUG FIX in dvb_ringbuffer_flush

[Oliver Endriss]

Andrea wrote:
> Andrea wrote:
> > Andrea wrote:
> >> I've just added a comment to patch 1/3.
> >> I post it here again.
> >>
> >> This patch fixes the bug in DMX_SET_BUFFER_SIZE for the demux.
> >> Basically it resets read and write pointers to 0 in case they are 
> >> beyond the new size of the buffer.
> >>
> >> In the next patch (2/3) I rewrite this function to behave the same as 
> >> the new DMX_SET_BUFFER_SIZE for the dvr.
> >> I thought it is a good idea for the 2 very similar ioctl to be 
> >> implemented in the same way.
> >>
> >> Andrea
> > 
> > I've fixed some formatting errors reported by "make checkpatch".
> > 
> > Andrea
> 
> 
> This patch fixes a bug in DMX_SET_BUFFER_SIZE in case the buffer shrinks.
> 
> Signed-off-by: Andrea Odetti <mariofutire@gmail.com>

Committed to HG. Thanks.

CU
Oliver

-- 
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
----------------------------------------------------------------

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb