Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[Tejun Heo]

Hello,

On Fri, Sep 23, 2016 at 11:41:33PM +0800, zijun_hu wrote:
> 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.

> 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.

> 3. are there any obvious hint for rang parameter requirements but BUG_ON(addr 
> >= end)

You're welcome to add documentation.

> 4. if range which seems right but wrong really is used such as mapping 
>virtual range [0x8800, 0x80007800) to physical area[0x2800, 
> 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

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.

-- 
tejun

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[Tejun Heo]

Hello,

On Fri, Sep 23, 2016 at 11:41:33PM +0800, zijun_hu wrote:
> 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.

> 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.

> 3. are there any obvious hint for rang parameter requirements but BUG_ON(addr 
> >= end)

You're welcome to add documentation.

> 4. if range which seems right but wrong really is used such as mapping 
>virtual range [0x8800, 0x80007800) to physical area[0x2800, 
> 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

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.

-- 
tejun

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[zijun_hu]

On 2016/9/23 22:42, Tejun Heo wrote:
> Hello,
> 
> On Wed, Sep 21, 2016 at 12:19:53PM +0800, zijun_hu wrote:
>> From: zijun_hu 
>>
>> 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()
>>
>> Signed-off-by: zijun_hu 
> 
> 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.
> 
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 [0x8800, 0x80007800) to physical area[0x2800, 
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

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[zijun_hu]

On 2016/9/23 22:42, Tejun Heo wrote:
> Hello,
> 
> On Wed, Sep 21, 2016 at 12:19:53PM +0800, zijun_hu wrote:
>> From: zijun_hu 
>>
>> 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()
>>
>> Signed-off-by: zijun_hu 
> 
> 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.
> 
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 [0x8800, 0x80007800) to physical area[0x2800, 
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

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[zijun_hu]

On 2016/9/23 22:27, Michal Hocko wrote:
> On Fri 23-09-16 22:14:40, zijun_hu wrote:
>> On 2016/9/23 21:33, Michal Hocko wrote:
>>> On Fri 23-09-16 21:00:18, zijun_hu wrote:
 On 09/23/2016 08:42 PM, Michal Hocko wrote:
 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, 0x20) and [0x20,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.
>>>
>> 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 [0x_ffc08800, 0x_f800) 
>> by
>> ioremap_page_range(),ioremap_pmd_range() is called to map the range
>> finally, ioremap_pmd_range() will call
>> ioremap_pte_range(pmd, 0x_ffc08800, 0x_fffe) and
>> ioremap_pte_range(pmd, 0x_fffe, 0x f800) 
>> 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.
>
 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...
>>
>> 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.
> 
you are right
this patch is consistent with that you pointed

for map virtual range [0x8800, 0x80007800) to physical area[0x2800, 
0x20007800)
it actually map range [0x8000, 0x80008000) to physical area[0x2000, 
0x20008000)

maybe expanding range [0x8800, 0x80007800) to [0x8000, 0x80008000) is 
better than
shrinking to [0x80001000, 0x80007000) because the following reasons

1. if a user is mapping [0x8800, 0x80007800) -> [0x2800, 0x20007800), 
he/she expect to
access physical address 0x2800 by virtual address 0x8800, 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 [0x2800, 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


 

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[zijun_hu]

On 2016/9/23 22:27, Michal Hocko wrote:
> On Fri 23-09-16 22:14:40, zijun_hu wrote:
>> On 2016/9/23 21:33, Michal Hocko wrote:
>>> On Fri 23-09-16 21:00:18, zijun_hu wrote:
 On 09/23/2016 08:42 PM, Michal Hocko wrote:
 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, 0x20) and [0x20,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.
>>>
>> 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 [0x_ffc08800, 0x_f800) 
>> by
>> ioremap_page_range(),ioremap_pmd_range() is called to map the range
>> finally, ioremap_pmd_range() will call
>> ioremap_pte_range(pmd, 0x_ffc08800, 0x_fffe) and
>> ioremap_pte_range(pmd, 0x_fffe, 0x f800) 
>> 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.
>
 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...
>>
>> 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.
> 
you are right
this patch is consistent with that you pointed

for map virtual range [0x8800, 0x80007800) to physical area[0x2800, 
0x20007800)
it actually map range [0x8000, 0x80008000) to physical area[0x2000, 
0x20008000)

maybe expanding range [0x8800, 0x80007800) to [0x8000, 0x80008000) is 
better than
shrinking to [0x80001000, 0x80007000) because the following reasons

1. if a user is mapping [0x8800, 0x80007800) -> [0x2800, 0x20007800), 
he/she expect to
access physical address 0x2800 by virtual address 0x8800, 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 [0x2800, 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


 

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[Tejun Heo]

Hello,

On Wed, Sep 21, 2016 at 12:19:53PM +0800, zijun_hu wrote:
> From: zijun_hu 
> 
> 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()
> 
> Signed-off-by: zijun_hu 

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.

-- 
tejun

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[Tejun Heo]

Hello,

On Wed, Sep 21, 2016 at 12:19:53PM +0800, zijun_hu wrote:
> From: zijun_hu 
> 
> 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()
> 
> Signed-off-by: zijun_hu 

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.

-- 
tejun

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[Michal Hocko]

On Fri 23-09-16 22:14:40, zijun_hu wrote:
> On 2016/9/23 21:33, Michal Hocko wrote:
> > On Fri 23-09-16 21:00:18, zijun_hu wrote:
> >> On 09/23/2016 08:42 PM, Michal Hocko wrote:
> >> 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, 0x20) and [0x20,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.
> >
>  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 [0x_ffc08800, 0x_f800) 
>  by
>  ioremap_page_range(),ioremap_pmd_range() is called to map the range
>  finally, ioremap_pmd_range() will call
>  ioremap_pte_range(pmd, 0x_ffc08800, 0x_fffe) and
>  ioremap_pte_range(pmd, 0x_fffe, 0x f800) 
>  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.
> >>>
> >> 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...
>
> 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.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[Michal Hocko]

On Fri 23-09-16 22:14:40, zijun_hu wrote:
> On 2016/9/23 21:33, Michal Hocko wrote:
> > On Fri 23-09-16 21:00:18, zijun_hu wrote:
> >> On 09/23/2016 08:42 PM, Michal Hocko wrote:
> >> 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, 0x20) and [0x20,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.
> >
>  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 [0x_ffc08800, 0x_f800) 
>  by
>  ioremap_page_range(),ioremap_pmd_range() is called to map the range
>  finally, ioremap_pmd_range() will call
>  ioremap_pte_range(pmd, 0x_ffc08800, 0x_fffe) and
>  ioremap_pte_range(pmd, 0x_fffe, 0x f800) 
>  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.
> >>>
> >> 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...
>
> 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.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[zijun_hu]

On 2016/9/23 21:33, Michal Hocko wrote:
> On Fri 23-09-16 21:00:18, zijun_hu wrote:
>> On 09/23/2016 08:42 PM, Michal Hocko wrote:
>> 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, 0x20) and [0x20,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.
>
 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 [0x_ffc08800, 0x_f800) by
 ioremap_page_range(),ioremap_pmd_range() is called to map the range
 finally, ioremap_pmd_range() will call
 ioremap_pte_range(pmd, 0x_ffc08800, 0x_fffe) and
 ioremap_pte_range(pmd, 0x_fffe, 0x f800) 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.
>>>
>> 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...
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
> 
>> 2, the rounding up start boundary maybe cause overflow, consider start 
>> boundary =
>>0x_f800  
> 
> this is just insane
> 

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[zijun_hu]

On 2016/9/23 21:33, Michal Hocko wrote:
> On Fri 23-09-16 21:00:18, zijun_hu wrote:
>> On 09/23/2016 08:42 PM, Michal Hocko wrote:
>> 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, 0x20) and [0x20,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.
>
 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 [0x_ffc08800, 0x_f800) by
 ioremap_page_range(),ioremap_pmd_range() is called to map the range
 finally, ioremap_pmd_range() will call
 ioremap_pte_range(pmd, 0x_ffc08800, 0x_fffe) and
 ioremap_pte_range(pmd, 0x_fffe, 0x f800) 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.
>>>
>> 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...
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
> 
>> 2, the rounding up start boundary maybe cause overflow, consider start 
>> boundary =
>>0x_f800  
> 
> this is just insane
> 

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[Michal Hocko]

On Fri 23-09-16 21:00:18, zijun_hu wrote:
> On 09/23/2016 08:42 PM, Michal Hocko wrote:
>  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, 0x20) and [0x20,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.
> >>>
> >> 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 [0x_ffc08800, 0x_f800) by
> >> ioremap_page_range(),ioremap_pmd_range() is called to map the range
> >> finally, ioremap_pmd_range() will call
> >> ioremap_pte_range(pmd, 0x_ffc08800, 0x_fffe) and
> >> ioremap_pte_range(pmd, 0x_fffe, 0x f800) 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.
> > 
> 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...

> 2, the rounding up start boundary maybe cause overflow, consider start 
> boundary =
>0x_f800  

this is just insane

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[Michal Hocko]

On Fri 23-09-16 21:00:18, zijun_hu wrote:
> On 09/23/2016 08:42 PM, Michal Hocko wrote:
>  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, 0x20) and [0x20,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.
> >>>
> >> 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 [0x_ffc08800, 0x_f800) by
> >> ioremap_page_range(),ioremap_pmd_range() is called to map the range
> >> finally, ioremap_pmd_range() will call
> >> ioremap_pte_range(pmd, 0x_ffc08800, 0x_fffe) and
> >> ioremap_pte_range(pmd, 0x_fffe, 0x f800) 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.
> > 
> 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...

> 2, the rounding up start boundary maybe cause overflow, consider start 
> boundary =
>0x_f800  

this is just insane

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[zijun_hu]

On 09/23/2016 08:42 PM, Michal Hocko wrote:
 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, 0x20) and [0x20,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.
>>>
>> 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 [0x_ffc08800, 0x_f800) by
>> ioremap_page_range(),ioremap_pmd_range() is called to map the range
>> finally, ioremap_pmd_range() will call
>> ioremap_pte_range(pmd, 0x_ffc08800, 0x_fffe) and
>> ioremap_pte_range(pmd, 0x_fffe, 0x f800) 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.
> 
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 
=
   0x_f800  

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[zijun_hu]

On 09/23/2016 08:42 PM, Michal Hocko wrote:
 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, 0x20) and [0x20,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.
>>>
>> 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 [0x_ffc08800, 0x_f800) by
>> ioremap_page_range(),ioremap_pmd_range() is called to map the range
>> finally, ioremap_pmd_range() will call
>> ioremap_pte_range(pmd, 0x_ffc08800, 0x_fffe) and
>> ioremap_pte_range(pmd, 0x_fffe, 0x f800) 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.
> 
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 
=
   0x_f800  

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[Michal Hocko]

On Fri 23-09-16 20:29:20, zijun_hu wrote:
> On 2016/9/23 16:45, Michal Hocko wrote:
> > On Thu 22-09-16 23:13:17, zijun_hu wrote:
> >> On 2016/9/22 20:47, Michal Hocko wrote:
> >>> On Wed 21-09-16 12:19:53, zijun_hu wrote:
>  From: zijun_hu 
> 
>  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
> > 
> > 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

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.

[...]

> >> 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, 0x20) and [0x20,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.
> > 
> 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 [0x_ffc08800, 0x_f800) by
> ioremap_page_range(),ioremap_pmd_range() is called to map the range
> finally, ioremap_pmd_range() will call
> ioremap_pte_range(pmd, 0x_ffc08800, 0x_fffe) and
> ioremap_pte_range(pmd, 0x_fffe, 0x f800) 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.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[Michal Hocko]

On Fri 23-09-16 20:29:20, zijun_hu wrote:
> On 2016/9/23 16:45, Michal Hocko wrote:
> > On Thu 22-09-16 23:13:17, zijun_hu wrote:
> >> On 2016/9/22 20:47, Michal Hocko wrote:
> >>> On Wed 21-09-16 12:19:53, zijun_hu wrote:
>  From: zijun_hu 
> 
>  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
> > 
> > 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

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.

[...]

> >> 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, 0x20) and [0x20,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.
> > 
> 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 [0x_ffc08800, 0x_f800) by
> ioremap_page_range(),ioremap_pmd_range() is called to map the range
> finally, ioremap_pmd_range() will call
> ioremap_pte_range(pmd, 0x_ffc08800, 0x_fffe) and
> ioremap_pte_range(pmd, 0x_fffe, 0x f800) 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.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[zijun_hu]

On 2016/9/23 16:45, Michal Hocko wrote:
> On Thu 22-09-16 23:13:17, zijun_hu wrote:
>> On 2016/9/22 20:47, Michal Hocko wrote:
>>> On Wed 21-09-16 12:19:53, zijun_hu wrote:
 From: zijun_hu 

 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
> 
> 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)

 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()

 Signed-off-by: zijun_hu 
 ---
  lib/ioremap.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

 diff --git a/lib/ioremap.c b/lib/ioremap.c
 index 86c8911..911bdca 100644
 --- 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(_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).
>>>
>>   this complicate express is used to avoid addr overflow, consider map
>>   virtual rang [0xffe00800, 0xf800) 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 
>>   npig...@gmail.com 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(_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
> 
>>  
>>  @@ -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.
> 
i append the additional BUG_ON() by resembling exiting BUG_ON(addr >= end);
>>  
>>  start = addr; 
>>  phys_addr -= addr; 
>>  
  
 @@ -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.
>>>
>> 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
  
 +  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  

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[zijun_hu]

On 2016/9/23 16:45, Michal Hocko wrote:
> On Thu 22-09-16 23:13:17, zijun_hu wrote:
>> On 2016/9/22 20:47, Michal Hocko wrote:
>>> On Wed 21-09-16 12:19:53, zijun_hu wrote:
 From: zijun_hu 

 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
> 
> 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)

 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()

 Signed-off-by: zijun_hu 
 ---
  lib/ioremap.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

 diff --git a/lib/ioremap.c b/lib/ioremap.c
 index 86c8911..911bdca 100644
 --- 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(_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).
>>>
>>   this complicate express is used to avoid addr overflow, consider map
>>   virtual rang [0xffe00800, 0xf800) 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 
>>   npig...@gmail.com 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(_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
> 
>>  
>>  @@ -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.
> 
i append the additional BUG_ON() by resembling exiting BUG_ON(addr >= end);
>>  
>>  start = addr; 
>>  phys_addr -= addr; 
>>  
  
 @@ -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.
>>>
>> 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
  
 +  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, 

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[Michal Hocko]

On Thu 22-09-16 23:13:17, zijun_hu wrote:
> On 2016/9/22 20:47, Michal Hocko wrote:
> > On Wed 21-09-16 12:19:53, zijun_hu wrote:
> >> From: zijun_hu 
> >>
> >> 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

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.

> >> 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()
> >>
> >> Signed-off-by: zijun_hu 
> >> ---
> >>  lib/ioremap.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/ioremap.c b/lib/ioremap.c
> >> index 86c8911..911bdca 100644
> >> --- 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(_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).
> > 
>   this complicate express is used to avoid addr overflow, consider map
>   virtual rang [0xffe00800, 0xf800) 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 
>   npig...@gmail.com 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(_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

>  
>  @@ -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.

>  
>  start = addr; 
>  phys_addr -= addr; 
>  
> >>  
> >> @@ -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.
> > 
> 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
> >>  
> >> +  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, 0x20) and [0x20,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.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[Michal Hocko]

On Thu 22-09-16 23:13:17, zijun_hu wrote:
> On 2016/9/22 20:47, Michal Hocko wrote:
> > On Wed 21-09-16 12:19:53, zijun_hu wrote:
> >> From: zijun_hu 
> >>
> >> 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

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.

> >> 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()
> >>
> >> Signed-off-by: zijun_hu 
> >> ---
> >>  lib/ioremap.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/ioremap.c b/lib/ioremap.c
> >> index 86c8911..911bdca 100644
> >> --- 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(_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).
> > 
>   this complicate express is used to avoid addr overflow, consider map
>   virtual rang [0xffe00800, 0xf800) 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 
>   npig...@gmail.com 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(_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

>  
>  @@ -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.

>  
>  start = addr; 
>  phys_addr -= addr; 
>  
> >>  
> >> @@ -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.
> > 
> 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
> >>  
> >> +  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, 0x20) and [0x20,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.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[zijun_hu]

On 2016/9/22 20:47, Michal Hocko wrote:
> On Wed 21-09-16 12:19:53, zijun_hu wrote:
>> From: zijun_hu 
>>
>> 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
>> 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()
>>
>> Signed-off-by: zijun_hu 
>> ---
>>  lib/ioremap.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/ioremap.c b/lib/ioremap.c
>> index 86c8911..911bdca 100644
>> --- 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(_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).
> 
  this complicate express is used to avoid addr overflow, consider map
  virtual rang [0xffe00800, 0xf800) 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 
  npig...@gmail.com 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(_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; 
 
>>  
>> @@ -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.
> 
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
>>  
>> +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, 0x20) and [0x20,0x208800) and map them separately
the first range will cause dead loop
>>  start = addr;
>>  phys_addr -= addr;
>>  pgd = pgd_offset_k(addr);
>> -- 
>> 1.9.1
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majord...@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 
> 

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[zijun_hu]

On 2016/9/22 20:47, Michal Hocko wrote:
> On Wed 21-09-16 12:19:53, zijun_hu wrote:
>> From: zijun_hu 
>>
>> 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
>> 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()
>>
>> Signed-off-by: zijun_hu 
>> ---
>>  lib/ioremap.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/ioremap.c b/lib/ioremap.c
>> index 86c8911..911bdca 100644
>> --- 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(_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).
> 
  this complicate express is used to avoid addr overflow, consider map
  virtual rang [0xffe00800, 0xf800) 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 
  npig...@gmail.com 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(_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; 
 
>>  
>> @@ -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.
> 
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
>>  
>> +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, 0x20) and [0x20,0x208800) and map them separately
the first range will cause dead loop
>>  start = addr;
>>  phys_addr -= addr;
>>  pgd = pgd_offset_k(addr);
>> -- 
>> 1.9.1
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majord...@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 
> 

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[Michal Hocko]

On Wed 21-09-16 12:19:53, zijun_hu wrote:
> From: zijun_hu 
> 
> 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?

> 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()
> 
> Signed-off-by: zijun_hu 
> ---
>  lib/ioremap.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/ioremap.c b/lib/ioremap.c
> index 86c8911..911bdca 100644
> --- 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(_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).

>  
> @@ -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.

>  
> + addr = round_down(addr, PAGE_SIZE);

end = round_up(end, PAGE_SIZE);

wouldn't work?

>   start = addr;
>   phys_addr -= addr;
>   pgd = pgd_offset_k(addr);
> -- 
> 1.9.1
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majord...@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[Michal Hocko]

On Wed 21-09-16 12:19:53, zijun_hu wrote:
> From: zijun_hu 
> 
> 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?

> 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()
> 
> Signed-off-by: zijun_hu 
> ---
>  lib/ioremap.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/ioremap.c b/lib/ioremap.c
> index 86c8911..911bdca 100644
> --- 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(_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).

>  
> @@ -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.

>  
> + addr = round_down(addr, PAGE_SIZE);

end = round_up(end, PAGE_SIZE);

wouldn't work?

>   start = addr;
>   phys_addr -= addr;
>   pgd = pgd_offset_k(addr);
> -- 
> 1.9.1
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majord...@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 

-- 
Michal Hocko
SUSE Labs

[PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[zijun_hu]

From: zijun_hu 

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()

Signed-off-by: zijun_hu 
---
 lib/ioremap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/ioremap.c b/lib/ioremap.c
index 86c8911..911bdca 100644
--- 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(_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);
-- 
1.9.1

[PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

[zijun_hu]

From: zijun_hu 

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()

Signed-off-by: zijun_hu 
---
 lib/ioremap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/ioremap.c b/lib/ioremap.c
index 86c8911..911bdca 100644
--- 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(_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);
-- 
1.9.1