On Sat, 5 Dec 2020, Thomas Gleixner wrote: > Corentin, > > On Sat, Dec 05 2020 at 19:43, Corentin Labbe wrote: > > On Fri, Dec 04, 2020 at 09:58:21PM +0100, Thomas Gleixner wrote: > >> Can you please replace the debug patch with the one below and try again? > >> That stops the trace right on the condition. > > > > Hello, the result could be found at http://kernel.montjoie.ovh/130739.log > > Thanks for providing this. This is clearly showing where stuff goes > wrong. It starts here at 729.550001. I removed the uninteresting parts: > > 0d..2 147103293us : __kmap_local_page_prot <-sg_miter_next > 0d..3 147103308us :__kmap_local_pfn_prot: kmap_local_pfn: 1 ffefd000 > > 0d..3 147103311us : __kmap_local_page_prot <-sg_miter_next > 0d..4 147103325us : __kmap_local_pfn_prot: kmap_local_pfn: 3 ffefb000 > > 0d..3 147103429us : kunmap_local_indexed <-sg_miter_stop > 0d..4 147103433us : kunmap_local_indexed: kunmap_local: 3 ffefd000 > > 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. > > Julia, it might be worth to have a coccinelle check for that. It's the > only place which got it wrong, but this goes unnoticed when code is > e.g. only fully tested on 64bit or as in this case never tested with > full debugging enabled. The whole kmap_atomic and kmap_local (new in > next) family and all users like the sg_miter stuff are affected by this. OK, thanks for the suggestion. I will look into it. julia > > Thanks, > > tglx > --- > Subject: crypto: sun4i-ss - Fix sg_miter_stop() ordering > From: Thomas Gleixner <tglx@xxxxxxxxxxxxx> > Date: Sat, 05 Dec 2020 20:17:28 +0100 > > sun4i_ss_opti_poll() and sun4i_ss_cipher_poll() do: > > sg_miter_next(&mi); > sg_miter_next(&mo); > ... > sg_miter_stop(&mi); > sg_miter_stop(&mo); > > which is the wrong order because sg_miter_next() maps a page with > kmap_atomic() and sg_miter_stop() unmaps it. kmap_atomic() uses a stack > internaly which requires that the nested map is unmapped first. As this > uses the wrong order it triggers the warning in kunmap_local_indexed() > which checks the to be unmapped address and subsequently crashes. > > This went unnoticed for 5 years because the ARM kmap_atomic() > implementation had the warning conditional on CONFIG_DEBUG_HIGHMEM which > was obviously never enabled when testing that driver. > > Flip the order to cure it. > > Reported-by: Corentin Labbe <clabbe.montjoie@xxxxxxxxx> > Fixes: 6298e948215f ("crypto: sunxi-ss - Add Allwinner Security System crypto accelerator") > Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx> > Cc: stable@xxxxxxxxxxxxxxx > --- > drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > --- a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c > +++ b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c > @@ -109,8 +109,8 @@ static int noinline_for_stack sun4i_ss_o > } > > release_ss: > - sg_miter_stop(&mi); > sg_miter_stop(&mo); > + sg_miter_stop(&mi); > writel(0, ss->base + SS_CTL); > spin_unlock_irqrestore(&ss->slock, flags); > return err; > @@ -333,8 +333,8 @@ static int sun4i_ss_cipher_poll(struct s > } > > release_ss: > - sg_miter_stop(&mi); > sg_miter_stop(&mo); > + sg_miter_stop(&mi); > writel(0, ss->base + SS_CTL); > spin_unlock_irqrestore(&ss->slock, flags); > > > >
