Re: crypto: sun4i-ss: error with kmap

[Thomas Gleixner <tglx@linutronix.de>]

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