Skip to contentSkip to conversation

ps3disk: passing wrong variable to bvec_kunmap_irq()

PATCH Applied Dan Carpenter 4 participants 6 messages Oct 11, 2010 – Oct 17, 2010

Subsystems (4)
PowerPC Block layer Kernel janitors PS3 platform support
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 ps3disk: passing wrong variable to bvec_kunmap_irq()

This should pass "buf" to bvec_kunmap_irq() instead of "bv". The api is like kmap_atomic() instead of kmap().

Signed-off-by Dan Carpenter <[email protected]>
drivers/block/ps3disk.c +1 -1
index e9da874..03688c2 100644
@@ -113,7 +113,7 @@ static void ps3disk_scatter_gather(struct ps3_storage_device *dev,
 			memcpy(buf, dev->bounce_buf+offset, size);
 		offset += size;
 		flush_kernel_dcache_page(bvec->bv_page);
-		bvec_kunmap_irq(bvec, &flags);
+		bvec_kunmap_irq(buf, &flags);
 		i++;
 	}
 }
Applied
#1 lore raw
Quoted 10 lines · Dan Carpenter

This should pass "buf" to bvec_kunmap_irq() instead of "bv". The api is like kmap_atomic() instead of kmap().

Signed-off-by Dan Carpenter <[email protected]>
drivers/block/ps3disk.c +0 -0
index e9da874..03688c2 100644

drivers/block/ps3disk.c static void ps3disk_scatter_gather(struct ps3_storage_device *dev, · line 119

Quoted 8 lines
drivers/block/ps3disk.c +1 -1
@@ -113,7 +113,7 @@ static void ps3disk_scatter_gather(struct ps3_storage_device *dev,
 			memcpy(buf, dev->bounce_buf+offset, size);
 		offset += size;
 		flush_kernel_dcache_page(bvec->bv_page);
-		bvec_kunmap_irq(bvec, &flags);
+		bvec_kunmap_irq(buf, &flags);
 		i++;
 	}
 }

Thanks applied, that bug is all too common.

Signature · 1 line
Jens Axboe
#2 lore raw
In reply to Jens Axboe
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Dan Carpenter

This should pass "buf" to bvec_kunmap_irq() instead of "bv".  The api is like kmap_atomic() instead of kmap().

Signed-off-by Dan Carpenter <[email protected]>
Quoted 15 lines

drivers/block/ps3disk.c static void ps3disk_scatter_gather(struct ps3_storage_device *dev, · line 119

Quoted 13 lines
drivers/block/ps3disk.c +1 -1
index e9da874..03688c2 100644
@@ -113,7 +113,7 @@ static void ps3disk_scatter_gather(struct ps3_storage_device *dev,
                       memcpy(buf, dev->bounce_buf+offset, size);
               offset += size;
               flush_kernel_dcache_page(bvec->bv_page);
-             bvec_kunmap_irq(bvec, &flags);
+             bvec_kunmap_irq(buf, &flags);
               i++;
       }
  }

Thanks applied, that bug is all too common.

Because bvec_kunmap_irq() is a macro if !CONFIG_HIGHMEM, and thus there's no argument type checking on e.g. pp64, which doesn't support HIGHMEM?

What about turning it into an inline function?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds

#3 lore raw
Quoted 23 lines · Geert Uytterhoeven
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Dan Carpenter

This should pass "buf" to bvec_kunmap_irq() instead of "bv". The api is like kmap_atomic() instead of kmap().

Signed-off-by Dan Carpenter <[email protected]>
Quoted 15 lines

drivers/block/ps3disk.c static void ps3disk_scatter_gather(struct ps3_storage_device *dev, · line 119

Quoted 13 lines
drivers/block/ps3disk.c +1 -1
index e9da874..03688c2 100644
@@ -113,7 +113,7 @@ static void ps3disk_scatter_gather(struct ps3_storage_device *dev,
                      memcpy(buf, dev->bounce_buf+offset, size);
              offset += size;
              flush_kernel_dcache_page(bvec->bv_page);
-             bvec_kunmap_irq(bvec, &flags);
+             bvec_kunmap_irq(buf, &flags);
              i++;
      }
 }

Thanks applied, that bug is all too common.

Quoted 2 lines

Because bvec_kunmap_irq() is a macro if !CONFIG_HIGHMEM, and thus there's no argument type checking on e.g. pp64, which doesn't support HIGHMEM?

It's a generic problem, not isolated to this case. The problem is that the API isn't symmetric, and the unmap parts take a void * pointer.

Signature · 1 line
Jens Axboe
#4 lore raw
Quoted 24 lines · Jens Axboe
Quoted 23 lines · Geert Uytterhoeven
Quoted 21 lines · Jens Axboe
Quoted 10 lines · Dan Carpenter

This should pass "buf" to bvec_kunmap_irq() instead of "bv".  The api is like kmap_atomic() instead of kmap().

Signed-off-by Dan Carpenter <[email protected]>
drivers/block/ps3disk.c +0 -0
index e9da874..03688c2 100644

drivers/block/ps3disk.c static void ps3disk_scatter_gather(struct ps3_storage_device *dev, · line 119

Quoted 8 lines
drivers/block/ps3disk.c +1 -1
@@ -113,7 +113,7 @@ static void ps3disk_scatter_gather(struct ps3_storage_device *dev,
                       memcpy(buf, dev->bounce_buf+offset, size);
               offset += size;
               flush_kernel_dcache_page(bvec->bv_page);
-             bvec_kunmap_irq(bvec, &flags);
+             bvec_kunmap_irq(buf, &flags);
               i++;
       }
  }

Thanks applied, that bug is all too common.

Quoted 5 lines
Quoted 2 lines

Because  bvec_kunmap_irq() is a macro if !CONFIG_HIGHMEM, and thus there's no argument type checking on e.g. pp64, which doesn't support HIGHMEM?

It's a generic problem, not isolated to this case. The problem is that the API isn't symmetric, and the unmap parts take a void * pointer.

No, the unmap takes a char *:

static inline void bvec_kunmap_irq(char *buffer, unsigned long *flags)

So it could be detected. Patch will follow.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds