From 4e9e92003529e5c7bb11281f7c2c9b3fe8858403 Mon Sep 17 00:00:00 2001 From: Pete Zaitcev Date: Thu, 11 Jun 2009 08:53:20 -0600 Subject: USB: usbmon: end ugly tricks with DMA peeking This patch fixes crashes when usbmon attempts to access GART aperture. The old code attempted to take a bus address and convert it into a virtual address, which clearly was impossible on systems with actual IOMMUs. Let us not persist in this foolishness, and use transfer_buffer in all cases instead. I think downsides are negligible. The ones I see are: - A driver may pass an address of one buffer down as transfer_buffer, and entirely different entity mapped for DMA, resulting in misleading output of usbmon. Note, however, that PIO based controllers would do transfer the same data that usbmon sees here. - Out of tree drivers may crash usbmon if they store garbage in transfer_buffer. I inspected the in-tree drivers, and clarified the documentation in comments. - Drivers that use get_user_pages will not be possible to monitor. I only found one driver with this problem (drivers/staging/rspiusb). - Same happens with with usb_storage transferring from highmem, but it works fine on 64-bit systems, so I think it's not a concern. At least we don't crash anymore. Why didn't we do this in 2.6.10? That's because back in those days it was popular not to fill in transfer_buffer, so almost all traffic would be invisible (e.g. all of HID was like that). But now, the tree is almost 100% PIO friendly, so we can do the right thing at last. Signed-off-by: Pete Zaitcev Signed-off-by: Greg Kroah-Hartman --- drivers/usb/mon/Makefile | 2 +- drivers/usb/mon/mon_bin.c | 12 +----- drivers/usb/mon/mon_dma.c | 95 ---------------------------------------------- drivers/usb/mon/mon_main.c | 1 - drivers/usb/mon/mon_text.c | 14 ------- drivers/usb/mon/usb_mon.h | 14 ------- 6 files changed, 2 insertions(+), 136 deletions(-) delete mode 100644 drivers/usb/mon/mon_dma.c (limited to 'drivers/usb') diff --git a/drivers/usb/mon/Makefile b/drivers/usb/mon/Makefile index c6516b5..384b198 100644 --- a/drivers/usb/mon/Makefile +++ b/drivers/usb/mon/Makefile @@ -2,6 +2,6 @@ # Makefile for USB monitor # -usbmon-objs := mon_main.o mon_stat.o mon_text.o mon_bin.o mon_dma.o +usbmon-objs := mon_main.o mon_stat.o mon_text.o mon_bin.o obj-$(CONFIG_USB_MON) += usbmon.o diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c index 0f7a30b..dfdc43e 100644 --- a/drivers/usb/mon/mon_bin.c +++ b/drivers/usb/mon/mon_bin.c @@ -220,9 +220,8 @@ static void mon_free_buff(struct mon_pgmap *map, int npages); /* * This is a "chunked memcpy". It does not manipulate any counters. - * But it returns the new offset for repeated application. */ -unsigned int mon_copy_to_buff(const struct mon_reader_bin *this, +static void mon_copy_to_buff(const struct mon_reader_bin *this, unsigned int off, const unsigned char *from, unsigned int length) { unsigned int step_len; @@ -247,7 +246,6 @@ unsigned int mon_copy_to_buff(const struct mon_reader_bin *this, from += step_len; length -= step_len; } - return off; } /* @@ -400,15 +398,8 @@ static char mon_bin_get_data(const struct mon_reader_bin *rp, unsigned int offset, struct urb *urb, unsigned int length) { - if (urb->dev->bus->uses_dma && - (urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) { - mon_dmapeek_vec(rp, offset, urb->transfer_dma, length); - return 0; - } - if (urb->transfer_buffer == NULL) return 'Z'; - mon_copy_to_buff(rp, offset, urb->transfer_buffer, length); return 0; } @@ -635,7 +626,6 @@ static int mon_bin_open(struct inode *inode, struct file *file) spin_lock_init(&rp->b_lock); init_waitqueue_head(&rp->b_wait); mutex_init(&rp->fetch_lock); - rp->b_size = BUFF_DFL; size = sizeof(struct mon_pgmap) * (rp->b_size/CHUNK_SIZE); diff --git a/drivers/usb/mon/mon_dma.c b/drivers/usb/mon/mon_dma.c deleted file mode 100644 index 140cc80..0000000 --- a/drivers/usb/mon/mon_dma.c +++ /dev/null @@ -1,95 +0,0 @@ -/* - * The USB Monitor, inspired by Dave Harding's USBMon. - * - * mon_dma.c: Library which snoops on DMA areas. - * - * Copyright (C) 2005 Pete Zaitcev (zaitcev@redhat.com) - */ -#include -#include -#include -#include - -#include /* Only needed for declarations in usb_mon.h */ -#include "usb_mon.h" - -/* - * PC-compatibles, are, fortunately, sufficiently cache-coherent for this. - */ -#if defined(__i386__) || defined(__x86_64__) /* CONFIG_ARCH_I386 doesn't exit */ -#define MON_HAS_UNMAP 1 - -#define phys_to_page(phys) pfn_to_page((phys) >> PAGE_SHIFT) - -char mon_dmapeek(unsigned char *dst, dma_addr_t dma_addr, int len) -{ - struct page *pg; - unsigned long flags; - unsigned char *map; - unsigned char *ptr; - - /* - * On i386, a DMA handle is the "physical" address of a page. - * In other words, the bus address is equal to physical address. - * There is no IOMMU. - */ - pg = phys_to_page(dma_addr); - - /* - * We are called from hardware IRQs in case of callbacks. - * But we can be called from softirq or process context in case - * of submissions. In such case, we need to protect KM_IRQ0. - */ - local_irq_save(flags); - map = kmap_atomic(pg, KM_IRQ0); - ptr = map + (dma_addr & (PAGE_SIZE-1)); - memcpy(dst, ptr, len); - kunmap_atomic(map, KM_IRQ0); - local_irq_restore(flags); - return 0; -} - -void mon_dmapeek_vec(const struct mon_reader_bin *rp, - unsigned int offset, dma_addr_t dma_addr, unsigned int length) -{ - unsigned long flags; - unsigned int step_len; - struct page *pg; - unsigned char *map; - unsigned long page_off, page_len; - - local_irq_save(flags); - while (length) { - /* compute number of bytes we are going to copy in this page */ - step_len = length; - page_off = dma_addr & (PAGE_SIZE-1); - page_len = PAGE_SIZE - page_off; - if (page_len < step_len) - step_len = page_len; - - /* copy data and advance pointers */ - pg = phys_to_page(dma_addr); - map = kmap_atomic(pg, KM_IRQ0); - offset = mon_copy_to_buff(rp, offset, map + page_off, step_len); - kunmap_atomic(map, KM_IRQ0); - dma_addr += step_len; - length -= step_len; - } - local_irq_restore(flags); -} - -#endif /* __i386__ */ - -#ifndef MON_HAS_UNMAP -char mon_dmapeek(unsigned char *dst, dma_addr_t dma_addr, int len) -{ - return 'D'; -} - -void mon_dmapeek_vec(const struct mon_reader_bin *rp, - unsigned int offset, dma_addr_t dma_addr, unsigned int length) -{ - ; -} - -#endif /* MON_HAS_UNMAP */ diff --git a/drivers/usb/mon/mon_main.c b/drivers/usb/mon/mon_main.c index 5e0ab42..e0c2db3 100644 --- a/drivers/usb/mon/mon_main.c +++ b/drivers/usb/mon/mon_main.c @@ -361,7 +361,6 @@ static int __init mon_init(void) } // MOD_INC_USE_COUNT(which_module?); - mutex_lock(&usb_bus_list_lock); list_for_each_entry (ubus, &usb_bus_list, bus_list) { mon_bus_init(ubus); diff --git a/drivers/usb/mon/mon_text.c b/drivers/usb/mon/mon_text.c index a7eb4c9..9f1a922 100644 --- a/drivers/usb/mon/mon_text.c +++ b/drivers/usb/mon/mon_text.c @@ -150,20 +150,6 @@ static inline char mon_text_get_data(struct mon_event_text *ep, struct urb *urb, return '>'; } - /* - * The check to see if it's safe to poke at data has an enormous - * number of corner cases, but it seems that the following is - * more or less safe. - * - * We do not even try to look at transfer_buffer, because it can - * contain non-NULL garbage in case the upper level promised to - * set DMA for the HCD. - */ - if (urb->dev->bus->uses_dma && - (urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) { - return mon_dmapeek(ep->data, urb->transfer_dma, len); - } - if (urb->transfer_buffer == NULL) return 'Z'; /* '0' would be not as pretty. */ diff --git a/drivers/usb/mon/usb_mon.h b/drivers/usb/mon/usb_mon.h index f5d84ff..df9a4df 100644 --- a/drivers/usb/mon/usb_mon.h +++ b/drivers/usb/mon/usb_mon.h @@ -65,20 +65,6 @@ int __init mon_bin_init(void); void mon_bin_exit(void); /* - * DMA interface. - * - * XXX The vectored side needs a serious re-thinking. Abstracting vectors, - * like in Paolo's original patch, produces a double pkmap. We need an idea. -*/ -extern char mon_dmapeek(unsigned char *dst, dma_addr_t dma_addr, int len); - -struct mon_reader_bin; -extern void mon_dmapeek_vec(const struct mon_reader_bin *rp, - unsigned int offset, dma_addr_t dma_addr, unsigned int len); -extern unsigned int mon_copy_to_buff(const struct mon_reader_bin *rp, - unsigned int offset, const unsigned char *from, unsigned int len); - -/* */ extern struct mutex mon_lock; -- cgit v1.1