From: Quanyang Wang <[email protected]>
There are two problems with the current code of memory_intersects:
First, it doesn't check whether the region (begin, end) falls inside
the region (virt, vend), that is (virt < begin && vend > end).
The second problem is if vend is equal to begin, it will return true
but this is wrong since vend (virt + size) is not the last address of
the memory region but (virt + size -1) is. The wrong determination will
trigger the misreporting when the function check_for_illegal_area calls
memory_intersects to check if the dma region intersects with stext region.
The misreporting is as below (stext is at 0x80100000):
WARNING: CPU: 0 PID: 77 at kernel/dma/debug.c:1073 check_for_illegal_area+0x130/0x168
DMA-API: chipidea-usb2 e0002000.usb: device driver maps memory from kernel text or rodata [addr=800f0000] [len=65536]
Modules linked in:
CPU: 1 PID: 77 Comm: usb-storage Not tainted 5.19.0-yocto-standard #5
Hardware name: Xilinx Zynq Platform
unwind_backtrace from show_stack+0x18/0x1c
show_stack from dump_stack_lvl+0x58/0x70
dump_stack_lvl from __warn+0xb0/0x198
__warn from warn_slowpath_fmt+0x80/0xb4
warn_slowpath_fmt from check_for_illegal_area+0x130/0x168
check_for_illegal_area from debug_dma_map_sg+0x94/0x368
debug_dma_map_sg from __dma_map_sg_attrs+0x114/0x128
__dma_map_sg_attrs from dma_map_sg_attrs+0x18/0x24
dma_map_sg_attrs from usb_hcd_map_urb_for_dma+0x250/0x3b4
usb_hcd_map_urb_for_dma from usb_hcd_submit_urb+0x194/0x214
usb_hcd_submit_urb from usb_sg_wait+0xa4/0x118
usb_sg_wait from usb_stor_bulk_transfer_sglist+0xa0/0xec
usb_stor_bulk_transfer_sglist from usb_stor_bulk_srb+0x38/0x70
usb_stor_bulk_srb from usb_stor_Bulk_transport+0x150/0x360
usb_stor_Bulk_transport from usb_stor_invoke_transport+0x38/0x440
usb_stor_invoke_transport from usb_stor_control_thread+0x1e0/0x238
usb_stor_control_thread from kthread+0xf8/0x104
kthread from ret_from_fork+0x14/0x2c
Refactor memory_intersects to fix the two problems above.
Fixes: 979559362516 ("asm/sections: add helpers to check for section data")
Signed-off-by: Quanyang Wang <[email protected]>
---
V1 ---> V2:
Add the consideration of the condition that one falls inside another
which is noticed by Ard.
---
include/asm-generic/sections.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index d0f7bdd2fdf23..c51b3e7925cdf 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -110,7 +110,10 @@ static inline bool memory_intersects(void *begin, void *end, void *virt,
{
void *vend = virt + size;
- return (virt >= begin && virt < end) || (vend >= begin && vend < end);
+ if (virt < end && vend > begin)
+ return true;
+
+ return false;
}
/**
--
2.36.1
On Fri, 19 Aug 2022 at 10:12, <[email protected]> wrote:
>
> From: Quanyang Wang <[email protected]>
>
> There are two problems with the current code of memory_intersects:
>
> First, it doesn't check whether the region (begin, end) falls inside
> the region (virt, vend), that is (virt < begin && vend > end).
>
> The second problem is if vend is equal to begin, it will return true
> but this is wrong since vend (virt + size) is not the last address of
> the memory region but (virt + size -1) is. The wrong determination will
> trigger the misreporting when the function check_for_illegal_area calls
> memory_intersects to check if the dma region intersects with stext region.
>
> The misreporting is as below (stext is at 0x80100000):
> WARNING: CPU: 0 PID: 77 at kernel/dma/debug.c:1073 check_for_illegal_area+0x130/0x168
> DMA-API: chipidea-usb2 e0002000.usb: device driver maps memory from kernel text or rodata [addr=800f0000] [len=65536]
> Modules linked in:
> CPU: 1 PID: 77 Comm: usb-storage Not tainted 5.19.0-yocto-standard #5
> Hardware name: Xilinx Zynq Platform
> unwind_backtrace from show_stack+0x18/0x1c
> show_stack from dump_stack_lvl+0x58/0x70
> dump_stack_lvl from __warn+0xb0/0x198
> __warn from warn_slowpath_fmt+0x80/0xb4
> warn_slowpath_fmt from check_for_illegal_area+0x130/0x168
> check_for_illegal_area from debug_dma_map_sg+0x94/0x368
> debug_dma_map_sg from __dma_map_sg_attrs+0x114/0x128
> __dma_map_sg_attrs from dma_map_sg_attrs+0x18/0x24
> dma_map_sg_attrs from usb_hcd_map_urb_for_dma+0x250/0x3b4
> usb_hcd_map_urb_for_dma from usb_hcd_submit_urb+0x194/0x214
> usb_hcd_submit_urb from usb_sg_wait+0xa4/0x118
> usb_sg_wait from usb_stor_bulk_transfer_sglist+0xa0/0xec
> usb_stor_bulk_transfer_sglist from usb_stor_bulk_srb+0x38/0x70
> usb_stor_bulk_srb from usb_stor_Bulk_transport+0x150/0x360
> usb_stor_Bulk_transport from usb_stor_invoke_transport+0x38/0x440
> usb_stor_invoke_transport from usb_stor_control_thread+0x1e0/0x238
> usb_stor_control_thread from kthread+0xf8/0x104
> kthread from ret_from_fork+0x14/0x2c
>
> Refactor memory_intersects to fix the two problems above.
>
> Fixes: 979559362516 ("asm/sections: add helpers to check for section data")
> Signed-off-by: Quanyang Wang <[email protected]>
> ---
> V1 ---> V2:
> Add the consideration of the condition that one falls inside another
> which is noticed by Ard.
> ---
> include/asm-generic/sections.h | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index d0f7bdd2fdf23..c51b3e7925cdf 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -110,7 +110,10 @@ static inline bool memory_intersects(void *begin, void *end, void *virt,
> {
> void *vend = virt + size;
>
> - return (virt >= begin && virt < end) || (vend >= begin && vend < end);
> + if (virt < end && vend > begin)
> + return true;
> +
> + return false;
> }
>
Reviewed-by: Ard Biesheuvel <[email protected]>
On Fri, 19 Aug 2022 16:11:45 +0800 [email protected] wrote:
> From: Quanyang Wang <[email protected]>
>
> There are two problems with the current code of memory_intersects:
>
> First, it doesn't check whether the region (begin, end) falls inside
> the region (virt, vend), that is (virt < begin && vend > end).
>
> The second problem is if vend is equal to begin, it will return true
> but this is wrong since vend (virt + size) is not the last address of
> the memory region but (virt + size -1) is. The wrong determination will
> trigger the misreporting when the function check_for_illegal_area calls
> memory_intersects to check if the dma region intersects with stext region.
>
> The misreporting is as below (stext is at 0x80100000):
> WARNING: CPU: 0 PID: 77 at kernel/dma/debug.c:1073 check_for_illegal_area+0x130/0x168
> DMA-API: chipidea-usb2 e0002000.usb: device driver maps memory from kernel text or rodata [addr=800f0000] [len=65536]
> Modules linked in:
> CPU: 1 PID: 77 Comm: usb-storage Not tainted 5.19.0-yocto-standard #5
> Hardware name: Xilinx Zynq Platform
> unwind_backtrace from show_stack+0x18/0x1c
> show_stack from dump_stack_lvl+0x58/0x70
> dump_stack_lvl from __warn+0xb0/0x198
> __warn from warn_slowpath_fmt+0x80/0xb4
> warn_slowpath_fmt from check_for_illegal_area+0x130/0x168
> check_for_illegal_area from debug_dma_map_sg+0x94/0x368
> debug_dma_map_sg from __dma_map_sg_attrs+0x114/0x128
> __dma_map_sg_attrs from dma_map_sg_attrs+0x18/0x24
> dma_map_sg_attrs from usb_hcd_map_urb_for_dma+0x250/0x3b4
> usb_hcd_map_urb_for_dma from usb_hcd_submit_urb+0x194/0x214
> usb_hcd_submit_urb from usb_sg_wait+0xa4/0x118
> usb_sg_wait from usb_stor_bulk_transfer_sglist+0xa0/0xec
> usb_stor_bulk_transfer_sglist from usb_stor_bulk_srb+0x38/0x70
> usb_stor_bulk_srb from usb_stor_Bulk_transport+0x150/0x360
> usb_stor_Bulk_transport from usb_stor_invoke_transport+0x38/0x440
> usb_stor_invoke_transport from usb_stor_control_thread+0x1e0/0x238
> usb_stor_control_thread from kthread+0xf8/0x104
> kthread from ret_from_fork+0x14/0x2c
>
> Refactor memory_intersects to fix the two problems above.
>
> ...
There must be tons of places in the kernel which check to see if two
regions overlap at all, I'm not sure why dma debug needs its own one?
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -110,7 +110,10 @@ static inline bool memory_intersects(void *begin, void *end, void *virt,
> {
> void *vend = virt + size;
>
> - return (virt >= begin && virt < end) || (vend >= begin && vend < end);
> + if (virt < end && vend > begin)
> + return true;
> +
> + return false;
> }
These things bend my brain, but all the cases I've mind-tested worked
out OK.
Now the forever question: is a -stable backport needed? The bug
appears to be six years old, so I guess not. Can you suggest why it
took this long? Are you doing something unusual?
While we're in there, I can't resist fixing that typo...
--- a/include/asm-generic/sections.h~asm-generic-sections-refactor-memory_intersects-fix
+++ a/include/asm-generic/sections.h
@@ -97,7 +97,7 @@ static inline bool memory_contains(void
/**
* memory_intersects - checks if the region occupied by an object intersects
* with another memory region
- * @begin: virtual address of the beginning of the memory regien
+ * @begin: virtual address of the beginning of the memory region
* @end: virtual address of the end of the memory region
* @virt: virtual address of the memory object
* @size: size of the memory object
_
Hi Andrew,
On 2022/8/20 05:51, Andrew Morton wrote:
> On Fri, 19 Aug 2022 16:11:45 +0800 [email protected] wrote:
>
>> From: Quanyang Wang <[email protected]>
>>
>> There are two problems with the current code of memory_intersects:
>>
>> First, it doesn't check whether the region (begin, end) falls inside
>> the region (virt, vend), that is (virt < begin && vend > end).
>>
>> The second problem is if vend is equal to begin, it will return true
>> but this is wrong since vend (virt + size) is not the last address of
>> the memory region but (virt + size -1) is. The wrong determination will
>> trigger the misreporting when the function check_for_illegal_area calls
>> memory_intersects to check if the dma region intersects with stext region.
>>
>> The misreporting is as below (stext is at 0x80100000):
>> WARNING: CPU: 0 PID: 77 at kernel/dma/debug.c:1073 check_for_illegal_area+0x130/0x168
>> DMA-API: chipidea-usb2 e0002000.usb: device driver maps memory from kernel text or rodata [addr=800f0000] [len=65536]
>> Modules linked in:
>> CPU: 1 PID: 77 Comm: usb-storage Not tainted 5.19.0-yocto-standard #5
>> Hardware name: Xilinx Zynq Platform
>> unwind_backtrace from show_stack+0x18/0x1c
>> show_stack from dump_stack_lvl+0x58/0x70
>> dump_stack_lvl from __warn+0xb0/0x198
>> __warn from warn_slowpath_fmt+0x80/0xb4
>> warn_slowpath_fmt from check_for_illegal_area+0x130/0x168
>> check_for_illegal_area from debug_dma_map_sg+0x94/0x368
>> debug_dma_map_sg from __dma_map_sg_attrs+0x114/0x128
>> __dma_map_sg_attrs from dma_map_sg_attrs+0x18/0x24
>> dma_map_sg_attrs from usb_hcd_map_urb_for_dma+0x250/0x3b4
>> usb_hcd_map_urb_for_dma from usb_hcd_submit_urb+0x194/0x214
>> usb_hcd_submit_urb from usb_sg_wait+0xa4/0x118
>> usb_sg_wait from usb_stor_bulk_transfer_sglist+0xa0/0xec
>> usb_stor_bulk_transfer_sglist from usb_stor_bulk_srb+0x38/0x70
>> usb_stor_bulk_srb from usb_stor_Bulk_transport+0x150/0x360
>> usb_stor_Bulk_transport from usb_stor_invoke_transport+0x38/0x440
>> usb_stor_invoke_transport from usb_stor_control_thread+0x1e0/0x238
>> usb_stor_control_thread from kthread+0xf8/0x104
>> kthread from ret_from_fork+0x14/0x2c
>>
>> Refactor memory_intersects to fix the two problems above.
>>
>> ...
> There must be tons of places in the kernel which check to see if two
> regions overlap at all, I'm not sure why dma debug needs its own one?
>
>> --- a/include/asm-generic/sections.h
>> +++ b/include/asm-generic/sections.h
>> @@ -110,7 +110,10 @@ static inline bool memory_intersects(void *begin, void *end, void *virt,
>> {
>> void *vend = virt + size;
>>
>> - return (virt >= begin && virt < end) || (vend >= begin && vend < end);
>> + if (virt < end && vend > begin)
>> + return true;
>> +
>> + return false;
>> }
> These things bend my brain, but all the cases I've mind-tested worked
> out OK.
>
> Now the forever question: is a -stable backport needed? The bug
> appears to be six years old, so I guess not. Can you suggest why it
> took this long? Are you doing something unusual?
Before the commit 1d7db834a027e ("dma-debug: use memory_intersects()
directly") , memory_intersects is called only by printk_late_init:
printk_late_init -> init_section_intersects ->memory_intersects.
There are few places memory_intersects is called.
When the commit 1d7db834a027e ("dma-debug: use memory_intersects()
directly") is merged and CONFIG_DMA_API_DEBUG is enabled,
DMA subsystem uses it to check illegal area and trigger the calltrace above.
Thanks,
Quanyang
>
>
> While we're in there, I can't resist fixing that typo...
>
> --- a/include/asm-generic/sections.h~asm-generic-sections-refactor-memory_intersects-fix
> +++ a/include/asm-generic/sections.h
> @@ -97,7 +97,7 @@ static inline bool memory_contains(void
> /**
> * memory_intersects - checks if the region occupied by an object intersects
> * with another memory region
> - * @begin: virtual address of the beginning of the memory regien
> + * @begin: virtual address of the beginning of the memory region
> * @end: virtual address of the end of the memory region
> * @virt: virtual address of the memory object
> * @size: size of the memory object
> _
>
On Sat, 20 Aug 2022 08:24:52 +0800 quanyang wang <[email protected]> wrote:
> Hi Andrew,
>
> On 2022/8/20 05:51, Andrew Morton wrote:
> > On Fri, 19 Aug 2022 16:11:45 +0800 [email protected] wrote:
> >
> >> From: Quanyang Wang <[email protected]>
> >>
> >> There are two problems with the current code of memory_intersects:
> >>
> >> First, it doesn't check whether the region (begin, end) falls inside
> >> the region (virt, vend), that is (virt < begin && vend > end).
> >>
> >> The second problem is if vend is equal to begin, it will return true
> >> but this is wrong since vend (virt + size) is not the last address of
> >> the memory region but (virt + size -1) is. The wrong determination will
> >> trigger the misreporting when the function check_for_illegal_area calls
> >> memory_intersects to check if the dma region intersects with stext region.
> >>
> >> The misreporting is as below (stext is at 0x80100000):
> >> WARNING: CPU: 0 PID: 77 at kernel/dma/debug.c:1073 check_for_illegal_area+0x130/0x168
> >> DMA-API: chipidea-usb2 e0002000.usb: device driver maps memory from kernel text or rodata [addr=800f0000] [len=65536]
> >> Modules linked in:
> >> CPU: 1 PID: 77 Comm: usb-storage Not tainted 5.19.0-yocto-standard #5
> >> Hardware name: Xilinx Zynq Platform
> >> unwind_backtrace from show_stack+0x18/0x1c
> >> show_stack from dump_stack_lvl+0x58/0x70
> >> dump_stack_lvl from __warn+0xb0/0x198
> >> __warn from warn_slowpath_fmt+0x80/0xb4
> >> warn_slowpath_fmt from check_for_illegal_area+0x130/0x168
> >> check_for_illegal_area from debug_dma_map_sg+0x94/0x368
> >> debug_dma_map_sg from __dma_map_sg_attrs+0x114/0x128
> >> __dma_map_sg_attrs from dma_map_sg_attrs+0x18/0x24
> >> dma_map_sg_attrs from usb_hcd_map_urb_for_dma+0x250/0x3b4
> >> usb_hcd_map_urb_for_dma from usb_hcd_submit_urb+0x194/0x214
> >> usb_hcd_submit_urb from usb_sg_wait+0xa4/0x118
> >> usb_sg_wait from usb_stor_bulk_transfer_sglist+0xa0/0xec
> >> usb_stor_bulk_transfer_sglist from usb_stor_bulk_srb+0x38/0x70
> >> usb_stor_bulk_srb from usb_stor_Bulk_transport+0x150/0x360
> >> usb_stor_Bulk_transport from usb_stor_invoke_transport+0x38/0x440
> >> usb_stor_invoke_transport from usb_stor_control_thread+0x1e0/0x238
> >> usb_stor_control_thread from kthread+0xf8/0x104
> >> kthread from ret_from_fork+0x14/0x2c
> >>
> >> Refactor memory_intersects to fix the two problems above.
> >>
> >> ...
> > There must be tons of places in the kernel which check to see if two
> > regions overlap at all, I'm not sure why dma debug needs its own one?
> >
> >> --- a/include/asm-generic/sections.h
> >> +++ b/include/asm-generic/sections.h
> >> @@ -110,7 +110,10 @@ static inline bool memory_intersects(void *begin, void *end, void *virt,
> >> {
> >> void *vend = virt + size;
> >>
> >> - return (virt >= begin && virt < end) || (vend >= begin && vend < end);
> >> + if (virt < end && vend > begin)
> >> + return true;
> >> +
> >> + return false;
> >> }
> > These things bend my brain, but all the cases I've mind-tested worked
> > out OK.
> >
> > Now the forever question: is a -stable backport needed? The bug
> > appears to be six years old, so I guess not. Can you suggest why it
> > took this long? Are you doing something unusual?
>
> Before the commit 1d7db834a027e ("dma-debug: use memory_intersects()
> directly") , memory_intersects is called only by printk_late_init:
>
> printk_late_init -> init_section_intersects ->memory_intersects.
>
> There are few places memory_intersects is called.
>
> When the commit 1d7db834a027e ("dma-debug: use memory_intersects()
> directly") is merged and CONFIG_DMA_API_DEBUG is enabled,
>
> DMA subsystem uses it to check illegal area and trigger the calltrace above.
>
OK, thanks. I'll add the cc:stable. It will get backported further
back than 1d7db834a027e, but that shouldn't be harmful and might even
be helpful.