hfsplus: fix cross-page bio requests

Function hfsplus_submit_bio() did not work when the passed buffer spanned over more than one page. That was because bio_alloc() is passed 1 as a number of vectors but more than one vector were added inside the 'while' loop. It periodically caused a mount error when the volume header could not be read.

This patch modifies the code so that only one vector is used. It works for multiple pages too. Also adds a return code check after bio_alloc().

Cc Anton Altaparmakov <[email protected]>

Cc Al Viro <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Vyacheslav Dubeyko <[email protected]>

Cc Hin-Tak Leung <[email protected]>

Cc Sougata Santra <[email protected]>

Signed-off-by Sergei Antonov <[email protected]>

---
 fs/hfsplus/wrapper.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

fs/hfsplus/wrapper.c +10 -19

index cc62356..e245faa 100644
@@ -62,29 +62,20 @@ int hfsplus_submit_bio(struct super_block *sb, sector_t sector,
 	offset = start & (io_size - 1);
 	sector &= ~((io_size >> HFSPLUS_SECTOR_SHIFT) - 1);
 
-	bio = bio_alloc(GFP_NOIO, 1);
-	bio->bi_iter.bi_sector = sector;
-	bio->bi_bdev = sb->s_bdev;
-
 	if (!(rw & WRITE) && data)
 		*data = (u8 *)buf + offset;
 
-	while (io_size > 0) {
-		unsigned int page_offset = offset_in_page(buf);
-		unsigned int len = min_t(unsigned int, PAGE_SIZE - page_offset,
-					 io_size);
-
-		ret = bio_add_page(bio, virt_to_page(buf), len, page_offset);
-		if (ret != len) {
-			ret = -EIO;
-			goto out;
-		}
-		io_size -= len;
-		buf = (u8 *)buf + len;
-	}
-
+	bio = bio_alloc(GFP_NOIO, 1);
+	if (!bio)
+		return -ENOMEM;
+	bio->bi_iter.bi_sector = sector;
+	bio->bi_bdev = sb->s_bdev;
+	bio->bi_vcnt = 1;
+	bio->bi_iter.bi_size = io_size;
+	bio->bi_io_vec[0].bv_page = virt_to_page(buf);
+	bio->bi_io_vec[0].bv_offset = offset_in_page(buf);
+	bio->bi_io_vec[0].bv_len = io_size;
 	ret = submit_bio_wait(rw, bio);
-out:
 	bio_put(bio);
 	return ret < 0 ? ret : 0;
 }

Quoted 7 lines · Sergei Antonov

Function hfsplus_submit_bio() did not work when the passed buffer spanned over more than one page. That was because bio_alloc() is passed 1 as a number of vectors but more than one vector were added inside the 'while' loop. It periodically caused a mount error when the volume header could not be read.

This patch modifies the code so that only one vector is used. It works for multiple pages too. Also adds a return code check after bio_alloc().

I think that it really makes sense to describe the issue's reproducing way. It will be really precious for understanding of symptoms and reasons of the issue.

Could you add more detailed description?

Then, I will have opportunity to test your patch.

Quoted 15 lines · Viacheslav Dubeyko

Quoted 7 lines · Sergei Antonov

Function hfsplus_submit_bio() did not work when the passed buffer spanned over more than one page. That was because bio_alloc() is passed 1 as a number of vectors but more than one vector were added inside the 'while' loop. It periodically caused a mount error when the volume header could not be read.

This patch modifies the code so that only one vector is used. It works for multiple pages too. Also adds a return code check after bio_alloc().

I think that it really makes sense to describe the issue's reproducing way. It will be really precious for understanding of symptoms and reasons of the issue.

Could you add more detailed description?

Quoted 1 line

Then, I will have opportunity to test your patch.

Well, the description says it all. To put it bluntly, when this line from wrapper.c
sbi->s_vhdr_buf = kmalloc(hfsplus_min_io_size(sb), GFP_KERNEL);
assigns s_vhdr_buf a value satisfying condition (PAGE_SIZE - (value & PAGE_SIZE) < 512) then this call (also from wrapper.c) returns an

error:
error = hfsplus_submit_bio(sb, part_start + HFSPLUS_VOLHEAD_SECTOR,

    sbi->s_vhdr_buf, (void **)&sbi->s_vhdr,
    READ);

To give a specific example, sbi->s_vhdr_buf equal to 0xffff8804085acec0 spans two pages and hfsplus_submit_bio() can not read into such a buffer, returns an error, mount operation fails.

Quoted 18 lines · Sergei Antonov

Quoted 16 lines · Viacheslav Dubeyko

Quoted 7 lines · Sergei Antonov

Function hfsplus_submit_bio() did not work when the passed buffer spanned over more than one page. That was because bio_alloc() is passed 1 as a number of vectors but more than one vector were added inside the 'while' loop. It periodically caused a mount error when the volume header could not be read.

This patch modifies the code so that only one vector is used. It works for multiple pages too. Also adds a return code check after bio_alloc().

I think that it really makes sense to describe the issue's reproducing way. It will be really precious for understanding of symptoms and reasons of the issue.

Could you add more detailed description?

Then, I will have opportunity to test your patch.

Quoted 5 lines

Well, the description says it all. To put it bluntly, when this line from wrapper.c
sbi->s_vhdr_buf = kmalloc(hfsplus_min_io_size(sb), GFP_KERNEL);
assigns s_vhdr_buf a value satisfying condition (PAGE_SIZE - (value & PAGE_SIZE) < 512) then this call (also from wrapper.c) returns an

I'm sorry, the right condition is (PAGE_SIZE - (value & (PAGE_SIZE - 1)) < 512).

Quoted 8 lines

error:
error = hfsplus_submit_bio(sb, part_start + HFSPLUS_VOLHEAD_SECTOR,

    sbi->s_vhdr_buf, (void **)&sbi->s_vhdr,
    READ);

To give a specific example, sbi->s_vhdr_buf equal to 0xffff8804085acec0 spans two pages and hfsplus_submit_bio() can not read into such a buffer, returns an error, mount operation fails.

Quoted 32 lines · Sergei Antonov

Quoted 16 lines · Sergei Antonov

Quoted 15 lines · Viacheslav Dubeyko

Quoted 7 lines · Sergei Antonov

Function hfsplus_submit_bio() did not work when the passed buffer spanned over more than one page. That was because bio_alloc() is passed 1 as a number of vectors but more than one vector were added inside the 'while' loop. It periodically caused a mount error when the volume header could not be read.

This patch modifies the code so that only one vector is used. It works for multiple pages too. Also adds a return code check after bio_alloc().

I think that it really makes sense to describe the issue's reproducing way. It will be really precious for understanding of symptoms and reasons of the issue.

Could you add more detailed description?

Quoted 7 lines

Quoted 1 line

Then, I will have opportunity to test your patch.

Well, the description says it all. To put it bluntly, when this line from wrapper.c
sbi->s_vhdr_buf = kmalloc(hfsplus_min_io_size(sb), GFP_KERNEL);
assigns s_vhdr_buf a value satisfying condition (PAGE_SIZE - (value & PAGE_SIZE) < 512) then this call (also from wrapper.c) returns an

I'm sorry, the right condition is (PAGE_SIZE - (value & (PAGE_SIZE - 1)) < 512).

Quoted 5 lines

error:
error = hfsplus_submit_bio(sb, part_start + HFSPLUS_VOLHEAD_SECTOR,

    sbi->s_vhdr_buf, (void **)&sbi->s_vhdr,
    READ);

Quoted 3 lines

Quoted 3 lines

To give a specific example, sbi->s_vhdr_buf equal to 0xffff8804085acec0 spans two pages and hfsplus_submit_bio() can not read into such a buffer, returns an error, mount operation fails.

How an ordinary user can discover this issue? Could you describe a real use-case for the reproducing?

Quoted 37 lines · Viacheslav Dubeyko

Quoted 35 lines · Sergei Antonov

Quoted 16 lines · Sergei Antonov

Quoted 15 lines · Viacheslav Dubeyko

Quoted 7 lines · Sergei Antonov

Function hfsplus_submit_bio() did not work when the passed buffer spanned over more than one page. That was because bio_alloc() is passed 1 as a number of vectors but more than one vector were added inside the 'while' loop. It periodically caused a mount error when the volume header could not be read.

This patch modifies the code so that only one vector is used. It works for multiple pages too. Also adds a return code check after bio_alloc().

I think that it really makes sense to describe the issue's reproducing way. It will be really precious for understanding of symptoms and reasons of the issue.

Could you add more detailed description?

Quoted 7 lines

Quoted 1 line

Then, I will have opportunity to test your patch.

Well, the description says it all. To put it bluntly, when this line from wrapper.c
sbi->s_vhdr_buf = kmalloc(hfsplus_min_io_size(sb), GFP_KERNEL);
assigns s_vhdr_buf a value satisfying condition (PAGE_SIZE - (value & PAGE_SIZE) < 512) then this call (also from wrapper.c) returns an

I'm sorry, the right condition is (PAGE_SIZE - (value & (PAGE_SIZE - 1)) < 512).

Quoted 8 lines

error:
error = hfsplus_submit_bio(sb, part_start + HFSPLUS_VOLHEAD_SECTOR,

    sbi->s_vhdr_buf, (void **)&sbi->s_vhdr,
    READ);

To give a specific example, sbi->s_vhdr_buf equal to 0xffff8804085acec0 spans two pages and hfsplus_submit_bio() can not read into such a buffer, returns an error, mount operation fails.

Quoted 2 lines

How an ordinary user can discover this issue? Could you describe a real use-case for the reproducing?

I discovered it by running "mount <partition> <mountpoint>" command. It was working probabilistically: sometimes it mounted the partition, sometimes not. If course, the driver was not specially tweaked, it was just a compiled source code from Linus' git tree.

Function hfsplus_submit_bio() did not work when the passed buffer spanned over more than one page. That was because bio_alloc() is passed 1 as a number of vectors but more than one vector were added inside the 'while' loop. It periodically caused a mount error when the volume header could not be read.

This patch modifies the code so that only one vector is used. It works for multiple pages too. Also adds a return code check after bio_alloc().

Cc Anton Altaparmakov <[email protected]>

Cc Al Viro <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Vyacheslav Dubeyko <[email protected]>

Cc Hin-Tak Leung <[email protected]>

Cc Sougata Santra <[email protected]>

Signed-off-by Sergei Antonov <[email protected]>

---
fs/hfsplus/wrapper.c | 29 ++++++++++-------------------

1 file changed, 10 insertions(+), 19 deletions(-)

fs/hfsplus/wrapper.c +1 -18

index cc62356..e245faa 100644
@@ -62,29 +62,20 @@ int hfsplus_submit_bio(struct super_block *sb, sector_t sector,
 	offset = start & (io_size - 1);
 	sector &= ~((io_size >> HFSPLUS_SECTOR_SHIFT) - 1);
 
-	bio = bio_alloc(GFP_NOIO, 1);
-	bio->bi_iter.bi_sector = sector;
-	bio->bi_bdev = sb->s_bdev;
-
 	if (!(rw & WRITE) && data)
 		*data = (u8 *)buf + offset;
 
-	while (io_size > 0) {
-		unsigned int page_offset = offset_in_page(buf);
-		unsigned int len = min_t(unsigned int, PAGE_SIZE - page_offset,
-					 io_size);
-
-		ret = bio_add_page(bio, virt_to_page(buf), len, page_offset);
-		if (ret != len) {
-			ret = -EIO;
-			goto out;
-		}
-		io_size -= len;
-		buf = (u8 *)buf + len;
-	}
-
+	bio = bio_alloc(GFP_NOIO, 1);

fs/hfsplus/wrapper.c +0 -1

-out:
 	bio_put(bio);
 	return ret < 0 ? ret : 0;
 }

Hi Sergei,

Quoted 56 lines · Sergei Antonov

Function hfsplus_submit_bio() did not work when the passed buffer spanned over more than one page. That was because bio_alloc() is passed 1 as a number of vectors but more than one vector were added inside the 'while' loop. It periodically caused a mount error when the volume header could not be read.

This patch modifies the code so that only one vector is used. It works for multiple pages too. Also adds a return code check after bio_alloc().

Cc Anton Altaparmakov <[email protected]>

Cc Al Viro <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Vyacheslav Dubeyko <[email protected]>

Cc Hin-Tak Leung <[email protected]>

Cc Sougata Santra <[email protected]>

Signed-off-by Sergei Antonov <[email protected]>

---
fs/hfsplus/wrapper.c | 29 ++++++++++-------------------

1 file changed, 10 insertions(+), 19 deletions(-)

fs/hfsplus/wrapper.c +5 -18

index cc62356..e245faa 100644
@@ -62,29 +62,20 @@ int hfsplus_submit_bio(struct super_block *sb, sector_t sector,
      offset = start & (io_size - 1);
      sector &= ~((io_size >> HFSPLUS_SECTOR_SHIFT) - 1);
 
-     bio = bio_alloc(GFP_NOIO, 1);
-     bio->bi_iter.bi_sector = sector;
-     bio->bi_bdev = sb->s_bdev;
-
      if (!(rw & WRITE) && data)
              *data = (u8 *)buf + offset;
 
-     while (io_size > 0) {
-             unsigned int page_offset = offset_in_page(buf);
-             unsigned int len = min_t(unsigned int, PAGE_SIZE - page_offset,
-                                      io_size);
-
-             ret = bio_add_page(bio, virt_to_page(buf), len, page_offset);
-             if (ret != len) {
-                     ret = -EIO;
-                     goto out;
-             }
-             io_size -= len;
-             buf = (u8 *)buf + len;
-     }
-
+     bio = bio_alloc(GFP_NOIO, 1);
+     if (!bio)
+             return -ENOMEM;
+     bio->bi_iter.bi_sector = sector;
+     bio->bi_bdev = sb->s_bdev;

fs/hfsplus/wrapper.c int hfsplus_submit_bio(struct super_block *sb, sector_t sector, · line 78

Quoted 6 lines

fs/hfsplus/wrapper.c +5 -0

+     bio->bi_vcnt = 1;
+     bio->bi_iter.bi_size = io_size;
+     bio->bi_io_vec[0].bv_page = virt_to_page(buf);
+     bio->bi_io_vec[0].bv_offset = offset_in_page(buf);
+     bio->bi_io_vec[0].bv_len = io_size;
      ret = submit_bio_wait(rw, bio);

I think you need to rethink this. Think of what you are doing: You are adding a single page to the bio but then submitting io that exceeds the page...

I think you should fix it the other way round, i.e. use the original code but increase the number passed to bio_alloc() to:

        bio = bio_alloc(GFP_NOIO, (offset_in_page(buf) + io_size + PAGE_CACHE_SIZE - 1) / PAGE_CACHE_SIZE);

Or if you know that it can only ever span two pages (you would need to check all call sites of hfsplus_submit_bio() which I have not done then you could potentially just use a constant 2, i.e.

        bio = bio_alloc(GFP_NOIO, 2);

And then it does not matter whether you add one or two pages it will always work. But don't do that without checking all call sites. If it can be called with a larger number of pages then the above more accurate version might be better.

Also note that if the number can actually grow large then you will need to break it up. bio_alloc() has a limit of BIO_MAX_PAGES so you cannot call it for more than that (or you will just get returned NULL without anything else happening).

Finally, checking the bio_alloc() for failure is obviously a good idea so do keep that change.

Best regards,

Anton

fs/hfsplus/wrapper.c int hfsplus_submit_bio(struct super_block *sb, sector_t sector, · line 81

Quoted 4 lines

fs/hfsplus/wrapper.c +0 -1

-out:
      bio_put(bio);
      return ret < 0 ? ret : 0;
 }

--
Anton Altaparmakov <anton at tuxera.com> (replace at with @) Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

Quoted 12 lines

Quoted 11 lines

fs/hfsplus/wrapper.c +10 -0

+     bio = bio_alloc(GFP_NOIO, 1);
+     if (!bio)
+             return -ENOMEM;
+     bio->bi_iter.bi_sector = sector;
+     bio->bi_bdev = sb->s_bdev;
+     bio->bi_vcnt = 1;
+     bio->bi_iter.bi_size = io_size;
+     bio->bi_io_vec[0].bv_page = virt_to_page(buf);
+     bio->bi_io_vec[0].bv_offset = offset_in_page(buf);
+     bio->bi_io_vec[0].bv_len = io_size;
      ret = submit_bio_wait(rw, bio);

Quoted 1 line

I think you need to rethink this. Think of what you are doing: You are adding a single page to the bio but then submitting io that exceeds the page...

I'm not adding a single page, I'm adding a single vector. This "bio->bi_vcnt = 1; bio->bi_io_vec[0].bv_page = ...; etc." approach can be found in a number of places in the kernel. Just followed other people's example. And I tested the code too :).