linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Corentin Labbe <clabbe.montjoie@gmail.com>
Cc: herbert@gondor.apana.org.au, mripard@kernel.org, wens@csie.org,
	linux-arm-kernel@lists.infradead.org,
	linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
	Jens Axboe <axboe@kernel.dk>,
	linux-mm@kvack.org, Andrew Morton <akpm@linuxfoundation.org>,
	Julia Lawall <julia.lawall@lip6.fr>,
	Matthew Wilcox <willy@infradead.org>
Subject: Re: crypto: sun4i-ss: error with kmap
Date: Mon, 07 Dec 2020 16:53:23 +0100	[thread overview]
Message-ID: <87o8j562a4.fsf@nanos.tec.linutronix.de> (raw)
In-Reply-To: <20201207121820.GB8458@Red>

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


      reply	other threads:[~2020-12-07 15:53 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20201201130102.GA23461@Red>
     [not found] ` <87ft4phcyx.fsf@nanos.tec.linutronix.de>
     [not found]   ` <20201201135252.GA9584@Red>
     [not found]     ` <87y2ihfw6z.fsf@nanos.tec.linutronix.de>
     [not found]       ` <20201201144529.GA6786@Red>
     [not found]         ` <87v9dlfthf.fsf@nanos.tec.linutronix.de>
     [not found]           ` <20201202195501.GA29296@Red>
     [not found]             ` <877dpzexfr.fsf@nanos.tec.linutronix.de>
     [not found]               ` <20201203173846.GA16207@Red>
2020-12-03 23:34                 ` crypto: sun4i-ss: error with kmap Thomas Gleixner
2020-12-04 13:26                   ` Corentin Labbe
2020-12-04 15:08                     ` Thomas Gleixner
2020-12-04 19:27                       ` Corentin Labbe
2020-12-04 20:58                         ` Thomas Gleixner
2020-12-05 18:43                           ` Corentin Labbe
2020-12-05 19:48                             ` Thomas Gleixner
2020-12-05 20:16                               ` Julia Lawall
2020-12-06 21:40                               ` Corentin Labbe
2020-12-07  0:15                                 ` Thomas Gleixner
2020-12-07 12:18                                   ` Corentin Labbe
2020-12-07 15:53                                     ` Thomas Gleixner [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87o8j562a4.fsf@nanos.tec.linutronix.de \
    --to=tglx@linutronix.de \
    --cc=akpm@linuxfoundation.org \
    --cc=axboe@kernel.dk \
    --cc=clabbe.montjoie@gmail.com \
    --cc=herbert@gondor.apana.org.au \
    --cc=julia.lawall@lip6.fr \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mripard@kernel.org \
    --cc=wens@csie.org \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).