Re: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity

[Wolfram Sang <wsa@the-dreams.de>]

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

On Fri, May 05, 2017 at 02:08:31PM -0700, Florian Fainelli wrote:
> On 04/25/2017 05:56 PM, Florian Fainelli wrote:
> > Changes in v3:
> > 
> > - added check in usb_gadget_map_request_by_dev (Felipe), new patch
> > - improved commit message description (Clemens)
> > - added additiona checks for urb->setup_packet (Alan)
> > 
> > Changes in v2:
> > 
> > - moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma()
> 
> Is this version looking good now? Thanks!

So, it seems I am in a similar situation with the I2C subsystem right
now. I need to check the message buffers if they are DMA capable.

Because you have basically the same checks in 3 different places, and I
need something similar for I2C, I wondered about a generic place to put
these checks. Especially since we want future improvements to these
checks applied everywhere immediately. Here is a small diff on what I
have now:

===

dma-mapping(?): introduce helper to check for DMA capable addresses

Introduce a helper to check if an address is DMA capable. Such a check
is subtle, so it is good to have a centralized place for it.

Note: I am absolutely not sure if dma-mapping.h is a good place for such
a function. I just couldn't think of a better one for now.

Second note: I am not even sure the checks are complete (kmapped mem?).
I am not an MM expert. But that just strengthens the argument of having
on centralized place IMO.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 5dea98358c05c4..777a37b395ff19 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1584,7 +1584,7 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 					ret = -EAGAIN;
 				else
 					urb->transfer_flags |= URB_DMA_MAP_PAGE;
-			} else if (is_vmalloc_addr(urb->transfer_buffer)) {
+			} else if (!is_dma_capable_addr(urb->transfer_buffer)) {
 				WARN_ONCE(1, "transfer buffer not dma capable\n");
 				ret = -EAGAIN;
 			} else {
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 4f3eecedca2d7c..da8c1230302505 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -10,6 +10,8 @@
 #include <linux/scatterlist.h>
 #include <linux/kmemcheck.h>
 #include <linux/bug.h>
+#include <linux/mm.h>
+#include <linux/sched/task_stack.h>
 
 /**
  * List of possible attributes associated with a DMA mapping. The semantics
@@ -818,4 +820,10 @@ static inline int dma_mmap_wc(struct device *dev,
 #define dma_unmap_len_set(PTR, LEN_NAME, VAL)    do { } while (0)
 #endif
 
+/* only works in process context because of stack detection */
+static inline bool is_dma_capable_addr(void *addr)
+{
+	return !(is_vmalloc_or_module_addr(addr) ||
+		 object_is_on_stack(addr));
+}
 #endif
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 34a1c3e46ed725..47de0c0a700e7c 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -261,6 +261,7 @@ int is_vmalloc_or_module_addr(const void *x)
 #endif
 	return is_vmalloc_addr(x);
 }
+EXPORT_SYMBOL_GPL(is_vmalloc_or_module_addr);
 
 /*
  * Walk a vmap address to the struct page it maps.

===

The WIP branch containing also the I2C parts can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/i2c-core-dma

I think the whole series needs 1 or 2 days more before I send out an
RFC, but I thought I'll let you know about my idea already.

Thanks and kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]