[PATCH] hfsplus: fix cross-page bio requests

[PATCH] hfsplus: fix cross-page bio requests

[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 <aia21@cam.ac.uk>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Vyacheslav Dubeyko <slava@dubeyko.com>
Cc: Hin-Tak Leung <htl10@users.sourceforge.net>
Cc: Sougata Santra <sougata@tuxera.com>
Signed-off-by: Sergei Antonov <saproj@gmail.com>
---
 fs/hfsplus/wrapper.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/fs/hfsplus/wrapper.c b/fs/hfsplus/wrapper.c
index cc62356..e245faa 100644
--- a/fs/hfsplus/wrapper.c
+++ b/fs/hfsplus/wrapper.c
@@ -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;
 }
-- 
2.3.0

Re: [PATCH] hfsplus: fix cross-page bio requests

[Viacheslav Dubeyko]

On Fri, 2015-04-10 at 11:02 +0200, Sergei Antonov wrote:
> 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.

Thanks,
Vyacheslav Dubeyko.

> 
> Cc: Anton Altaparmakov <aia21@cam.ac.uk>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Vyacheslav Dubeyko <slava@dubeyko.com>
> Cc: Hin-Tak Leung <htl10@users.sourceforge.net>
> Cc: Sougata Santra <sougata@tuxera.com>
> Signed-off-by: Sergei Antonov <saproj@gmail.com>
> ---
>  fs/hfsplus/wrapper.c | 29 ++++++++++-------------------
>  1 file changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/hfsplus/wrapper.c b/fs/hfsplus/wrapper.c
> index cc62356..e245faa 100644
> --- a/fs/hfsplus/wrapper.c
> +++ b/fs/hfsplus/wrapper.c
> @@ -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;
>  }

Re: [PATCH] hfsplus: fix cross-page bio requests

[Sergei Antonov]

On 10 April 2015 at 18:48, Viacheslav Dubeyko <slava@dubeyko.com> wrote:
> On Fri, 2015-04-10 at 11:02 +0200, Sergei Antonov wrote:
>> 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.

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.

Re: [PATCH] hfsplus: fix cross-page bio requests

[Sergei Antonov]

On 7 June 2015 at 22:05, Sergei Antonov <saproj@gmail.com> wrote:
> On 10 April 2015 at 18:48, Viacheslav Dubeyko <slava@dubeyko.com> wrote:
>> On Fri, 2015-04-10 at 11:02 +0200, Sergei Antonov wrote:
>>> 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.
>
> 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).

> 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.

Re: [PATCH] hfsplus: fix cross-page bio requests

[Viacheslav Dubeyko]

On Sun, 2015-06-07 at 22:09 +0200, Sergei Antonov wrote:
> On 7 June 2015 at 22:05, Sergei Antonov <saproj@gmail.com> wrote:
> > On 10 April 2015 at 18:48, Viacheslav Dubeyko <slava@dubeyko.com> wrote:
> >> On Fri, 2015-04-10 at 11:02 +0200, Sergei Antonov wrote:
> >>> 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.
> >
> > 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).
> 
> > 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.

How an ordinary user can discover this issue? Could you describe a real
use-case for the reproducing? Maybe you can share some guess how it can
occur?

If such situation doesn't take place in the real life then it doesn't
make sense to fix it. Please, prove that your fix is valid.

Thanks,
Vyacheslav Dubeyko.

Re: [PATCH] hfsplus: fix cross-page bio requests

[Sergei Antonov]

On 8 June 2015 at 19:03, Viacheslav Dubeyko <slava@dubeyko.com> wrote:
> On Sun, 2015-06-07 at 22:09 +0200, Sergei Antonov wrote:
>> On 7 June 2015 at 22:05, Sergei Antonov <saproj@gmail.com> wrote:
>> > On 10 April 2015 at 18:48, Viacheslav Dubeyko <slava@dubeyko.com> wrote:
>> >> On Fri, 2015-04-10 at 11:02 +0200, Sergei Antonov wrote:
>> >>> 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.
>> >
>> > 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).
>>
>> > 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.
>
> 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.

Re: [PATCH] hfsplus: fix cross-page bio requests

[Viacheslav Dubeyko]

On Mon, 2015-06-08 at 19:20 +0200, Sergei Antonov wrote:
> On 8 June 2015 at 19:03, Viacheslav Dubeyko <slava@dubeyko.com> wrote:
> > On Sun, 2015-06-07 at 22:09 +0200, Sergei Antonov wrote:
> >> On 7 June 2015 at 22:05, Sergei Antonov <saproj@gmail.com> wrote:
> >> > On 10 April 2015 at 18:48, Viacheslav Dubeyko <slava@dubeyko.com> wrote:
> >> >> On Fri, 2015-04-10 at 11:02 +0200, Sergei Antonov wrote:
> >> >>> 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.
> >> >
> >> > 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).
> >>
> >> > 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.
> >
> > 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.

Could you describe your environment with more details? What partition do
you have? How did it created? What option did you use for partition
creation? What mount option did you use? What Linux version did you use?

Thanks,
Vyacheslav Dubeyko.

Re: [PATCH] hfsplus: fix cross-page bio requests

[Anton Altaparmakov]

Hi Sergei,

> On 10 Apr 2015, at 12:02, Sergei Antonov <saproj@gmail.com> wrote:
> 
> 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 <aia21@cam.ac.uk>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Vyacheslav Dubeyko <slava@dubeyko.com>
> Cc: Hin-Tak Leung <htl10@users.sourceforge.net>
> Cc: Sougata Santra <sougata@tuxera.com>
> Signed-off-by: Sergei Antonov <saproj@gmail.com>
> ---
> fs/hfsplus/wrapper.c | 29 ++++++++++-------------------
> 1 file changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/hfsplus/wrapper.c b/fs/hfsplus/wrapper.c
> index cc62356..e245faa 100644
> --- a/fs/hfsplus/wrapper.c
> +++ b/fs/hfsplus/wrapper.c
> @@ -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);

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

> -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

Re: [PATCH] hfsplus: fix cross-page bio requests

[Sergei Antonov]

On 10 June 2015 at 00:32, Anton Altaparmakov <anton@tuxera.com> wrote:
> Hi Sergei,
>
>> On 10 Apr 2015, at 12:02, Sergei Antonov <saproj@gmail.com> wrote:
>>
>> 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 <aia21@cam.ac.uk>
>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>> Cc: Christoph Hellwig <hch@infradead.org>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Vyacheslav Dubeyko <slava@dubeyko.com>
>> Cc: Hin-Tak Leung <htl10@users.sourceforge.net>
>> Cc: Sougata Santra <sougata@tuxera.com>
>> Signed-off-by: Sergei Antonov <saproj@gmail.com>
>> ---
>> fs/hfsplus/wrapper.c | 29 ++++++++++-------------------
>> 1 file changed, 10 insertions(+), 19 deletions(-)
>>
>> diff --git a/fs/hfsplus/wrapper.c b/fs/hfsplus/wrapper.c
>> index cc62356..e245faa 100644
>> --- a/fs/hfsplus/wrapper.c
>> +++ b/fs/hfsplus/wrapper.c
>> @@ -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);
>
> 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 :).

>  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
>
>> -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
>

Re: [PATCH] hfsplus: fix cross-page bio requests

[hch]

On Wed, Jun 10, 2015 at 01:53:47AM +0200, Sergei Antonov wrote:
> >> +     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);
> >
> > 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 :).

And we're getting right of it because it's problematic.  Please allocate
an optimistic numbero of vectors in bio_alloc and use
bio_add_page as only that takes care of the block driver limits
properly.