Hi Geert,
Am 22.05.2018 um 20:18 schrieb Geert Uytterhoeven:
Hi Michael,
On Mon, May 14, 2018 at 1:10 PM, Michael Schmitz <schmitzmic@xxxxxxxxx> wrote:
On kernels with 020/030 support ...
get_io_area leaves an IO_SIZE gap between mappings which is added to
the vm_struct representing the mapping. __ioremap() uses the actual
requested size (after alignment), while __iounmap() is passed the
size from the vm_struct.
On 020/030, early termination descriptors are used to set up mappings of
extent 'size', which are validated on unmapping. The unmapped gap of size
IO_SIZE defeats the sanity check of the pmd tables, causing __iounmap to
loop forever on 030.
On 040/040, unmapping of page table entries does not check for a valid
040/060
My bad ...
mapping, so the umapping loop always completes there.
Adjust size to be unmapped by the gap that had been added in the vm_struct
prior.
This fixes the hang in atari_platform_init() reported a long time ago, and
a similar one reported by Finn recently (addressed by removing ioremap()
use from the SWIM driver.
Tested on my Falcon in 030 mode - untested but should work the same on
040/060 (the extra page tables cleared there would never have been set up
anyway).
Comment on whether the gap size should be considered in looking for a
suitable address to place the next mapping in get_io_area() would be welcome.
At first sight (and looking in full-history-linux git history), I see no
reason for the gap. I'd assume having a block with address and size aligned
to 256 KiB (which the caller already takes care of: IO_SIZE is 256 KiB if
020/030 support is enabled) should be sufficient to use early termination
tables.
My guess is that someone wanted to catch out of bounds accesses by
leaving the unmapped areas in between ioremapped 256 kB chunks. The
unmapped gaps must 256 kB as well to avoid disturbing the alignment.
The adjustment for gap size was dropped sometime between 2.4.30 and
2.6.18. At the same time, a comment in get_io_area was removed that
stated 'leave a gap of IO_SIZE'. I haven't looked at the full history to
find out who removed that comment (and the adjustment).
What I hoped for comments on is this: when searching for an available
unmapped chunk in get_io_area(), the minimum size requirement is 'size',
not 'size+IO_SIZE'. If we find an area that just fits the minimum size,
can we end up with no gap between the new mapping and the following one?
As far as I now can see, what get_io_area() does is safe.
Obviously removing the gap would fix the issue as well, but that's more
risky...
Yep, even though the code doesn't seem to get used a lot I'd rather not
audit all callers.
Signed-off-by: Michael Schmitz <schmitzmic@xxxxxxxxx>
---
arch/m68k/mm/kmap.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/arch/m68k/mm/kmap.c b/arch/m68k/mm/kmap.c
index c2a3832..3b420f6 100644
--- a/arch/m68k/mm/kmap.c
+++ b/arch/m68k/mm/kmap.c
@@ -89,7 +89,8 @@ static inline void free_io_area(void *addr)
for (p = &iolist ; (tmp = *p) ; p = &tmp->next) {
if (tmp->addr == addr) {
*p = tmp->next;
- __iounmap(tmp->addr, tmp->size);
+ /* remove gap added in get_io_area() */
+ __iounmap(tmp->addr, tmp->size - IO_SIZE);
kfree(tmp);
return;
}
Looks good to me, so I will apply and queue for v4.18 with the patch
description slightly modified, but will wait one more day, in case someone
has a comment.
Thanks, let's see whether this helps some 030 users ...
Cheers,
Michael
Thanks a lot!
Gr{oetje,eeting}s,
Geert
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
