kmap_atomic() oopses in current mainline

kmap_atomic() oopses in current mainline

[Andrew Morton]

I've had a handful of random mystery oopses associated with no particular
activity.  A typical trace is at:

	http://userweb.kernel.org/~akpm/dsc03659.jpg

the trace is messy and it doesn't seem to want to happen now I've turned on
frame pointers, but it looks networky to me.

So if anyone has made kmap_atomic() changes in networking recently, please
check your work.  The machine does have highmem.



The crash appears to be happening here:

        BUG_ON(!pte_none(*(kmap_pte-idx)));

which would indicate a wild value is being passing km kmap_atomic()'s
km_type arg.




I don't think the 2-year-old Vaio has offload engine support ;) Dan, this:

+		if (flags & ASYNC_TX_KMAP_DST)
+			dest_buf = kmap_atomic(dest, KM_USER0) + dest_offset;
+		else
+			dest_buf = page_address(dest) + dest_offset;
+
+		if (flags & ASYNC_TX_KMAP_SRC)
+			src_buf = kmap_atomic(src, KM_USER0) + src_offset;
+		else
+			src_buf = page_address(src) + src_offset;
+
+		memcpy(dest_buf, src_buf, len);
+
+		if (flags & ASYNC_TX_KMAP_DST)
+			kunmap_atomic(dest_buf, KM_USER0);
+
+		if (flags & ASYNC_TX_KMAP_SRC)
+			kunmap_atomic(src_buf, KM_USER0);
+

is very wrong if both ASYNC_TX_KMAP_DST and ASYNC_TX_KMAP_SRC can ever be
set.  We'll end up using the same kmap slot for both src add dest and we
get either corrupted data or a BUG.

Re: kmap_atomic() oopses in current mainline

[Evgeniy Polyakov]

Hi.

On Thu, Jul 19, 2007 at 01:33:04AM -0700, Andrew Morton (akpm@linux-foundation.org) wrote:
> I don't think the 2-year-old Vaio has offload engine support ;) Dan, this:
> 
> +		if (flags & ASYNC_TX_KMAP_DST)
> +			dest_buf = kmap_atomic(dest, KM_USER0) + dest_offset;
> +		else
> +			dest_buf = page_address(dest) + dest_offset;
> +
> +		if (flags & ASYNC_TX_KMAP_SRC)
> +			src_buf = kmap_atomic(src, KM_USER0) + src_offset;
> +		else
> +			src_buf = page_address(src) + src_offset;
> +
> +		memcpy(dest_buf, src_buf, len);
> +
> +		if (flags & ASYNC_TX_KMAP_DST)
> +			kunmap_atomic(dest_buf, KM_USER0);
> +
> +		if (flags & ASYNC_TX_KMAP_SRC)
> +			kunmap_atomic(src_buf, KM_USER0);
> +
> 
> is very wrong if both ASYNC_TX_KMAP_DST and ASYNC_TX_KMAP_SRC can ever be
> set.  We'll end up using the same kmap slot for both src add dest and we
> get either corrupted data or a BUG.

So far it can not since the only user is raid code, which only allows to
perform either reading from bio or writing into one, which requires only
one mapping.

Btw, shouldn't it always be kmap_atomic() even if flag is not set.
That pages are usual one returned by alloc_page().
 

-- 
	Evgeniy Polyakov

Re: kmap_atomic() oopses in current mainline

[Andrew Morton]

On Thu, 19 Jul 2007 13:28:56 +0400 Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:

> Hi.
> 
> On Thu, Jul 19, 2007 at 01:33:04AM -0700, Andrew Morton (akpm@linux-foundation.org) wrote:
> > I don't think the 2-year-old Vaio has offload engine support ;) Dan, this:
> > 
> > +		if (flags & ASYNC_TX_KMAP_DST)
> > +			dest_buf = kmap_atomic(dest, KM_USER0) + dest_offset;
> > +		else
> > +			dest_buf = page_address(dest) + dest_offset;
> > +
> > +		if (flags & ASYNC_TX_KMAP_SRC)
> > +			src_buf = kmap_atomic(src, KM_USER0) + src_offset;
> > +		else
> > +			src_buf = page_address(src) + src_offset;
> > +
> > +		memcpy(dest_buf, src_buf, len);
> > +
> > +		if (flags & ASYNC_TX_KMAP_DST)
> > +			kunmap_atomic(dest_buf, KM_USER0);
> > +
> > +		if (flags & ASYNC_TX_KMAP_SRC)
> > +			kunmap_atomic(src_buf, KM_USER0);
> > +
> > 
> > is very wrong if both ASYNC_TX_KMAP_DST and ASYNC_TX_KMAP_SRC can ever be
> > set.  We'll end up using the same kmap slot for both src add dest and we
> > get either corrupted data or a BUG.
> 
> So far it can not since the only user is raid code, which only allows to
> perform either reading from bio or writing into one, which requires only
> one mapping.

hm, so we got lucky?

> Btw, shouldn't it always be kmap_atomic() even if flag is not set.
> That pages are usual one returned by alloc_page().

The code would work OK if the kmap_atomic()s were unconditional, but it
would be a bit more expensive if the page is in highmem and we don't
actually intend to access it with the CPU.

kmap_atomic() against a non-highmem page is basically free: just an
additional test_bit().

Re: kmap_atomic() oopses in current mainline

[Evgeniy Polyakov]

On Thu, Jul 19, 2007 at 02:38:31AM -0700, Andrew Morton (akpm@linux-foundation.org) wrote:
> > > is very wrong if both ASYNC_TX_KMAP_DST and ASYNC_TX_KMAP_SRC can ever be
> > > set.  We'll end up using the same kmap slot for both src add dest and we
> > > get either corrupted data or a BUG.
> > 
> > So far it can not since the only user is raid code, which only allows to
> > perform either reading from bio or writing into one, which requires only
> > one mapping.
> 
> hm, so we got lucky?

I would say it was intentionally, current code can perform only one
operation in a time. Of course changing KM_USER from 0 to 1 in second
kmap_atomic will not force oceans to run out of coasts.

Kind of:

diff --git a/crypto/async_tx/async_memcpy.c b/crypto/async_tx/async_memcpy.c
index a973f4e..a48c7f3 100644
--- a/crypto/async_tx/async_memcpy.c
+++ b/crypto/async_tx/async_memcpy.c
@@ -94,7 +94,7 @@ async_memcpy(struct page *dest, struct page *src, unsigned int dest_offset,
 			dest_buf = page_address(dest) + dest_offset;
 
 		if (flags & ASYNC_TX_KMAP_SRC)
-			src_buf = kmap_atomic(src, KM_USER0) + src_offset;
+			src_buf = kmap_atomic(src, KM_USER1) + src_offset;
 		else
 			src_buf = page_address(src) + src_offset;
 
@@ -104,7 +104,7 @@ async_memcpy(struct page *dest, struct page *src, unsigned int dest_offset,
 			kunmap_atomic(dest_buf, KM_USER0);
 
 		if (flags & ASYNC_TX_KMAP_SRC)
-			kunmap_atomic(src_buf, KM_USER0);
+			kunmap_atomic(src_buf, KM_USER1);
 
 		async_tx_sync_epilog(flags, depend_tx, cb_fn, cb_param);
 	}

> > Btw, shouldn't it always be kmap_atomic() even if flag is not set.
> > That pages are usual one returned by alloc_page().
> 
> The code would work OK if the kmap_atomic()s were unconditional, but it
> would be a bit more expensive if the page is in highmem and we don't
> actually intend to access it with the CPU.
> 
> kmap_atomic() against a non-highmem page is basically free: just an
> additional test_bit().

As far as I recall there was an intention to do async memory copy to
userspace, so likely kmapping is a good idea.

-- 
	Evgeniy Polyakov

Re: kmap_atomic() oopses in current mainline

[Dan Williams]

On 7/19/07, Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:
> On Thu, Jul 19, 2007 at 02:38:31AM -0700, Andrew Morton (akpm@linux-foundation.org) wrote:
> > > > is very wrong if both ASYNC_TX_KMAP_DST and ASYNC_TX_KMAP_SRC can ever be
> > > > set.  We'll end up using the same kmap slot for both src add dest and we
> > > > get either corrupted data or a BUG.
> > >
> > > So far it can not since the only user is raid code, which only allows to
> > > perform either reading from bio or writing into one, which requires only
> > > one mapping.
> >
> > hm, so we got lucky?
>
> I would say it was intentionally, current code can perform only one
> operation in a time. Of course changing KM_USER from 0 to 1 in second
> kmap_atomic will not force oceans to run out of coasts.
>
> Kind of:
>
> diff --git a/crypto/async_tx/async_memcpy.c b/crypto/async_tx/async_memcpy.c
> index a973f4e..a48c7f3 100644
> --- a/crypto/async_tx/async_memcpy.c
> +++ b/crypto/async_tx/async_memcpy.c
> @@ -94,7 +94,7 @@ async_memcpy(struct page *dest, struct page *src, unsigned int dest_offset,
>                         dest_buf = page_address(dest) + dest_offset;
>
>                 if (flags & ASYNC_TX_KMAP_SRC)
> -                       src_buf = kmap_atomic(src, KM_USER0) + src_offset;
> +                       src_buf = kmap_atomic(src, KM_USER1) + src_offset;
>                 else
>                         src_buf = page_address(src) + src_offset;
>
> @@ -104,7 +104,7 @@ async_memcpy(struct page *dest, struct page *src, unsigned int dest_offset,
>                         kunmap_atomic(dest_buf, KM_USER0);
>
>                 if (flags & ASYNC_TX_KMAP_SRC)
> -                       kunmap_atomic(src_buf, KM_USER0);
> +                       kunmap_atomic(src_buf, KM_USER1);
>
>                 async_tx_sync_epilog(flags, depend_tx, cb_fn, cb_param);
>         }
>
> > > Btw, shouldn't it always be kmap_atomic() even if flag is not set.
> > > That pages are usual one returned by alloc_page().
> >
> > The code would work OK if the kmap_atomic()s were unconditional, but it
> > would be a bit more expensive if the page is in highmem and we don't
> > actually intend to access it with the CPU.
> >
> > kmap_atomic() against a non-highmem page is basically free: just an
> > additional test_bit().
>
Always kmap'ing the page is the way to go, since in this path the page
is always accessed with the CPU.  This also allows these ASYNC_TX_
flags to be killed off as they are not necessary.  I'll cook up a
patch, and be more careful about my kmap usage going forward.

> As far as I recall there was an intention to do async memory copy to
> userspace, so likely kmapping is a good idea.
>
> --
>         Evgeniy Polyakov

Thanks,
Dan