Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
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
In-Reply-To: <20201206214053.GA8458@Red>
References: <20201202195501.GA29296@Red> <877dpzexfr.fsf@nanos.tec.linutronix.de> <20201203173846.GA16207@Red> <87r1o6bh1u.fsf@nanos.tec.linutronix.de> <20201204132631.GA25321@Red> <874kl1bod0.fsf@nanos.tec.linutronix.de> <20201204192753.GA19782@Red> <87wnxx9tle.fsf@nanos.tec.linutronix.de> <20201205184334.GA8034@Red> <87mtys8268.fsf@nanos.tec.linutronix.de> <20201206214053.GA8458@Red>
Date:   Mon, 07 Dec 2020 01:15:49 +0100
Message-ID: <87ft4i79oq.fsf@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Sun, Dec 06 2020 at 22:40, Corentin Labbe wrote:
> On Sat, Dec 05, 2020 at 08:48:15PM +0100, Thomas Gleixner wrote:
>> So this maps two pages and unmaps the first one. That's all called from
>> sun4i_ss_opti_poll() and the bug is clearly visible there:
>> 
>> 	sg_miter_next(&mi);
>> 	sg_miter_next(&mo);
>> 
>> release_ss:
>> 	sg_miter_stop(&mi);
>> 	sg_miter_stop(&mo);
>> 
>> Written by yourself :) Same issue in sun4i_ss_cipher_poll()
>> 
>> Fix below.
>> 
>
> Unfortunatly, the crash still happen with the fix.
> See http://kernel.montjoie.ovh/131321.log

And why are you not looking for the reason of this problem in your own
code yourself? It's not a regression caused by my work.

Turn on CONFIG_DEBUG_HIGHMEM on 5.10-rcX or older kernels and you will
get the very same crashes. My work just made these checks unconditional.

This was broken forever and it's not my problem that you did not enable
mandatory debug options when developing this thing.

I gave you tons of hints by now how to debug this and what to look
for. Obviously I overlooked something and here is the final hint:

 	sg_miter_next(&mi);
 	sg_miter_next(&mo);

        do {
           ....
           if (cond1)
               sg_miter_next(&mi);      <--- HINT
           ....
           if (cond2)
               sg_miter_next(&mo);
 
release_ss:
 	sg_miter_stop(&mi);
 	sg_miter_stop(&mo);

So yes, I overlooked the obvious, but as I said above it's not something
which my is failing due to my changes. It was broken forever, it just
was not tested properly. Don't blame the messenger.

My knowledge about how to use nested sg_miter correctly is close to
zero. I can and did explain you the rules of kmap_atomic/local() but
that's it.

Thanks,

        tglx