[PATCH] Fix incorrect kunmap_atomic in pktcdvd

[PATCH] Fix incorrect kunmap_atomic in pktcdvd

[Peter Osterlund]

The pktcdvd driver uses kunmap_atomic() incorrectly. The function is
supposed to take an address as the first parameter, but the pktcdvd
driver passed a page pointer. Thanks to Douglas Gilbert and Jens Axboe
for discovering this.

Signed-off-by: Peter Osterlund <petero2@telia.com>
---

 linux-petero/drivers/block/pktcdvd.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff -puN drivers/block/pktcdvd.c~packet-kmap-fix drivers/block/pktcdvd.c
--- linux/drivers/block/pktcdvd.c~packet-kmap-fix	2004-10-23 12:04:01.000000000 +0200
+++ linux-petero/drivers/block/pktcdvd.c	2004-10-23 12:07:11.000000000 +0200
@@ -621,7 +621,7 @@ static void pkt_copy_bio_data(struct bio
 
 		BUG_ON(len < 0);
 		memcpy(vto, vfrom, len);
-		kunmap_atomic(src_bvl->bv_page, KM_USER0);
+		kunmap_atomic(vfrom, KM_USER0);
 
 		seg++;
 		offs = 0;
@@ -649,7 +649,7 @@ static void pkt_make_local_copy(struct p
 			void *vfrom = kmap_atomic(pages[f], KM_USER0) + offsets[f];
 			void *vto = page_address(pkt->pages[p]) + offs;
 			memcpy(vto, vfrom, CD_FRAMESIZE);
-			kunmap_atomic(pages[f], KM_USER0);
+			kunmap_atomic(vfrom, KM_USER0);
 			pages[f] = pkt->pages[p];
 			offsets[f] = offs;
 		} else {
_

-- 
Peter Osterlund - petero2@telia.com
http://w1.894.telia.com/~u89404340

Re: [PATCH] Fix incorrect kunmap_atomic in pktcdvd

[Andrew Morton]

Peter Osterlund <petero2@telia.com> wrote:
>
>  The pktcdvd driver uses kunmap_atomic() incorrectly. The function is
>  supposed to take an address as the first parameter, but the pktcdvd
>  driver passed a page pointer. Thanks to Douglas Gilbert and Jens Axboe
>  for discovering this.

You're about the 7,000th person to make that mistake.  We really should
catch it via typechecking but the code's really lame and nobody ever got
around to rotorooting it.

Re: [PATCH] Fix incorrect kunmap_atomic in pktcdvd

[Peter Osterlund]

Andrew Morton <akpm@osdl.org> writes:

> Peter Osterlund <petero2@telia.com> wrote:
> >
> >  The pktcdvd driver uses kunmap_atomic() incorrectly. The function is
> >  supposed to take an address as the first parameter, but the pktcdvd
> >  driver passed a page pointer. Thanks to Douglas Gilbert and Jens Axboe
> >  for discovering this.
> 
> You're about the 7,000th person to make that mistake.  We really should
> catch it via typechecking but the code's really lame and nobody ever got
> around to rotorooting it.

Why was the interface made different from kmap()/kunmap() in the first
place? Wouldn't it have made more sense to let kunmap_atomic() take a
page pointer as the first parameter?

-- 
Peter Osterlund - petero2@telia.com
http://w1.894.telia.com/~u89404340

Re: [PATCH] Fix incorrect kunmap_atomic in pktcdvd

[Andrew Morton]

Peter Osterlund <petero2@telia.com> wrote:
>
> Andrew Morton <akpm@osdl.org> writes:
> 
> > Peter Osterlund <petero2@telia.com> wrote:
> > >
> > >  The pktcdvd driver uses kunmap_atomic() incorrectly. The function is
> > >  supposed to take an address as the first parameter, but the pktcdvd
> > >  driver passed a page pointer. Thanks to Douglas Gilbert and Jens Axboe
> > >  for discovering this.
> > 
> > You're about the 7,000th person to make that mistake.  We really should
> > catch it via typechecking but the code's really lame and nobody ever got
> > around to rotorooting it.
> 
> Why was the interface made different from kmap()/kunmap() in the first
> place? Wouldn't it have made more sense to let kunmap_atomic() take a
> page pointer as the first parameter?

No, kmap-atomic() maps a single page into the CPU's address space by making
a pte point at the page.  To unmap that page we need to get at the pte, not
at the page.  If kmap_atomic() were to take a pageframe address we'd need
to search the whole fixmap space for the corresponding page - a reverse
lookup.

Re: [PATCH] Fix incorrect kunmap_atomic in pktcdvd

[William Lee Irwin III]

Peter Osterlund <petero2@telia.com> wrote:
>> Why was the interface made different from kmap()/kunmap() in the first
>> place? Wouldn't it have made more sense to let kunmap_atomic() take a
>> page pointer as the first parameter?

On Sun, Oct 24, 2004 at 04:18:27AM -0700, Andrew Morton wrote:
> No, kmap-atomic() maps a single page into the CPU's address space by making
> a pte point at the page.  To unmap that page we need to get at the pte, not
> at the page.  If kmap_atomic() were to take a pageframe address we'd need
> to search the whole fixmap space for the corresponding page - a reverse
> lookup.

I don't recall anything truly ancient, but fixmap indices should be
enough to recover the virtual address and pte. I think the virtual
address is primarily for checking purposes. The same kind of check
could be done by checking the pfn derived from the page structure
against the contents of the pte at the fixmap index, but I suspect
more damage would ensue from changing the calling convention than
aligning it with common expectations.


-- wli