On Mon, Dec 07 2020 at 13:18, Corentin Labbe wrote:
> On Mon, Dec 07, 2020 at 01:15:49AM +0100, Thomas Gleixner wrote:
> So if I understand correctly, basicly I cannot have two atomic kmap at
> the same time since it made unmapping them in the right order complex.
You can, but the ordering has to be correct and with sg_miter that's
probably hard to get right.
> I am not sure to have well understood your hint, but could you give me
So the point is:
sg_miter_next(&mi); map 1 -> vaddr1
sg_miter_next(&mo); map 2 -> vaddr2
do {
...
if (cond) {
sg_miter_next(&mi)
sg_miter_stop()
unmap(vaddr1); unmaps map2 -> FAIL
if (next_page)
map(); maps map2 -> vaddr2 -> FAIL
}
The only way how that could have ever worked is when the conditional
sg_miter_next(&mi) did not try to map a new page, i.e. end of data.
The ARM kunmap_atomic() had:
#ifdef CONFIG_DEBUG_HIGHMEM
BUG_ON(vaddr != __fix_to_virt(idx));
set_fixmap_pte(idx, __pte(0));
#else
which means the warning and clearing the PTE only happens when debugging
is enabled. That made your code "work" by chance because the unmap
leaves map2 intact which means the vaddr2 mapping stays valid, so the
access to it further down still worked.
sg_miter_next(&mi); map 1 -> vaddr1
sg_miter_next(&mo); map 2 -> vaddr2
do {
...
if (cond) {
sg_miter_next(&mi)
sg_miter_stop()
unmap(vaddr1); idx 2 ---> 1
but mapping still valid for vaddr2
}
*vaddr2 = x; works by chance
But that also would cause trouble in the following case:
sg_miter_next(&mi); map 1 -> vaddr1
sg_miter_next(&mo); map 2 -> vaddr2
do {
...
if (cond) {
sg_miter_next(&mi)
sg_miter_stop()
unmap(vaddr1); idx 2 ---> 1
but mapping still valid for vaddr2
}
interrupt
kmap_atomic(some_other_page)
idx 1 -> 2 map some_otherpage to vaddr2
kunmap_atomic(vaddr2) idx 2 ---> 1
mapping still valid for vaddr2,
but now points to some_other_page
end of interrupt
*vaddr2 = x; <-- accesses some_other_page -> FAIL
This is the worst one because it's random data corruption and extremly
hard to debug.
I made the warning and the pte clearing in the new code unconditional
just to catch any issues upfront which it did.
sg_miter_next(&mi); map 1 -> vaddr1
sg_miter_next(&mo); map 2 -> vaddr2
do {
...
if (cond) {
sg_miter_next(&mi)
sg_miter_stop()
unmap(vaddr1); unmaps map2 -> FAIL
clear map2 invalidates vaddr2
}
*vaddr2 = x; <-- accesses the unmapped vaddr2 -> CRASH
> what you think about the following patch which fix (at least) the
> crash. Instead of holding SGmiter (and so two kmap), I use only one
> at a time.
That looks fine at least vs. the sg_miter/kmap_atomic usage.
Thanks,
tglx
