Re: [PATCH] dma-mapping: Lift address space checks out of debug code
On Thu, Oct 03, 2019 at 02:38:43PM -0700, Kees Cook wrote: > > I think it would be reasonable to pull the is_vmalloc_addr() check inline, > > as that probably covers 90+% of badness (especially given vmapped stacks), > > and as you say should be reliably cheap everywhere. Callers are certainly > > expected to use dma_mapping_error() and handle failure, so refusing to do a > > bogus mapping operation should be OK API-wise - ultimately if a driver goes > > ahead and uses DMA_MAPPING_ERROR as an address anyway, that's not likely to > > be any *more* catastrophic than if it did the same with whatever nonsense > > virt_to_phys() of a vmalloc address had returned. > > What do you think about the object_is_on_stack() check? That does a > dereference through "current" to find the stack bounds... I can be persuaded about just the vmalloc check as people tend to get a lot of vmalloc alloctions without knowing these days. But what I'd really like to see is a new config option that enables relatively cheap checks without the full dma debugging infrastructure. That way you can turn those on at least for all development builds, and can easily benchmark having the checks vs not.
Re: [PATCH] dma-mapping: Lift address space checks out of debug code
On Fri, Oct 04, 2019 at 07:50:54PM +0100, Robin Murphy wrote: > On 03/10/2019 22:38, Kees Cook wrote: > > What do you think about the object_is_on_stack() check? That does a > > dereference through "current" to find the stack bounds... > > I guess it depends what the aim is - is it just to bail out of operations > which have near-zero chance of working correctly and every chance of going > catastrophically wrong, or to lay down strict argument checking for the API > in general? (for cache-coherent devices, or if the caller is careful to > ensure the appropriate alignment, DMA from a non-virtually-mapped stack can > be *technically* fine, it's just banned in general because those necessary > assumptions can be tricky to meet and aren't at all portable). Okay, then since the vmap check is both the cheapest and the most important to catch in the face of breaking everything, I'll move that in and we can keep USB's other checks separately. -- Kees Cook ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] dma-mapping: Lift address space checks out of debug code
On 03/10/2019 22:38, Kees Cook wrote:
On Thu, Oct 03, 2019 at 10:42:45AM +0100, Robin Murphy wrote:
On 03/10/2019 00:58, Kees Cook wrote:
On Wed, Oct 02, 2019 at 10:15:43PM +0100, Robin Murphy wrote:
Hi Kees,
On 2019-10-02 9:46 pm, Kees Cook wrote:
As we've seen from USB and other areas, we need to always do runtime
checks for DMA operating on memory regions that might be remapped. This
consolidates the (existing!) checks and makes them on by default. A
warning will be triggered for any drivers still using DMA on the stack
(as has been seen in a few recent reports).
Suggested-by: Laura Abbott
Signed-off-by: Kees Cook
---
include/linux/dma-debug.h | 8
include/linux/dma-mapping.h | 8 +++-
kernel/dma/debug.c | 16
3 files changed, 7 insertions(+), 25 deletions(-)
diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
index 4208f94d93f7..2af9765d9af7 100644
--- a/include/linux/dma-debug.h
+++ b/include/linux/dma-debug.h
@@ -18,9 +18,6 @@ struct bus_type;
extern void dma_debug_add_bus(struct bus_type *bus);
-extern void debug_dma_map_single(struct device *dev, const void *addr,
-unsigned long len);
-
extern void debug_dma_map_page(struct device *dev, struct page *page,
size_t offset, size_t size,
int direction, dma_addr_t dma_addr);
@@ -75,11 +72,6 @@ static inline void dma_debug_add_bus(struct bus_type *bus)
{
}
-static inline void debug_dma_map_single(struct device *dev, const void *addr,
- unsigned long len)
-{
-}
-
static inline void debug_dma_map_page(struct device *dev, struct page *page,
size_t offset, size_t size,
int direction, dma_addr_t dma_addr)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 4a1c4fca475a..2d6b8382eab1 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -583,7 +583,13 @@ static inline unsigned long dma_get_merge_boundary(struct
device *dev)
static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
size_t size, enum dma_data_direction dir, unsigned long attrs)
{
- debug_dma_map_single(dev, ptr, size);
+ /* DMA must never operate on stack or other remappable places. */
+ WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
This stands to absolutely cripple I/O performance on arm64, because every
valid call will end up going off and scanning the memblock list, which is
not something we want on a fastpath in non-debug configurations. We'd need a
much better solution to the "pfn_valid() vs. EFI no-map" problem before this
might be viable.
Ah! Interesting. I didn't realize this was fast-path (I don't know the
DMA code at all). I thought it was more of a "one time setup" before
actual DMA activity started.
That's strictly true, it's just that many workloads can involve tens of
thousands of "one time"s per second ;)
Overhead on the dma_map_* paths has shown to have a direct impact on
throughput in such situations, hence various optimisation effort in IOVA
allocation for IOMMU-based DMA ops, and the recent work to remove indirect
calls entirely for the common dma-direct/SWIOTLB cases.
Regardless, is_vmalloc_addr() is extremely light (a bounds check), and is the
most important part of this as far as catching stack-based DMA attempts.
I thought virt_addr_valid() was cheap too, but I see it's much heavier on
arm64.
I just went to compare what the existing USB check does, and it happens
immediately before its call to dma_map_single(). Both checks are simple
bounds checks, so it shouldn't be an issue:
if (is_vmalloc_addr(urb->setup_packet)) {
WARN_ONCE(1, "setup packet is not dma
capable\n");
return -EAGAIN;
} else if (object_is_on_stack(urb->setup_packet)) {
WARN_ONCE(1, "setup packet is on stack\n");
return -EAGAIN;
}
urb->setup_dma = dma_map_single(
hcd->self.sysdev,
urb->setup_packet,
sizeof(struct usb_ctrlrequest),
In the USB case, it'll actually refuse to do the operation. Should
dma_map_single() similarly fail? I could push these checks down into
dma_map_single(), which would be a no-change on behavior for USB and
gain the checks on all other callers...
I think it would be reasonable to pull the is_vmalloc_addr() check inline,
as that probably covers 90+% of badness (especially given vmapped stacks),
and as you say should be reliably cheap everywhere. Callers are certainly
expected to use dma_mapping_error() and
Re: [PATCH] dma-mapping: Lift address space checks out of debug code
On Thu, Oct 03, 2019 at 10:42:45AM +0100, Robin Murphy wrote:
> On 03/10/2019 00:58, Kees Cook wrote:
> > On Wed, Oct 02, 2019 at 10:15:43PM +0100, Robin Murphy wrote:
> > > Hi Kees,
> > >
> > > On 2019-10-02 9:46 pm, Kees Cook wrote:
> > > > As we've seen from USB and other areas, we need to always do runtime
> > > > checks for DMA operating on memory regions that might be remapped. This
> > > > consolidates the (existing!) checks and makes them on by default. A
> > > > warning will be triggered for any drivers still using DMA on the stack
> > > > (as has been seen in a few recent reports).
> > > >
> > > > Suggested-by: Laura Abbott
> > > > Signed-off-by: Kees Cook
> > > > ---
> > > >include/linux/dma-debug.h | 8
> > > >include/linux/dma-mapping.h | 8 +++-
> > > >kernel/dma/debug.c | 16
> > > >3 files changed, 7 insertions(+), 25 deletions(-)
> > > >
> > > > diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
> > > > index 4208f94d93f7..2af9765d9af7 100644
> > > > --- a/include/linux/dma-debug.h
> > > > +++ b/include/linux/dma-debug.h
> > > > @@ -18,9 +18,6 @@ struct bus_type;
> > > >extern void dma_debug_add_bus(struct bus_type *bus);
> > > > -extern void debug_dma_map_single(struct device *dev, const void *addr,
> > > > -unsigned long len);
> > > > -
> > > >extern void debug_dma_map_page(struct device *dev, struct page *page,
> > > >size_t offset, size_t size,
> > > >int direction, dma_addr_t dma_addr);
> > > > @@ -75,11 +72,6 @@ static inline void dma_debug_add_bus(struct bus_type
> > > > *bus)
> > > >{
> > > >}
> > > > -static inline void debug_dma_map_single(struct device *dev, const void
> > > > *addr,
> > > > - unsigned long len)
> > > > -{
> > > > -}
> > > > -
> > > >static inline void debug_dma_map_page(struct device *dev, struct
> > > > page *page,
> > > > size_t offset, size_t size,
> > > > int direction, dma_addr_t
> > > > dma_addr)
> > > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > > > index 4a1c4fca475a..2d6b8382eab1 100644
> > > > --- a/include/linux/dma-mapping.h
> > > > +++ b/include/linux/dma-mapping.h
> > > > @@ -583,7 +583,13 @@ static inline unsigned long
> > > > dma_get_merge_boundary(struct device *dev)
> > > >static inline dma_addr_t dma_map_single_attrs(struct device *dev,
> > > > void *ptr,
> > > > size_t size, enum dma_data_direction dir, unsigned long
> > > > attrs)
> > > >{
> > > > - debug_dma_map_single(dev, ptr, size);
> > > > + /* DMA must never operate on stack or other remappable places.
> > > > */
> > > > + WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
> > >
> > > This stands to absolutely cripple I/O performance on arm64, because every
> > > valid call will end up going off and scanning the memblock list, which is
> > > not something we want on a fastpath in non-debug configurations. We'd
> > > need a
> > > much better solution to the "pfn_valid() vs. EFI no-map" problem before
> > > this
> > > might be viable.
> >
> > Ah! Interesting. I didn't realize this was fast-path (I don't know the
> > DMA code at all). I thought it was more of a "one time setup" before
> > actual DMA activity started.
>
> That's strictly true, it's just that many workloads can involve tens of
> thousands of "one time"s per second ;)
>
> Overhead on the dma_map_* paths has shown to have a direct impact on
> throughput in such situations, hence various optimisation effort in IOVA
> allocation for IOMMU-based DMA ops, and the recent work to remove indirect
> calls entirely for the common dma-direct/SWIOTLB cases.
>
> > Regardless, is_vmalloc_addr() is extremely light (a bounds check), and is
> > the
> > most important part of this as far as catching stack-based DMA attempts.
> > I thought virt_addr_valid() was cheap too, but I see it's much heavier on
> > arm64.
> >
> > I just went to compare what the existing USB check does, and it happens
> > immediately before its call to dma_map_single(). Both checks are simple
> > bounds checks, so it shouldn't be an issue:
> >
> > if (is_vmalloc_addr(urb->setup_packet)) {
> > WARN_ONCE(1, "setup packet is not dma
> > capable\n");
> > return -EAGAIN;
> > } else if (object_is_on_stack(urb->setup_packet)) {
> > WARN_ONCE(1, "setup packet is on stack\n");
> > return -EAGAIN;
> > }
> >
> > urb->setup_dma = dma_map_single(
> > hcd->self.sysdev,
> > urb->setup_packet,
> >
Re: [PATCH] dma-mapping: Lift address space checks out of debug code
On 03/10/2019 00:58, Kees Cook wrote:
On Wed, Oct 02, 2019 at 10:15:43PM +0100, Robin Murphy wrote:
Hi Kees,
On 2019-10-02 9:46 pm, Kees Cook wrote:
As we've seen from USB and other areas, we need to always do runtime
checks for DMA operating on memory regions that might be remapped. This
consolidates the (existing!) checks and makes them on by default. A
warning will be triggered for any drivers still using DMA on the stack
(as has been seen in a few recent reports).
Suggested-by: Laura Abbott
Signed-off-by: Kees Cook
---
include/linux/dma-debug.h | 8
include/linux/dma-mapping.h | 8 +++-
kernel/dma/debug.c | 16
3 files changed, 7 insertions(+), 25 deletions(-)
diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
index 4208f94d93f7..2af9765d9af7 100644
--- a/include/linux/dma-debug.h
+++ b/include/linux/dma-debug.h
@@ -18,9 +18,6 @@ struct bus_type;
extern void dma_debug_add_bus(struct bus_type *bus);
-extern void debug_dma_map_single(struct device *dev, const void *addr,
-unsigned long len);
-
extern void debug_dma_map_page(struct device *dev, struct page *page,
size_t offset, size_t size,
int direction, dma_addr_t dma_addr);
@@ -75,11 +72,6 @@ static inline void dma_debug_add_bus(struct bus_type *bus)
{
}
-static inline void debug_dma_map_single(struct device *dev, const void *addr,
- unsigned long len)
-{
-}
-
static inline void debug_dma_map_page(struct device *dev, struct page *page,
size_t offset, size_t size,
int direction, dma_addr_t dma_addr)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 4a1c4fca475a..2d6b8382eab1 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -583,7 +583,13 @@ static inline unsigned long dma_get_merge_boundary(struct
device *dev)
static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
size_t size, enum dma_data_direction dir, unsigned long attrs)
{
- debug_dma_map_single(dev, ptr, size);
+ /* DMA must never operate on stack or other remappable places. */
+ WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
This stands to absolutely cripple I/O performance on arm64, because every
valid call will end up going off and scanning the memblock list, which is
not something we want on a fastpath in non-debug configurations. We'd need a
much better solution to the "pfn_valid() vs. EFI no-map" problem before this
might be viable.
Ah! Interesting. I didn't realize this was fast-path (I don't know the
DMA code at all). I thought it was more of a "one time setup" before
actual DMA activity started.
That's strictly true, it's just that many workloads can involve tens of
thousands of "one time"s per second ;)
Overhead on the dma_map_* paths has shown to have a direct impact on
throughput in such situations, hence various optimisation effort in IOVA
allocation for IOMMU-based DMA ops, and the recent work to remove
indirect calls entirely for the common dma-direct/SWIOTLB cases.
Regardless, is_vmalloc_addr() is extremely light (a bounds check), and is the
most important part of this as far as catching stack-based DMA attempts.
I thought virt_addr_valid() was cheap too, but I see it's much heavier on
arm64.
I just went to compare what the existing USB check does, and it happens
immediately before its call to dma_map_single(). Both checks are simple
bounds checks, so it shouldn't be an issue:
if (is_vmalloc_addr(urb->setup_packet)) {
WARN_ONCE(1, "setup packet is not dma
capable\n");
return -EAGAIN;
} else if (object_is_on_stack(urb->setup_packet)) {
WARN_ONCE(1, "setup packet is on stack\n");
return -EAGAIN;
}
urb->setup_dma = dma_map_single(
hcd->self.sysdev,
urb->setup_packet,
sizeof(struct usb_ctrlrequest),
In the USB case, it'll actually refuse to do the operation. Should
dma_map_single() similarly fail? I could push these checks down into
dma_map_single(), which would be a no-change on behavior for USB and
gain the checks on all other callers...
I think it would be reasonable to pull the is_vmalloc_addr() check
inline, as that probably covers 90+% of badness (especially given
vmapped stacks), and as you say should be reliably cheap everywhere.
Callers are certainly expected to use dma_mapping_error() and handle
failure, so refusing to do a bogus mapping operation should be OK
API-wise - ultimately if a
Re: [PATCH] dma-mapping: Lift address space checks out of debug code
Hi Kees,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[cannot apply to v5.4-rc1 next-20191002]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Kees-Cook/dma-mapping-Lift-address-space-checks-out-of-debug-code/20191003-060622
config: i386-randconfig-c004-201939 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All errors (new ones prefixed by >>):
In file included from arch/x86/include/asm/bug.h:83:0,
from include/linux/bug.h:5,
from include/linux/debug_locks.h:7,
from include/linux/lockdep.h:28,
from include/linux/spinlock_types.h:18,
from include/linux/mutex.h:16,
from include/linux/kernfs.h:12,
from include/linux/sysfs.h:16,
from include/linux/kobject.h:20,
from include/linux/of.h:17,
from include/linux/irqdomain.h:35,
from include/linux/acpi.h:13,
from drivers/gpu/drm/i915/i915_drv.c:30:
include/linux/dma-mapping.h: In function 'dma_map_single_attrs':
>> include/linux/dma-mapping.h:588:3: error: format '%lu' expects argument of
>> type 'long unsigned int', but argument 4 has type 'size_t {aka unsigned
>> int}' [-Werror=format=]
"%s %s: driver maps %lu bytes from %s area\n",
^
include/asm-generic/bug.h:92:17: note: in definition of macro '__WARN_printf'
__warn_printk(arg); \
^~~
include/asm-generic/bug.h:155:3: note: in expansion of macro 'WARN'
WARN(1, format);\
^~~~
include/linux/dma-mapping.h:587:2: note: in expansion of macro 'WARN_ONCE'
WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
^
cc1: all warnings being treated as errors
vim +588 include/linux/dma-mapping.h
582
583 static inline dma_addr_t dma_map_single_attrs(struct device *dev, void
*ptr,
584 size_t size, enum dma_data_direction dir, unsigned long
attrs)
585 {
586 /* DMA must never operate on stack or other remappable places.
*/
587 WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
> 588 "%s %s: driver maps %lu bytes from %s area\n",
589 dev ? dev_driver_string(dev) : "unknown driver",
590 dev ? dev_name(dev) : "unknown device", size,
591 is_vmalloc_addr(ptr) ? "vmalloc" : "invalid");
592
593 return dma_map_page_attrs(dev, virt_to_page(ptr),
offset_in_page(ptr),
594 size, dir, attrs);
595 }
596
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH] dma-mapping: Lift address space checks out of debug code
On Wed, Oct 02, 2019 at 04:58:39PM -0700, Kees Cook wrote: > In the USB case, it'll actually refuse to do the operation. Should > dma_map_single() similarly fail? I could push these checks down into > dma_map_single(), which would be a no-change on behavior for USB and > gain the checks on all other callers... Which begs the question: are all callers actually checking the result of dma_map_single(). Many are paired with dma_mapping_error(), but lots more aren't... -- Kees Cook
Re: [PATCH] dma-mapping: Lift address space checks out of debug code
On Wed, Oct 02, 2019 at 10:15:43PM +0100, Robin Murphy wrote:
> Hi Kees,
>
> On 2019-10-02 9:46 pm, Kees Cook wrote:
> > As we've seen from USB and other areas, we need to always do runtime
> > checks for DMA operating on memory regions that might be remapped. This
> > consolidates the (existing!) checks and makes them on by default. A
> > warning will be triggered for any drivers still using DMA on the stack
> > (as has been seen in a few recent reports).
> >
> > Suggested-by: Laura Abbott
> > Signed-off-by: Kees Cook
> > ---
> > include/linux/dma-debug.h | 8
> > include/linux/dma-mapping.h | 8 +++-
> > kernel/dma/debug.c | 16
> > 3 files changed, 7 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
> > index 4208f94d93f7..2af9765d9af7 100644
> > --- a/include/linux/dma-debug.h
> > +++ b/include/linux/dma-debug.h
> > @@ -18,9 +18,6 @@ struct bus_type;
> > extern void dma_debug_add_bus(struct bus_type *bus);
> > -extern void debug_dma_map_single(struct device *dev, const void *addr,
> > -unsigned long len);
> > -
> > extern void debug_dma_map_page(struct device *dev, struct page *page,
> >size_t offset, size_t size,
> >int direction, dma_addr_t dma_addr);
> > @@ -75,11 +72,6 @@ static inline void dma_debug_add_bus(struct bus_type
> > *bus)
> > {
> > }
> > -static inline void debug_dma_map_single(struct device *dev, const void
> > *addr,
> > - unsigned long len)
> > -{
> > -}
> > -
> > static inline void debug_dma_map_page(struct device *dev, struct page
> > *page,
> > size_t offset, size_t size,
> > int direction, dma_addr_t dma_addr)
> > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > index 4a1c4fca475a..2d6b8382eab1 100644
> > --- a/include/linux/dma-mapping.h
> > +++ b/include/linux/dma-mapping.h
> > @@ -583,7 +583,13 @@ static inline unsigned long
> > dma_get_merge_boundary(struct device *dev)
> > static inline dma_addr_t dma_map_single_attrs(struct device *dev, void
> > *ptr,
> > size_t size, enum dma_data_direction dir, unsigned long attrs)
> > {
> > - debug_dma_map_single(dev, ptr, size);
> > + /* DMA must never operate on stack or other remappable places. */
> > + WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
>
> This stands to absolutely cripple I/O performance on arm64, because every
> valid call will end up going off and scanning the memblock list, which is
> not something we want on a fastpath in non-debug configurations. We'd need a
> much better solution to the "pfn_valid() vs. EFI no-map" problem before this
> might be viable.
Ah! Interesting. I didn't realize this was fast-path (I don't know the
DMA code at all). I thought it was more of a "one time setup" before
actual DMA activity started.
Regardless, is_vmalloc_addr() is extremely light (a bounds check), and is the
most important part of this as far as catching stack-based DMA attempts.
I thought virt_addr_valid() was cheap too, but I see it's much heavier on
arm64.
I just went to compare what the existing USB check does, and it happens
immediately before its call to dma_map_single(). Both checks are simple
bounds checks, so it shouldn't be an issue:
if (is_vmalloc_addr(urb->setup_packet)) {
WARN_ONCE(1, "setup packet is not dma
capable\n");
return -EAGAIN;
} else if (object_is_on_stack(urb->setup_packet)) {
WARN_ONCE(1, "setup packet is on stack\n");
return -EAGAIN;
}
urb->setup_dma = dma_map_single(
hcd->self.sysdev,
urb->setup_packet,
sizeof(struct usb_ctrlrequest),
In the USB case, it'll actually refuse to do the operation. Should
dma_map_single() similarly fail? I could push these checks down into
dma_map_single(), which would be a no-change on behavior for USB and
gain the checks on all other callers...
--
Kees Cook
Re: [PATCH] dma-mapping: Lift address space checks out of debug code
Hi Kees,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.4-rc1 next-20191002]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Kees-Cook/dma-mapping-Lift-address-space-checks-out-of-debug-code/20191003-060622
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:15:0,
from include/linux/list.h:9,
from include/linux/module.h:9,
from init/do_mounts.c:2:
include/linux/dma-mapping.h: In function 'dma_map_single_attrs':
>> include/linux/dma-mapping.h:588:3: warning: format '%lu' expects argument of
>> type 'long unsigned int', but argument 4 has type 'size_t {aka unsigned
>> int}' [-Wformat=]
"%s %s: driver maps %lu bytes from %s area\n",
^
include/linux/printk.h:137:10: note: in definition of macro 'no_printk'
printk(fmt, ##__VA_ARGS__); \
^~~
include/asm-generic/bug.h:196:41: note: in expansion of macro 'WARN'
#define WARN_ONCE(condition, format...) WARN(condition, format)
^~~~
>> include/linux/dma-mapping.h:587:2: note: in expansion of macro 'WARN_ONCE'
WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
^
vim +588 include/linux/dma-mapping.h
582
583 static inline dma_addr_t dma_map_single_attrs(struct device *dev, void
*ptr,
584 size_t size, enum dma_data_direction dir, unsigned long
attrs)
585 {
586 /* DMA must never operate on stack or other remappable places.
*/
> 587 WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
> 588 "%s %s: driver maps %lu bytes from %s area\n",
589 dev ? dev_driver_string(dev) : "unknown driver",
590 dev ? dev_name(dev) : "unknown device", size,
591 is_vmalloc_addr(ptr) ? "vmalloc" : "invalid");
592
593 return dma_map_page_attrs(dev, virt_to_page(ptr),
offset_in_page(ptr),
594 size, dir, attrs);
595 }
596
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] dma-mapping: Lift address space checks out of debug code
Hi Kees,
On 2019-10-02 9:46 pm, Kees Cook wrote:
As we've seen from USB and other areas, we need to always do runtime
checks for DMA operating on memory regions that might be remapped. This
consolidates the (existing!) checks and makes them on by default. A
warning will be triggered for any drivers still using DMA on the stack
(as has been seen in a few recent reports).
Suggested-by: Laura Abbott
Signed-off-by: Kees Cook
---
include/linux/dma-debug.h | 8
include/linux/dma-mapping.h | 8 +++-
kernel/dma/debug.c | 16
3 files changed, 7 insertions(+), 25 deletions(-)
diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
index 4208f94d93f7..2af9765d9af7 100644
--- a/include/linux/dma-debug.h
+++ b/include/linux/dma-debug.h
@@ -18,9 +18,6 @@ struct bus_type;
extern void dma_debug_add_bus(struct bus_type *bus);
-extern void debug_dma_map_single(struct device *dev, const void *addr,
-unsigned long len);
-
extern void debug_dma_map_page(struct device *dev, struct page *page,
size_t offset, size_t size,
int direction, dma_addr_t dma_addr);
@@ -75,11 +72,6 @@ static inline void dma_debug_add_bus(struct bus_type *bus)
{
}
-static inline void debug_dma_map_single(struct device *dev, const void *addr,
- unsigned long len)
-{
-}
-
static inline void debug_dma_map_page(struct device *dev, struct page *page,
size_t offset, size_t size,
int direction, dma_addr_t dma_addr)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 4a1c4fca475a..2d6b8382eab1 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -583,7 +583,13 @@ static inline unsigned long dma_get_merge_boundary(struct
device *dev)
static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
size_t size, enum dma_data_direction dir, unsigned long attrs)
{
- debug_dma_map_single(dev, ptr, size);
+ /* DMA must never operate on stack or other remappable places. */
+ WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
This stands to absolutely cripple I/O performance on arm64, because
every valid call will end up going off and scanning the memblock list,
which is not something we want on a fastpath in non-debug
configurations. We'd need a much better solution to the "pfn_valid() vs.
EFI no-map" problem before this might be viable.
Robin.
+ "%s %s: driver maps %lu bytes from %s area\n",
+ dev ? dev_driver_string(dev) : "unknown driver",
+ dev ? dev_name(dev) : "unknown device", size,
+ is_vmalloc_addr(ptr) ? "vmalloc" : "invalid");
+
return dma_map_page_attrs(dev, virt_to_page(ptr), offset_in_page(ptr),
size, dir, attrs);
}
diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
index 099002d84f46..aa1e6a1990b2 100644
--- a/kernel/dma/debug.c
+++ b/kernel/dma/debug.c
@@ -1232,22 +1232,6 @@ static void check_sg_segment(struct device *dev, struct
scatterlist *sg)
#endif
}
-void debug_dma_map_single(struct device *dev, const void *addr,
- unsigned long len)
-{
- if (unlikely(dma_debug_disabled()))
- return;
-
- if (!virt_addr_valid(addr))
- err_printk(dev, NULL, "device driver maps memory from invalid area
[addr=%p] [len=%lu]\n",
- addr, len);
-
- if (is_vmalloc_addr(addr))
- err_printk(dev, NULL, "device driver maps memory from vmalloc area
[addr=%p] [len=%lu]\n",
- addr, len);
-}
-EXPORT_SYMBOL(debug_dma_map_single);
-
void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
size_t size, int direction, dma_addr_t dma_addr)
{
[PATCH] dma-mapping: Lift address space checks out of debug code
As we've seen from USB and other areas, we need to always do runtime
checks for DMA operating on memory regions that might be remapped. This
consolidates the (existing!) checks and makes them on by default. A
warning will be triggered for any drivers still using DMA on the stack
(as has been seen in a few recent reports).
Suggested-by: Laura Abbott
Signed-off-by: Kees Cook
---
include/linux/dma-debug.h | 8
include/linux/dma-mapping.h | 8 +++-
kernel/dma/debug.c | 16
3 files changed, 7 insertions(+), 25 deletions(-)
diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
index 4208f94d93f7..2af9765d9af7 100644
--- a/include/linux/dma-debug.h
+++ b/include/linux/dma-debug.h
@@ -18,9 +18,6 @@ struct bus_type;
extern void dma_debug_add_bus(struct bus_type *bus);
-extern void debug_dma_map_single(struct device *dev, const void *addr,
-unsigned long len);
-
extern void debug_dma_map_page(struct device *dev, struct page *page,
size_t offset, size_t size,
int direction, dma_addr_t dma_addr);
@@ -75,11 +72,6 @@ static inline void dma_debug_add_bus(struct bus_type *bus)
{
}
-static inline void debug_dma_map_single(struct device *dev, const void *addr,
- unsigned long len)
-{
-}
-
static inline void debug_dma_map_page(struct device *dev, struct page *page,
size_t offset, size_t size,
int direction, dma_addr_t dma_addr)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 4a1c4fca475a..2d6b8382eab1 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -583,7 +583,13 @@ static inline unsigned long dma_get_merge_boundary(struct
device *dev)
static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
size_t size, enum dma_data_direction dir, unsigned long attrs)
{
- debug_dma_map_single(dev, ptr, size);
+ /* DMA must never operate on stack or other remappable places. */
+ WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
+ "%s %s: driver maps %lu bytes from %s area\n",
+ dev ? dev_driver_string(dev) : "unknown driver",
+ dev ? dev_name(dev) : "unknown device", size,
+ is_vmalloc_addr(ptr) ? "vmalloc" : "invalid");
+
return dma_map_page_attrs(dev, virt_to_page(ptr), offset_in_page(ptr),
size, dir, attrs);
}
diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
index 099002d84f46..aa1e6a1990b2 100644
--- a/kernel/dma/debug.c
+++ b/kernel/dma/debug.c
@@ -1232,22 +1232,6 @@ static void check_sg_segment(struct device *dev, struct
scatterlist *sg)
#endif
}
-void debug_dma_map_single(struct device *dev, const void *addr,
- unsigned long len)
-{
- if (unlikely(dma_debug_disabled()))
- return;
-
- if (!virt_addr_valid(addr))
- err_printk(dev, NULL, "device driver maps memory from invalid
area [addr=%p] [len=%lu]\n",
- addr, len);
-
- if (is_vmalloc_addr(addr))
- err_printk(dev, NULL, "device driver maps memory from vmalloc
area [addr=%p] [len=%lu]\n",
- addr, len);
-}
-EXPORT_SYMBOL(debug_dma_map_single);
-
void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
size_t size, int direction, dma_addr_t dma_addr)
{
--
2.17.1
--
Kees Cook
