bttv driver memory corruption

PATCH New Laurent Birtz 1 participant 1 message Aug 16, 2010 Media (V4L/DVB)

lore

Share & tools

lore

Laurent Birtz

author

2010-08-16 17:14 UTC #0 lore raw

PATCH 1/1 bttv driver memory corruption

Hello,

here is a bug report for the bttv driver and tentative patches to solve the problem. Two bugs are addressed, a memory corruption of random kernel pages and a race condition in the DMA RISC code.

I couldn't get checkpatch.pl to work so the patches are in plain 'hg diff' format.

The card I am using is Osprey 440, Bt878 (rev 17).

I have tested my patches on kernel 2.6.18-164.el5 (RHEL 5) which is the kernel on my target production machines. The V4L2_MEMORY_USERPTR method was not tested as I don't have the proper support functions in this kernel; I also don't know how to reliably allocate physically contiguous DMA32 memory in user space.

I have checked that VBI capture doesn't crash the kernel but not that it works correctly. I haven't tested the capture of only a single field, odd or even.

I've used the driver code revision ee9826bc7106 (hg). The stable tip crashes my kernel on module load and I don't have the resources to investigate this problem at this time.

Memory corruption:

The bt878 chipset has a hardware limitation on the buffers used to capture a line. Quote from the specification:

"The target memory for a given scan line of data is assumed to be linear, incrementing, and contiguous. For a 1024-pixel scan line a maximum of 4 KB of contiguous physical memory is required. Each scan line can be stored anywhere in the 32-bit address space. A scan line can be broken into segments with each segment sent to a different target area. An image buffer can be allocated to line fragments anywhere in the physical memory, as the line sequence is arbitrary."

The fourth sentence seems to directly contradict the first but it appears that the first sentence is true. I've observed a memory corruption by allocating a single buffer twice as large as necessary, dividing it into pages and poisoning all of it. When the user memory maps a buffer, I allocate every even page from the poisoned buffer in the nopage fault handler.

Picture: A=Allocated, P=Poison, PoisonedBuffer=APAPAPAP...

I've observed that the poison was corrupted at some random pages, but always in the first four bytes. The value of those bytes is 0x23232323. This byte sequence often appears in kernel oops reports on my setup.

I dumped the content of the RISC write instructions and noticed that while the corruptions are random, they always occur when the 'end of line' bit of the write instructions is not set and the instructions write data up to the end of a page. It looks like the DMA controller gets confused when it needs to write to non-contiguous memory.

The first patch changes the use of scatter-gather buffers for a single contiguous buffer. I have emulated the scatter-gather behavior in the RISC subprogram setup code. Obviously the code can be made simpler when a single buffer is used. As a side note, the code in bttv_risc_planar() appears to choke if the number of bytes to write in 'todo' is not a multiple of 4, due to the horizontal shift.

The use of line-based scatter-gather buffers does not seem appealing, since it requires memory copies to present user-space with the contiguous buffer that it expects.

RISC program synchronization:

All modifications to the main RISC program and its subprograms (i.e. the RISC instructions that fill the content of a buffer) and to the card registers are protected by a spinlock. However, the driver takes no provision to avoid race conditions with the DMA controller itself. Holding the spinlock or servicing an IRQ does not prevent the DMA controller from executing RISC instructions. This leads to a race condition that can crash the kernel.

The current RISC program loops over all its instructions continuously. When a RISC interrupt is raised, the IRQ handler modifies the program, without knowing which instruction the DMA controller is currently executing. Usually, this will be the SYNC instruction for the odd field as this stalls the DMA controller for a while.

If the IRQ handler is slow, which can happen when it waits on the spinlock, then the DMA controller will re-execute a subprogram capturing a field. The code tries to detect this situation with the is_active() macro but this method is not reliable. In the case where the IRQ handler dequeues a buffer while it is active, then a kernel crash can occur. Assuming the application quits at this point, the function bttv_dma_free() will find that the capture buffer is done and then free the buffer containing the RISC instructions being executed by the DMA controller.

The proposed fix modifies the driver to solve this race condition. The implementation is kept as simple as possible and preserve the existing synchronization logic. The code is not optimal for performance as it needlessly causes an interrupt to be raised when both fields are being captured with the same buffer. The code could also be made simpler by reworking the logic.

The code uses gates to control the flow of execution of the RISC program. A gate is a jump instruction that either jumps to itself (when the gate is closed) or jumps to the next capture instructions (when the gate is open). The IRQ handler uses two gates in alternance to functionally stop the DMA controller when the RISC program is being modified. When a RISC interrupt is raised, the IRQ handler verifies that the DMA controller loops harmlessly on the closed gate, modifies the RISC program, close the open gate, flush the memory and open the closed gate.

Thank you,
Laurent Birtz

Signed-off-by Laurent Birtz <[email protected]>

linux/drivers/media/video/bt8xx/bttv-driver.c +16 ~~-16~~

diff -r ee9826bc7106 linux/drivers/media/video/bt8xx/bttv-driver.c
@@ -2403,7 +2403,7 @@
 	if (check_btres(fh, RESOURCE_OVERLAY)) {
 		struct bttv_buffer *new;
 
-		new = videobuf_sg_alloc(sizeof(*new));
+		new = videobuf_contig_alloc(sizeof(*new));
 		new->crop = btv->crop[!!fh->do_crop].rect;
 		bttv_overlay_risc(btv, &fh->ov, fh->ovfmt, new);
 		retval = bttv_switch_overlay(btv,fh,new);

@@ -2780,7 +2780,7 @@
 	mutex_lock(&fh->cap.vb_lock);
 	if (on) {
 		fh->ov.tvnorm = btv->tvnorm;
-		new = videobuf_sg_alloc(sizeof(*new));
+		new = videobuf_contig_alloc(sizeof(*new));
 		new->crop = btv->crop[!!fh->do_crop].rect;
 		bttv_overlay_risc(btv, &fh->ov, fh->ovfmt, new);
 	} else {

@@ -2854,7 +2854,7 @@
 		if (check_btres(fh, RESOURCE_OVERLAY)) {
 			struct bttv_buffer *new;
 
-			new = videobuf_sg_alloc(sizeof(*new));
+			new = videobuf_contig_alloc(sizeof(*new));
 			new->crop = btv->crop[!!fh->do_crop].rect;
 			bttv_overlay_risc(btv, &fh->ov, fh->ovfmt, new);
 			retval = bttv_switch_overlay(btv, fh, new);

@@ -3207,7 +3207,7 @@
 			/* need to capture a new frame */
 			if (locked_btres(fh->btv,RESOURCE_VIDEO_STREAM))
 				goto err;
-			fh->cap.read_buf = videobuf_sg_alloc(fh->cap.msize);
+			fh->cap.read_buf = videobuf_contig_alloc(fh->cap.msize);
 			if (NULL == fh->cap.read_buf)
 				goto err;
 			fh->cap.read_buf->memory = V4L2_MEMORY_USERPTR;

@@ -3270,18 +3270,18 @@
 	fh->ov.setup_ok = 0;
 	v4l2_prio_open(&btv->prio,&fh->prio);
 
-	videobuf_queue_sg_init(&fh->cap, &bttv_video_qops,
-			    &btv->c.pci->dev, &btv->s_lock,
-			    V4L2_BUF_TYPE_VIDEO_CAPTURE,
-			    V4L2_FIELD_INTERLACED,
-			    sizeof(struct bttv_buffer),
-			    fh);
-	videobuf_queue_sg_init(&fh->vbi, &bttv_vbi_qops,
-			    &btv->c.pci->dev, &btv->s_lock,
-			    V4L2_BUF_TYPE_VBI_CAPTURE,
-			    V4L2_FIELD_SEQ_TB,
-			    sizeof(struct bttv_buffer),
-			    fh);
+	videobuf_queue_dma_contig_init(&fh->cap, &bttv_video_qops,
+				       &btv->c.pci->dev, &btv->s_lock,
+				       V4L2_BUF_TYPE_VIDEO_CAPTURE,
+				       V4L2_FIELD_INTERLACED,
+				       sizeof(struct bttv_buffer),
+				       fh);
+	videobuf_queue_dma_contig_init(&fh->vbi, &bttv_vbi_qops,
+				       &btv->c.pci->dev, &btv->s_lock,
+				       V4L2_BUF_TYPE_VBI_CAPTURE,
+				       V4L2_FIELD_SEQ_TB,
+				       sizeof(struct bttv_buffer),
+				       fh);
 	set_tvnorm(btv,btv->tvnorm);
 	set_input(btv, btv->input, btv->tvnorm);

linux/drivers/media/video/bt8xx/bttv-risc.c +25 ~~-19~~

121 lines not on this page · raw message

linux/drivers/media/video/bt8xx/bttv-vbi.c +12 -3

diff -r ee9826bc7106 linux/drivers/media/video/bt8xx/bttv-vbi.c
@@ -151,14 +151,23 @@
 
 	if (redo_dma_risc) {
 		unsigned int bpl, padding, offset;
-		struct videobuf_dmabuf *dma=videobuf_to_dma(&buf->vb);
+		struct videobuf_dma_contig_memory *dma =
+			videobuf_to_dma_contig_memory(&buf->vb);
+		struct scatterlist sglist;
+		
+		/* Create a one-entry scatter list for compatibility. */
+		sglist.page = virt_to_page(dma->vaddr);
+		sglist.offset = 0;
+		sglist.length = dma->size;
+		sglist.dma_address = dma->dma_handle;
+		sglist.dma_length = dma->size;
 
 		bpl = 2044; /* max. vbipack */
 		padding = VBI_BPL - bpl;
 
 		if (fh->vbi_fmt.fmt.count[0] > 0) {
 			rc = bttv_risc_packed(btv, &buf->top,
-					      dma->sglist,
+					      &sglist,
 					      /* offset */ 0, bpl,
 					      padding, skip_lines0,
 					      fh->vbi_fmt.fmt.count[0]);

@@ -170,7 +179,7 @@
 			offset = fh->vbi_fmt.fmt.count[0] * VBI_BPL;
 
 			rc = bttv_risc_packed(btv, &buf->bottom,
-					      dma->sglist,
+					      &sglist,
 					      offset, bpl,
 					      padding, skip_lines1,
 					      fh->vbi_fmt.fmt.count[1]);

linux/drivers/media/video/bt8xx/bttvp.h +1 -1

diff -r ee9826bc7106 linux/drivers/media/video/bt8xx/bttvp.h
@@ -40,7 +40,7 @@
 #include "compat.h"
 #include <media/v4l2-common.h>
 #include <linux/device.h>
-#include <media/videobuf-dma-sg.h>
+#include <media/videobuf-dma-contig.h>
 #include <media/tveeprom.h>
 #include <media/ir-common.h>

linux/drivers/media/video/videobuf-dma-contig.c +32 -8

diff -r ee9826bc7106 linux/drivers/media/video/videobuf-dma-contig.c
@@ -23,14 +23,6 @@
 #include <media/videobuf-dma-contig.h>
 #include "compat.h"
 
-struct videobuf_dma_contig_memory {
-	u32 magic;
-	void *vaddr;
-	dma_addr_t dma_handle;
-	unsigned long size;
-	int is_userptr;
-};
-
 #define MAGIC_DC_MEM 0x0733ac61
 #define MAGIC_CHECK(is, should)						    \
 	if (unlikely((is) != (should)))	{				    \

@@ -142,6 +134,10 @@
 static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory *mem,
 					struct videobuf_buffer *vb)
 {
+/* TODO: Find out equivalent of follow_pfn for kernel 2.6.18. */
+#if LINUX_VERSION_CODE <= KERNEL_VERSION(2, 6, 18)
+	return -EINVAL;
+#else
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma;
 	unsigned long prev_pfn, this_pfn;

@@ -192,6 +188,7 @@
 	up_read(&current->mm->mmap_sem);
 
 	return ret;
+#endif
 }
 
 static void *__videobuf_alloc(size_t size)

@@ -435,6 +432,33 @@
 	.vmalloc      = __videobuf_to_vmalloc,
 };
 
+struct videobuf_dma_contig_memory *
+videobuf_to_dma_contig_memory(struct videobuf_buffer *buf)
+{
+	struct videobuf_dma_contig_memory *mem = buf->priv;
+	BUG_ON(!mem);
+
+	MAGIC_CHECK(mem->magic, MAGIC_DC_MEM);
+
+	return mem;
+}
+
+EXPORT_SYMBOL_GPL(videobuf_to_dma_contig_memory);
+
+/* This is a kludge to allocate memory without a queue object. */
+void *videobuf_contig_alloc(size_t size)
+{
+        struct videobuf_queue q;
+
+        /* Required to make generic handler to call __videobuf_alloc */
+        q.int_ops = &qops;
+
+        q.msize = size;
+
+        return videobuf_alloc(&q);
+}
+EXPORT_SYMBOL_GPL(videobuf_contig_alloc);
+
 void videobuf_queue_dma_contig_init(struct videobuf_queue *q,
 				    const struct videobuf_queue_ops *ops,
 				    struct device *dev,

linux/include/media/videobuf-dma-contig.h +11 -0

diff -r ee9826bc7106 linux/include/media/videobuf-dma-contig.h
@@ -16,6 +16,17 @@
 #include <linux/dma-mapping.h>
 #include <media/videobuf-core.h>
 
+struct videobuf_dma_contig_memory {
+	u32 magic;
+	void *vaddr;
+	dma_addr_t dma_handle;
+	unsigned long size;
+	int is_userptr;
+};
+
+struct videobuf_dma_contig_memory *
+videobuf_to_dma_contig_memory(struct videobuf_buffer *buf);
+void *videobuf_contig_alloc(size_t size);
 void videobuf_queue_dma_contig_init(struct videobuf_queue *q,
 				    const struct videobuf_queue_ops *ops,
 				    struct device *dev,

linux/drivers/media/video/bt8xx/bttv-driver.c +115 ~~-37~~

202 lines not on this page · raw message

linux/drivers/media/video/bt8xx/bttv-risc.c +51 ~~-39~~

140 lines not on this page · raw message

linux/drivers/media/video/bt8xx/bttvp.h +23 -5

diff -r ee9826bc7106 linux/drivers/media/video/bt8xx/bttvp.h
@@ -58,11 +58,11 @@
 #define FORMAT_FLAGS_RAW          0x08
 #define FORMAT_FLAGS_CrCb         0x10
 
-#define RISC_SLOT_O_VBI        4
-#define RISC_SLOT_O_FIELD      6
-#define RISC_SLOT_E_VBI       10
-#define RISC_SLOT_E_FIELD     12
-#define RISC_SLOT_LOOP        14
+#define RISC_SLOT_O_VBI        0
+#define RISC_SLOT_O_FIELD      1
+#define RISC_SLOT_E_VBI        2
+#define RISC_SLOT_E_FIELD      3
+#define RISC_SLOT_LOOP         4
 
 #define RESOURCE_OVERLAY       1
 #define RESOURCE_VIDEO_STREAM  2

@@ -428,6 +428,24 @@
 	struct bttv_buffer      *cvbi;      /* active vbi buffer   */
 	int                     loop_irq;
 	int                     new_input;
+	int                     closed_gate; /* ID (0 or 1) of the gate in the  
+						main RISC program that is  
+						currently closed. */ 
+	struct btcx_riscmem    *sub_addr[4]; /* Address of the capture RISC  
+						subprograms. */ 
+	int                     sub_irq[4];  /* IRQ flags of the capture 
+						RISC subprograms. */ 
+	int                     cur_irq;     /* IRQ flags that have been set for 
+						the current slot. */ 
+	int                     sub_slot;    /* Current subprogram being 
+						executed. 
+						0: VBI top. 
+						1: Video top. 
+						2: VBI bottom. 
+						3: Video bottom. 
+						4: IRQ loop scheduling. */ 
+	int                     field_sync;  /* True if the current field 
+						has been synchronized. */ 
 
 	unsigned long cap_ctl;
 	unsigned long dma_on;

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list