lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges
Subsystems (2)
PATCH 1/1 lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges
From: zijun_hu <[email protected]>
endless loop maybe happen if either of parameter addr and end is not page aligned for kernel API function ioremap_page_range()
in order to fix this issue and alert improper range parameters to user WARN_ON() checkup and rounding down range lower boundary are performed firstly, loop end condition within ioremap_pte_range() is optimized due to lack of relevant macro pte_addr_end()
--- lib/ioremap.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
lib/ioremap.c +3 -1
@@ -64,7 +64,7 @@ static int ioremap_pte_range(pmd_t *pmd, unsigned long addr,BUG_ON (!pte_none (*pte));set_pte_at (&init_mm, addr, pte,pfn_pte (pfn, prot)); pfn++; - }while (pte++, addr +=PAGE_SIZE , addr != end); + }while (pte++, addr +=PAGE_SIZE , addr < end && addr >=PAGE_SIZE );return 0 ; }
@@ -129,7 +129,9 @@ int ioremap_page_range(unsigned long addr,int err;BUG_ON (addr >= end); +WARN_ON (!PAGE_ALIGNED (addr) || !PAGE_ALIGNED (end)); + addr =round_down (addr,PAGE_SIZE ); start = addr; phys_addr -= addr; pgd =pgd_offset_k (addr);
Signature · 1 line
1.9.1
Quoted 4 lines · zijun_hu
From:
zijun_hu<[email protected]>endless loop maybe happen if either of parameter addr and end is not page aligned for kernel API function
ioremap_page_range()
Does this happen in practise or this you found it by reading the code?
Quoted 15 lines
in order to fix this issue and alert improper range parameters to user
WARN_ON()checkup and rounding down range lower boundary are performed firstly, loop end condition withinioremap_pte_range()is optimized due to lack of relevant macropte_addr_end()Signed-off-by zijun_hu <[email protected]>--- lib/ioremap.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)lib/ioremap.c +0
-0
lib/ioremap.c static int ioremap_pte_range(pmd_t *pmd, unsigned long addr, · line 69
Quoted 7 lines
lib/ioremap.c +1
-1@@ -64,7 +64,7 @@ static int ioremap_pte_range(pmd_t *pmd, unsigned long addr,BUG_ON (!pte_none (*pte));set_pte_at (&init_mm, addr, pte,pfn_pte (pfn, prot)); pfn++; - }while (pte++, addr +=PAGE_SIZE , addr != end); + }while (pte++, addr +=PAGE_SIZE , addr < end && addr >=PAGE_SIZE );return 0 ; }
Ble, this just overcomplicate things. Can we just make sure that the proper alignment is done in ioremap_page_range which is the only caller of this (and add VM_BUG_ON in ioremap_pud_range to make sure no new caller will forget about that).
lib/ioremap.c int ioremap_page_range(unsigned long addr, · line 132
Quoted 6 lines
lib/ioremap.c +1
-0@@ -129,7 +129,9 @@ int ioremap_page_range(unsigned long addr,int err;BUG_ON (addr >= end); +WARN_ON (!PAGE_ALIGNED (addr) || !PAGE_ALIGNED (end));
maybe WARN_ON_ONCE would be sufficient to prevent from swamping logs if something just happens to do this too often in some pathological path.
lib/ioremap.c int ioremap_page_range(unsigned long addr, · line 134
Quoted 2 lines
lib/ioremap.c +1
-0+ addr =round_down (addr,PAGE_SIZE );
end = round_up(end, PAGE_SIZE);
wouldn't work?
Quoted 11 lines
lib/ioremap.c +0
-0start = addr; phys_addr -= addr; pgd =pgd_offset_k (addr);Signature · 7 lines
1.9.1 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Signature · 8 lines
Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 8 lines · Michal Hocko
Quoted 4 lines · zijun_hu
From:
zijun_hu<[email protected]>endless loop maybe happen if either of parameter addr and end is not page aligned for kernel API function
ioremap_page_range()Does this happen in practise or this you found it by reading the code?
i found it by reading the code, this is a kernel API function and there are no enough hint for parameter requirements, so any parameters combination maybe be used by user, moreover, it seems appropriate for many bad parameter combination, for example, provided PMD_SIZE=2M and PAGE_SIZE=4K, 0x00 is used for aligned very well address
a user maybe want to map virtual range[0x1ff800, 0x200800) to physical address 0x300800, it will cause endless loop
Quoted 19 lines
Quoted 19 lines
in order to fix this issue and alert improper range parameters to user
WARN_ON()checkup and rounding down range lower boundary are performed firstly, loop end condition withinioremap_pte_range()is optimized due to lack of relevant macropte_addr_end()Signed-off-by zijun_hu <[email protected]>--- lib/ioremap.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)lib/ioremap.c +0
-1@@ -64,7 +64,7 @@ static int ioremap_pte_range(pmd_t *pmd, unsigned long addr,BUG_ON (!pte_none (*pte));set_pte_at (&init_mm, addr, pte,pfn_pte (pfn, prot)); pfn++; - }while (pte++, addr +=PAGE_SIZE , addr != end);
Quoted 9 lines
lib/ioremap.c static int ioremap_pte_range(pmd_t *pmd, unsigned long addr, · line 69
Quoted 3 lines
lib/ioremap.c +1
-0+ }while (pte++, addr +=PAGE_SIZE , addr < end && addr >=PAGE_SIZE );return 0 ; }Ble, this just overcomplicate things. Can we just make sure that the proper alignment is done in
ioremap_page_rangewhich is the only caller of this (and addVM_BUG_ONinioremap_pud_rangeto make sure no new caller will forget about that).
this complicate express is used to avoid addr overflow, consider map virtual rang [0xffe00800, 0xfffff800) for 32bit machine actually, my previous approach is just like that you pointed mailed at 20/09 as below, besides, i apply my previous approach for mm/vmalloc.c and [email protected] have "For API functions perhaps it's reasonable" comments i don't tell which is better
diff --git a/lib/ioremap.c b/lib/ioremap.c
--- a/lib/ioremap.c
+++ b/lib/ioremap.c
@@ -64,7 +64,7 @@ static int ioremap_pte_range(pmd_t *pmd, unsigned long addr,
BUG_ON(!pte_none(*pte));
set_pte_at(&init_mm, addr, pte, pfn_pte(pfn, prot));
pfn++;
- } while (pte++, addr += PAGE_SIZE, addr != end);
+ } while (pte++, addr += PAGE_SIZE, addr < end);
return 0; }
@@ -129,6 +129,7 @@ int ioremap_page_range(unsigned long addr,
int err;
BUG_ON(addr >= end); + BUG_ON(!PAGE_ALIGNED(addr) || !PAGE_ALIGNED(end));
start = addr;
phys_addr -= addr; Quoted 10 lines
lib/ioremap.c int ioremap_page_range(unsigned long addr, · line 132
Quoted 6 lines
lib/ioremap.c +1
-0@@ -129,7 +129,9 @@ int ioremap_page_range(unsigned long addr,int err;BUG_ON (addr >= end); +WARN_ON (!PAGE_ALIGNED (addr) || !PAGE_ALIGNED (end));maybe
WARN_ON_ONCEwould be sufficient to prevent from swamping logs if something just happens to do this too often in some pathological path.
if WARN_ON_ONCE is used, the later bad ranges for many other purposes can't be alerted, and ioremap_page_range() can map large enough ranges so not too many calls happens for a purpose
Quoted 7 lines
lib/ioremap.c int ioremap_page_range(unsigned long addr, · line 134
Quoted 2 lines
lib/ioremap.c +1
-0+ addr =round_down (addr,PAGE_SIZE );end = round_up(end, PAGE_SIZE);wouldn't work?
no, it don't work for many special case
for example, provided PMD_SIZE=2M
mapping [0x1f8800, 0x208800) virtual range will be split to two ranges [0x1f8800, 0x200000) and [0x200000,0x208800) and map them separately the first range will cause dead loop
Quoted 12 lines
Quoted 11 lines
lib/ioremap.c +0
-0start = addr; phys_addr -= addr; pgd =pgd_offset_k (addr);Signature · 7 lines
1.9.1 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 9 lines · zijun_hu
Quoted 8 lines · Michal Hocko
Quoted 4 lines · zijun_hu
From:
zijun_hu<[email protected]>endless loop maybe happen if either of parameter addr and end is not page aligned for kernel API function
ioremap_page_range()Does this happen in practise or this you found it by reading the code?
Quoted 7 lines
i found it by reading the code, this is a kernel API function and there are no enough hint for parameter requirements, so any parameters combination maybe be used by user, moreover, it seems appropriate for many bad parameter combination, for example, provided PMD_SIZE=2M and PAGE_SIZE=4K,
0x00is used for aligned very well address
a user maybe want to map virtual range[0x1ff800,0x200800) to physical address0x300800, it will cause endless loop
Well, we are relying on the kernel to do the sane thing otherwise we would be screwed anyway. If this can be triggered by a userspace then it would be a different story. Just look at how we are doing mmap, we sanitize the page alignment at the high level and the lower level functions just assume sane values.
Quoted 39 lines
Quoted 19 lines
Quoted 19 lines
in order to fix this issue and alert improper range parameters to user
WARN_ON()checkup and rounding down range lower boundary are performed firstly, loop end condition withinioremap_pte_range()is optimized due to lack of relevant macropte_addr_end()Signed-off-by zijun_hu <[email protected]>--- lib/ioremap.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)lib/ioremap.c +0
-1@@ -64,7 +64,7 @@ static int ioremap_pte_range(pmd_t *pmd, unsigned long addr,BUG_ON (!pte_none (*pte));set_pte_at (&init_mm, addr, pte,pfn_pte (pfn, prot)); pfn++; - }while (pte++, addr +=PAGE_SIZE , addr != end);Quoted 9 lines
lib/ioremap.c static int ioremap_pte_range(pmd_t *pmd, unsigned long addr, · line 69
Quoted 3 lines
lib/ioremap.c +1
-0+ }while (pte++, addr +=PAGE_SIZE , addr < end && addr >=PAGE_SIZE );return 0 ; }Ble, this just overcomplicate things. Can we just make sure that the proper alignment is done in
ioremap_page_rangewhich is the only caller of this (and addVM_BUG_ONinioremap_pud_rangeto make sure no new caller will forget about that).this complicate express is used to avoid addr overflow, consider map virtual rang [
0xffe00800,0xfffff800) for 32bit machine actually, my previous approach is just like that you pointed mailed at 20/09 as below, besides, i apply my previous approach formm/vmalloc.cand [email protected] have "For API functions perhaps it's reasonable" comments i don't tell which is betterdiff --git
a/lib/ioremap.cb/lib/ioremap.c
---a/lib/ioremap.c
+++b/lib/ioremap.c
@@ -64,7 +64,7 @@ static int ioremap_pte_range(pmd_t *pmd, unsigned long addr,
Quoted 7 lines
BUG_ON(!pte_none(*pte)); set_pte_at(&init_mm, addr, pte, pfn_pte(pfn, prot)); pfn++; - } while (pte++, addr += PAGE_SIZE, addr != end); + } while (pte++, addr += PAGE_SIZE, addr < end); return 0;}
yes this looks good to me
Quoted 6 lines
@@ -129,6 +129,7 @@ int ioremap_page_range(unsigned long addr,
int err; BUG_ON(addr >= end);+
BUG_ON(!PAGE_ALIGNED(addr) || !PAGE_ALIGNED(end));
Well, BUG_ON is rather harsh for something that would be trivially fixable.
Quoted 22 lines
start = addr; phys_addr -= addr;Quoted 10 lines
lib/ioremap.c int ioremap_page_range(unsigned long addr, · line 132
Quoted 6 lines
lib/ioremap.c +1
-0@@ -129,7 +129,9 @@ int ioremap_page_range(unsigned long addr,int err;BUG_ON (addr >= end); +WARN_ON (!PAGE_ALIGNED (addr) || !PAGE_ALIGNED (end));maybe
WARN_ON_ONCEwould be sufficient to prevent from swamping logs if something just happens to do this too often in some pathological path.if
WARN_ON_ONCEis used, the later bad ranges for many other purposes can't be alerted, andioremap_page_range()can map large enough ranges so not too many calls happens for a purposeQuoted 5 lines
lib/ioremap.c int ioremap_page_range(unsigned long addr, · line 134
Quoted 2 lines
lib/ioremap.c +1
-0+ addr =round_down (addr,PAGE_SIZE );end = round_up(end, PAGE_SIZE);
Quoted 7 lines
Quoted 2 lines
wouldn't work?
no, it don't work for many special case
for example, provided PMD_SIZE=2M
mapping [0x1f8800,0x208800) virtual range will be split to two ranges [0x1f8800,0x200000) and [0x200000,0x208800) and map them separately the first range will cause dead loop
I am not sure I see your point. How can we deadlock if _both_ addresses get aligned to the page boundary and how does PMD_SIZE make any difference.
Signature · 8 lines
Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 18 lines · Michal Hocko
Quoted 9 lines · zijun_hu
Quoted 8 lines · Michal Hocko
Quoted 4 lines · zijun_hu
From:
zijun_hu<[email protected]>endless loop maybe happen if either of parameter addr and end is not page aligned for kernel API function
ioremap_page_range()Does this happen in practise or this you found it by reading the code?
Quoted 7 lines
i found it by reading the code, this is a kernel API function and there are no enough hint for parameter requirements, so any parameters combination maybe be used by user, moreover, it seems appropriate for many bad parameter combination, for example, provided PMD_SIZE=2M and PAGE_SIZE=4K,
0x00is used for aligned very well address
a user maybe want to map virtual range[0x1ff800,0x200800) to physical address0x300800, it will cause endless loop
Quoted 6 lines
Well, we are relying on the kernel to do the sane thing otherwise we would be screwed anyway. If this can be triggered by a userspace then it would be a different story. Just look at how we are doing mmap, we sanitize the page alignment at the high level and the lower level functions just assume sane values.
ioremap_page_range() is exported by EXPORT_SYMBOL_GPL() as a kernel interface so perhaps it is called by not only any kernel module authors but also other kernel parts
if the bad range is used by a careless kernel user really, it seems a better choice to alert the warning message or panic the kernel than hanging the system due to endless loop, it can help them locate problem usefully
so additional sane check BUG_ON(!PAGE_ALIGNED(addr) || !PAGE_ALIGNED(end)) is appended to existing check BUG_ON(addr >= end)
Quoted 50 lines
Quoted 39 lines
Quoted 19 lines
Quoted 19 lines
in order to fix this issue and alert improper range parameters to user
WARN_ON()checkup and rounding down range lower boundary are performed firstly, loop end condition withinioremap_pte_range()is optimized due to lack of relevant macropte_addr_end()Signed-off-by zijun_hu <[email protected]>--- lib/ioremap.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)lib/ioremap.c +0
-1@@ -64,7 +64,7 @@ static int ioremap_pte_range(pmd_t *pmd, unsigned long addr,BUG_ON (!pte_none (*pte));set_pte_at (&init_mm, addr, pte,pfn_pte (pfn, prot)); pfn++; - }while (pte++, addr +=PAGE_SIZE , addr != end);Quoted 9 lines
lib/ioremap.c static int ioremap_pte_range(pmd_t *pmd, unsigned long addr, · line 69
Quoted 3 lines
lib/ioremap.c +1
-0+ }while (pte++, addr +=PAGE_SIZE , addr < end && addr >=PAGE_SIZE );return 0 ; }Ble, this just overcomplicate things. Can we just make sure that the proper alignment is done in
ioremap_page_rangewhich is the only caller of this (and addVM_BUG_ONinioremap_pud_rangeto make sure no new caller will forget about that).this complicate express is used to avoid addr overflow, consider map virtual rang [
0xffe00800,0xfffff800) for 32bit machine actually, my previous approach is just like that you pointed mailed at 20/09 as below, besides, i apply my previous approach formm/vmalloc.cand [email protected] have "For API functions perhaps it's reasonable" comments i don't tell which is betterdiff --git
a/lib/ioremap.cb/lib/ioremap.c
---a/lib/ioremap.c
+++b/lib/ioremap.c
@@ -64,7 +64,7 @@ static int ioremap_pte_range(pmd_t *pmd, unsigned long addr,Quoted 7 lines
BUG_ON(!pte_none(*pte)); set_pte_at(&init_mm, addr, pte, pfn_pte(pfn, prot)); pfn++; - } while (pte++, addr += PAGE_SIZE, addr != end); + } while (pte++, addr += PAGE_SIZE, addr < end); return 0;}
yes this looks good to me
Quoted 1 line
Quoted 9 lines
Quoted 5 lines
@@ -129,6 +129,7 @@ int ioremap_page_range(unsigned long addr,
int err; BUG_ON(addr >= end);+
BUG_ON(!PAGE_ALIGNED(addr) || !PAGE_ALIGNED(end));Well,
BUG_ONis rather harsh for something that would be trivially fixable.
i append the additional BUG_ON() by resembling exiting BUG_ON(addr >= end);
Quoted 30 lines
Quoted 22 lines
start = addr; phys_addr -= addr;Quoted 10 lines
lib/ioremap.c int ioremap_page_range(unsigned long addr, · line 132
Quoted 6 lines
lib/ioremap.c +1
-0@@ -129,7 +129,9 @@ int ioremap_page_range(unsigned long addr,int err;BUG_ON (addr >= end); +WARN_ON (!PAGE_ALIGNED (addr) || !PAGE_ALIGNED (end));maybe
WARN_ON_ONCEwould be sufficient to prevent from swamping logs if something just happens to do this too often in some pathological path.if
WARN_ON_ONCEis used, the later bad ranges for many other purposes can't be alerted, andioremap_page_range()can map large enough ranges so not too many calls happens for a purposeQuoted 5 lines
lib/ioremap.c int ioremap_page_range(unsigned long addr, · line 134
Quoted 2 lines
lib/ioremap.c +1
-0+ addr =round_down (addr,PAGE_SIZE );end = round_up(end, PAGE_SIZE);Quoted 7 lines
Quoted 2 lines
wouldn't work?
no, it don't work for many special case
for example, provided PMD_SIZE=2M
mapping [0x1f8800,0x208800) virtual range will be split to two ranges [0x1f8800,0x200000) and [0x200000,0x208800) and map them separately the first range will cause dead loop
Quoted 4 lines
I am not sure I see your point. How can we deadlock if _both_ addresses get aligned to the page boundary and how does
PMD_SIZEmake any difference.
i will take a example to illustrate my considerations provided PUD_SIZE == 1G, PMD_SIZE == 2M, PAGE_SIZE == 4K it is used by arm64 normally
we want to map virtual range [0xffffffff_ffc08800, 0xffffffff_fffff800) by ioremap_page_range(),ioremap_pmd_range() is called to map the range finally, ioremap_pmd_range() will callioremap_pte_range(pmd, 0xffffffff_ffc08800, 0xffffffff_fffe0000) and ioremap_pte_range(pmd, 0xffffffff_fffe0000, 0xffffffff fffff800) separately
let's loop end condition(pte++, addr += PAGE_SIZE, addr != end) within ioremap_pte_range() for the two ioremap_pte_range() calls separately
for ioremap_pte_range(pmd, 0xffffffff_ffc08800, 0xffffffff_fffe0000) addr != end don't end the while loop due to parameter addr = 0xffffffff_ffc08800 is not page aligned, even any number of PAGE_SIZE is added to addr, addr can't equal to page aligned parameter end 0xffffffff_fffe0000
so we change the "addr != end" to "addr < end"
for ioremap_pte_range(pmd, 0xffffffff_fffe0000, 0xffffffff fffff800) let us consider the state after the final page with the range is mapped via a number of while loops, at that moment, addr == 0xffffffff_fffff000, end == 0xffffffff_fffff800, let us see the fixed loop end condition (pte++, addr += PAGE_SIZE, addr < end), addr overflow to 0 after PAGE_SIZE is added , it cause the condition is true and loop continues, it should be terminated the loop
so we correct the "addr < end" to "addr < end && addr >= PAGE_SIZE"
in summary, there are two approaches for fixing this issue i don't tell which is better
Quoted 25 lines · zijun_hu
Quoted 18 lines · Michal Hocko
Quoted 9 lines · zijun_hu
Quoted 8 lines · Michal Hocko
Quoted 4 lines · zijun_hu
From:
zijun_hu<[email protected]>endless loop maybe happen if either of parameter addr and end is not page aligned for kernel API function
ioremap_page_range()Does this happen in practise or this you found it by reading the code?
Quoted 7 lines
i found it by reading the code, this is a kernel API function and there are no enough hint for parameter requirements, so any parameters combination maybe be used by user, moreover, it seems appropriate for many bad parameter combination, for example, provided PMD_SIZE=2M and PAGE_SIZE=4K,
0x00is used for aligned very well address
a user maybe want to map virtual range[0x1ff800,0x200800) to physical address0x300800, it will cause endless loopQuoted 6 lines
Well, we are relying on the kernel to do the sane thing otherwise we would be screwed anyway. If this can be triggered by a userspace then it would be a different story. Just look at how we are doing mmap, we sanitize the page alignment at the high level and the lower level functions just assume sane values.
Quoted 7 lines
ioremap_page_range()is exported byEXPORT_SYMBOL_GPL()as a kernel interface so perhaps it is called by not only any kernel module authors but also other kernel partsif the bad range is used by a careless kernel user really, it seems a better choice to alert the warning message or panic the kernel than hanging the system due to endless loop, it can help them locate problem usefully
I absolutely do not want to panic my system just because a crapy module or whatnot doesn't provide an aligned address. Warning and a fixup sounds much more sane to me.
[...]
Quoted 14 lines
Quoted 10 lines
Quoted 5 lines
no, it don't work for many special case
for example, provided PMD_SIZE=2M
mapping [0x1f8800,0x208800) virtual range will be split to two ranges [0x1f8800,0x200000) and [0x200000,0x208800) and map them separately the first range will cause dead loopI am not sure I see your point. How can we deadlock if _both_ addresses get aligned to the page boundary and how does
PMD_SIZEmake any difference.i will take a example to illustrate my considerations provided
PUD_SIZE== 1G,PMD_SIZE== 2M,PAGE_SIZE== 4K it is used by arm64 normally
Quoted 5 lines
we want to map virtual range [0xffffffff_ffc08800, 0xffffffff_fffff800) by ioremap_page_range(),ioremap_pmd_range() is called to map the range finally,
ioremap_pmd_range()will callioremap_pte_range(pmd, 0xffffffff_ffc08800, 0xffffffff_fffe0000)andioremap_pte_range(pmd, 0xffffffff_fffe0000, 0xffffffff fffff800)separately
but those ranges are not aligned and it ioremap_page_range fix them up to _be_ aligned then there is no problem, right? So either I am missing something or we are talking past each other.
Signature · 8 lines
Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 20 lines · Michal Hocko
Quoted 14 lines
Quoted 10 lines
Quoted 5 lines
no, it don't work for many special case
for example, provided PMD_SIZE=2M
mapping [0x1f8800,0x208800) virtual range will be split to two ranges [0x1f8800,0x200000) and [0x200000,0x208800) and map them separately the first range will cause dead loopI am not sure I see your point. How can we deadlock if _both_ addresses get aligned to the page boundary and how does
PMD_SIZEmake any difference.i will take a example to illustrate my considerations provided
PUD_SIZE== 1G,PMD_SIZE== 2M,PAGE_SIZE== 4K it is used by arm64 normallyQuoted 5 lines
we want to map virtual range [0xffffffff_ffc08800, 0xffffffff_fffff800) by ioremap_page_range(),ioremap_pmd_range() is called to map the range finally,
ioremap_pmd_range()will callioremap_pte_range(pmd, 0xffffffff_ffc08800, 0xffffffff_fffe0000)andioremap_pte_range(pmd, 0xffffffff_fffe0000, 0xffffffff fffff800)separately
Quoted 4 lines
but those ranges are not aligned and it
ioremap_page_rangefix them up to _be_ aligned then there is no problem, right? So either I am missing something or we are talking past each other.
my complementary considerations are show below
why not to round up the range start boundary to page aligned?
1, it don't remain consistent with the original logic
take map [0x1800, 0x4800) as example
the original logic map range [0x1000, 0x2000), but rounding up start boundary don't mapping the range [0x1000, 0x2000)
2, the rounding up start boundary maybe cause overflow, consider start boundary =
0xffffffff_fffff800
Quoted 25 lines · zijun_hu
Quoted 20 lines · Michal Hocko
Quoted 14 lines
Quoted 10 lines
Quoted 5 lines
no, it don't work for many special case
for example, provided PMD_SIZE=2M
mapping [0x1f8800,0x208800) virtual range will be split to two ranges [0x1f8800,0x200000) and [0x200000,0x208800) and map them separately the first range will cause dead loopI am not sure I see your point. How can we deadlock if _both_ addresses get aligned to the page boundary and how does
PMD_SIZEmake any difference.i will take a example to illustrate my considerations provided
PUD_SIZE== 1G,PMD_SIZE== 2M,PAGE_SIZE== 4K it is used by arm64 normallyQuoted 5 lines
we want to map virtual range [0xffffffff_ffc08800, 0xffffffff_fffff800) by ioremap_page_range(),ioremap_pmd_range() is called to map the range finally,
ioremap_pmd_range()will callioremap_pte_range(pmd, 0xffffffff_ffc08800, 0xffffffff_fffe0000)andioremap_pte_range(pmd, 0xffffffff_fffe0000, 0xffffffff fffff800)separatelyQuoted 4 lines
but those ranges are not aligned and it
ioremap_page_rangefix them up to _be_ aligned then there is no problem, right? So either I am missing something or we are talking past each other.
Quoted 7 lines
my complementary considerations are show below
why not to round up the range start boundary to page aligned?
1, it don't remain consistent with the original logic
take map [0x1800,0x4800) as example
the original logic map range [0x1000,0x2000), but rounding up start boundary don't mapping the range [0x1000,0x2000)
just look at how we do that for the mmap...
Quoted 2 lines
2, the rounding up start boundary maybe cause overflow, consider start boundary =
0xffffffff_fffff800
this is just insane
Signature · 8 lines
Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 28 lines · Michal Hocko
Quoted 27 lines · zijun_hu
Quoted 20 lines · Michal Hocko
Quoted 14 lines
Quoted 10 lines
Quoted 5 lines
no, it don't work for many special case
for example, provided PMD_SIZE=2M
mapping [0x1f8800,0x208800) virtual range will be split to two ranges [0x1f8800,0x200000) and [0x200000,0x208800) and map them separately the first range will cause dead loopI am not sure I see your point. How can we deadlock if _both_ addresses get aligned to the page boundary and how does
PMD_SIZEmake any difference.i will take a example to illustrate my considerations provided
PUD_SIZE== 1G,PMD_SIZE== 2M,PAGE_SIZE== 4K it is used by arm64 normallyQuoted 5 lines
we want to map virtual range [0xffffffff_ffc08800, 0xffffffff_fffff800) by ioremap_page_range(),ioremap_pmd_range() is called to map the range finally,
ioremap_pmd_range()will callioremap_pte_range(pmd, 0xffffffff_ffc08800, 0xffffffff_fffe0000)andioremap_pte_range(pmd, 0xffffffff_fffe0000, 0xffffffff fffff800)separatelyQuoted 4 lines
but those ranges are not aligned and it
ioremap_page_rangefix them up to _be_ aligned then there is no problem, right? So either I am missing something or we are talking past each other.my complementary considerations are show below
Quoted 7 lines
Quoted 5 lines
why not to round up the range start boundary to page aligned?
1, it don't remain consistent with the original logic
take map [0x1800,0x4800) as example
the original logic map range [0x1000,0x2000), but rounding up start boundary don't mapping the range [0x1000,0x2000)just look at how we do that for the mmap...
okay
i don't familiar with mmap code very well now
it is okay to roundup start boundary to page aligned in order to keep consistent with Mmap code if insane start boundary overflow is considered
Quoted 6 lines
Quoted 3 lines
2, the rounding up start boundary maybe cause overflow, consider start boundary =
0xffffffff_fffff800this is just insane
Quoted 35 lines · zijun_hu
Quoted 34 lines · Michal Hocko
Quoted 32 lines · zijun_hu
Quoted 20 lines · Michal Hocko
Quoted 14 lines
Quoted 10 lines
Quoted 5 lines
no, it don't work for many special case
for example, provided PMD_SIZE=2M
mapping [0x1f8800,0x208800) virtual range will be split to two ranges [0x1f8800,0x200000) and [0x200000,0x208800) and map them separately the first range will cause dead loopI am not sure I see your point. How can we deadlock if _both_ addresses get aligned to the page boundary and how does
PMD_SIZEmake any difference.i will take a example to illustrate my considerations provided
PUD_SIZE== 1G,PMD_SIZE== 2M,PAGE_SIZE== 4K it is used by arm64 normallyQuoted 5 lines
we want to map virtual range [0xffffffff_ffc08800, 0xffffffff_fffff800) by ioremap_page_range(),ioremap_pmd_range() is called to map the range finally,
ioremap_pmd_range()will callioremap_pte_range(pmd, 0xffffffff_ffc08800, 0xffffffff_fffe0000)andioremap_pte_range(pmd, 0xffffffff_fffe0000, 0xffffffff fffff800)separatelyQuoted 4 lines
but those ranges are not aligned and it
ioremap_page_rangefix them up to _be_ aligned then there is no problem, right? So either I am missing something or we are talking past each other.my complementary considerations are show below
why not to round up the range start boundary to page aligned?
1, it don't remain consistent with the original logic
take map [0x1800,0x4800) as example
the original logic map range [0x1000,0x2000), but rounding up start boundary don't mapping the range [0x1000,0x2000)
Quoted 4 lines
Quoted 1 line
just look at how we do that for the mmap...
okay
i don't familiar with mmap code very well now
mmap basically does addr &= PAGE_MASK (modulo mmap_min_addr) and len = PAGE_ALIGN(len).
this is [star, end) raher than [start, start+len) but you should get the point I guess.
Signature · 8 lines
Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 38 lines · Michal Hocko
Quoted 37 lines · zijun_hu
Quoted 35 lines · Michal Hocko
Quoted 25 lines · zijun_hu
Quoted 20 lines · Michal Hocko
Quoted 14 lines
Quoted 10 lines
Quoted 5 lines
no, it don't work for many special case
for example, provided PMD_SIZE=2M
mapping [0x1f8800,0x208800) virtual range will be split to two ranges [0x1f8800,0x200000) and [0x200000,0x208800) and map them separately the first range will cause dead loopI am not sure I see your point. How can we deadlock if _both_ addresses get aligned to the page boundary and how does
PMD_SIZEmake any difference.i will take a example to illustrate my considerations provided
PUD_SIZE== 1G,PMD_SIZE== 2M,PAGE_SIZE== 4K it is used by arm64 normallyQuoted 5 lines
we want to map virtual range [0xffffffff_ffc08800, 0xffffffff_fffff800) by ioremap_page_range(),ioremap_pmd_range() is called to map the range finally,
ioremap_pmd_range()will callioremap_pte_range(pmd, 0xffffffff_ffc08800, 0xffffffff_fffe0000)andioremap_pte_range(pmd, 0xffffffff_fffe0000, 0xffffffff fffff800)separatelyQuoted 4 lines
but those ranges are not aligned and it
ioremap_page_rangefix them up to _be_ aligned then there is no problem, right? So either I am missing something or we are talking past each other.Quoted 7 lines
my complementary considerations are show below
why not to round up the range start boundary to page aligned?
1, it don't remain consistent with the original logic
take map [0x1800,0x4800) as example
the original logic map range [0x1000,0x2000), but rounding up start boundary don't mapping the range [0x1000,0x2000)just look at how we do that for the mmap...
Quoted 9 lines
Quoted 2 lines
okay
i don't familiar with mmap code very well nowmmap basically does addr &=
PAGE_MASK(modulommap_min_addr) and len =PAGE_ALIGN(len).this is [star, end) raher than [start, start+len) but you should get the point I guess.
you are right
this patch is consistent with that you pointed
for map virtual range [0x80000800, 0x80007800) to physical area[0x20000800, 0x20007800) it actually map range [0x80000000, 0x80008000) to physical area[0x20000000, 0x20008000)
maybe expanding range [0x80000800, 0x80007800) to [0x80000000, 0x80008000) is better than shrinking to [0x80001000, 0x80007000) because the following reasons
1. if a user is mapping [0x80000800, 0x80007800) -> [0x20000800, 0x20007800), he/she expect to access physical address 0x20000800 by virtual address 0x80000800, expanding range do the right thing but shrinking will cause address fault
2. shrinking will cause not enough virtual range [0x80001000, 0x80007000) to mapping physical area [0x20000800, 0x20007800)
3. this is no need to round up parameter end to page boundary to expand the end limit, it has less modification for code
BTW
there are many page table operations to using this similar logic, maybe a universal fixing is used to all, not just lib/ioremap.c or mm/vmalloc.c
Quoted 30 lines · zijun_hu
From:
zijun_hu<[email protected]>endless loop maybe happen if either of parameter addr and end is not page aligned for kernel API function
ioremap_page_range()in order to fix this issue and alert improper range parameters to user
WARN_ON()checkup and rounding down range lower boundary are performed firstly, loop end condition withinioremap_pte_range()is optimized due to lack of relevant macropte_addr_end()Signed-off-by zijun_hu <[email protected]>--- lib/ioremap.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)lib/ioremap.c +1
-1@@ -64,7 +64,7 @@ static int ioremap_pte_range(pmd_t *pmd, unsigned long addr,BUG_ON (!pte_none (*pte));set_pte_at (&init_mm, addr, pte,pfn_pte (pfn, prot)); pfn++; - }while (pte++, addr +=PAGE_SIZE , addr != end); + }while (pte++, addr +=PAGE_SIZE , addr < end && addr >=PAGE_SIZE );return 0 ; }
lib/ioremap.c int ioremap_page_range(unsigned long addr, · line 137
Quoted 9 lines
lib/ioremap.c +2
-0@@ -129,7 +129,9 @@ int ioremap_page_range(unsigned long addr,int err;BUG_ON (addr >= end); +WARN_ON (!PAGE_ALIGNED (addr) || !PAGE_ALIGNED (end)); + addr =round_down (addr,PAGE_SIZE ); start = addr; phys_addr -= addr; pgd =pgd_offset_k (addr);
From: zijun_hu <[email protected]>
s/WARN_ON()/WARN_ON_ONCE()/ to reduce warning messages
--- lib/ioremap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/ioremap.c +1 -1
@@ -129,7 +129,7 @@ int ioremap_page_range(unsigned long addr,int err;BUG_ON (addr >= end); -WARN_ON (!PAGE_ALIGNED (addr) || !PAGE_ALIGNED (end)); +WARN_ON_ONCE (!PAGE_ALIGNED (addr) || !PAGE_ALIGNED (end)); addr =round_down (addr,PAGE_SIZE ); start = addr;
Signature · 1 line
1.9.1
Hello,
Quoted 11 lines · zijun_hu
From:
zijun_hu<[email protected]>endless loop maybe happen if either of parameter addr and end is not page aligned for kernel API function
ioremap_page_range()in order to fix this issue and alert improper range parameters to user
WARN_ON()checkup and rounding down range lower boundary are performed firstly, loop end condition withinioremap_pte_range()is optimized due to lack of relevant macropte_addr_end()Signed-off-by zijun_hu <[email protected]>
Unfortunately, I can't see what the points are in this series of patches. Most seem to be gratuitous changes which don't address real issues or improve anything. "I looked at the code and realized that, if the input were wrong, the function would misbehave" isn't good enough a reason. What's next? Are we gonna harden all pointer taking functions too?
For internal functions, we don't by default do input sanitization / sanity check. There sure are cases where doing so is beneficial but reading a random function and thinking "oh this combo of parameters can make it go bonkers" isn't the right approach for it. We end up with cruft and code changes which nobody needed in the first place and can easily introduce actual real bugs in the process.
It'd be an a lot more productive use of time and effort for everyone involved if the work is around actual issues.
Thanks.
Signature · 7 lines
tejun -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 29 lines · Tejun Heo
Hello,
Quoted 11 lines · zijun_hu
From:
zijun_hu<[email protected]>endless loop maybe happen if either of parameter addr and end is not page aligned for kernel API function
ioremap_page_range()in order to fix this issue and alert improper range parameters to user
WARN_ON()checkup and rounding down range lower boundary are performed firstly, loop end condition withinioremap_pte_range()is optimized due to lack of relevant macropte_addr_end()Signed-off-by zijun_hu <[email protected]>Unfortunately, I can't see what the points are in this series of patches. Most seem to be gratuitous changes which don't address real issues or improve anything. "I looked at the code and realized that, if the input were wrong, the function would misbehave" isn't good enough a reason. What's next? Are we gonna harden all pointer taking functions too?
For internal functions, we don't by default do input sanitization / sanity check. There sure are cases where doing so is beneficial but reading a random function and thinking "oh this combo of parameters can make it go bonkers" isn't the right approach for it. We end up with cruft and code changes which nobody needed in the first place and can easily introduce actual real bugs in the process.
Quoted 5 lines
It'd be an a lot more productive use of time and effort for everyone involved if the work is around actual issues.
Thanks.
thanks for your reply firstly
1. ioremap_page_range() is not a kernel internal function
2. sanity check "BUG_ON(addr >= end)" have existed already, but don't check enough
3. are there any obvious hint for rang parameter requirements but BUG_ON(addr >= end)
4. if range which seems right but wrong really is used such as mapping virtual range [0x80000800, 0x80007800) to physical area[0x20000800, 0x20007800) what actions should we take? warning message and trying to finish user request or panic kernel or hang system in endless loop or return -EINVAL？ how to help user find their problem?
5. if both boundary of the range are aligned to page, ioremap_page_range() works well otherwise endless loop maybe happens
Hello,
Quoted 1 line · zijun_hu
1.
ioremap_page_range()is not a kernel internal function
What matters is whether the input is from userland or easy to get wrong (IOW the checks serve practical purposes). Whether a function is exported via EXPORT_SYMBOL doesn't matter that much in this regard. EXPORT_SYMBOL doesn't really demark an API layer (we don't put any effort into keeping it stable or versioned). Modules are loaded separately but still part of the same program.
Quoted 1 line
2. sanity check "
BUG_ON(addr >= end)" have existed already, but don't check enough
That particular check being there doesn't imply that it needs to be expanded. If you actually have cases where this mattered and extra checks would have substantially helped debugging, sure, but that's not the case here.
Quoted 1 line
3. are there any obvious hint for rang parameter requirements but
BUG_ON(addr >= end)
You're welcome to add documentation.
Quoted 7 lines
4. if range which seems right but wrong really is used such as mapping virtual range [
0x80000800,0x80007800) to physical area[0x20000800,0x20007800) what actions should we take? warning message and trying to finish user request or panic kernel or hang system in endless loop or return -EINVALi 1/4 ? how to help user find their problem?
5. if both boundary of the range are aligned to page,ioremap_page_range()works well otherwise endless loop maybe happens
I don't think it's helpful to imgaine pathological conditions and try to address all of them. There's no evidence, not even a tenuous one, that anyone is suffering from this. Sometimes it is useful to implement precautions preemptively but in those cases the rationles should be along the line of "it's easy to get the inputs wrong for this function because ABC and those cases are difficult to debug due to XYZ", not "let's harden all the functions against all input combinations regardless of likelihood or usefulness".
The thing is that the latter approach not only wastes time of everyone involved without any real gain but also actually deteriorates the code base by adding unnecessary complications and introduces bugs through gratuitous changes. Please note that I'm not trying to say re-factoring or cleanups are to be avoided. We need them for long term maintainability, even at the cost of introducing some bugs in the process, but the patches you're submitting are quite off the mark.
Thanks.
Signature · 7 lines
tejun -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>