[PATCH] Btrfs: fix segment fault when doing dio read

[PATCH] Btrfs: fix segment fault when doing dio read

[Liu Bo]

Commit 2dabb3248453 ("Btrfs: Direct I/O read: Work on sectorsized blocks")
introduced this bug during iterating bio pages in dio read's endio hook,
and it could end up with segment fault of the dio reading task.

So the reason is 'if (nr_sectors--)', and it makes the code assume that
there is one more block in the same page, so page offset is increased and
the bio which is created to repair the bad block then has an incorrect
bvec.bv_offset, and a later access of the page content would throw a
segment fault.

This also adds ASSERT to check page offset against page size.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/inode.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index c875e68..5e71f1e 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7972,8 +7972,10 @@ static int __btrfs_correct_data_nocsum(struct inode *inode,
 
 		start += sectorsize;
 
-		if (nr_sectors--) {
+		nr_sectors--;
+		if (nr_sectors) {
 			pgoff += sectorsize;
+			ASSERT(pgoff < PAGE_SIZE);
 			goto next_block_or_try_again;
 		}
 	}
@@ -8074,8 +8076,10 @@ static int __btrfs_subio_endio_read(struct inode *inode,
 
 		ASSERT(nr_sectors);
 
-		if (--nr_sectors) {
+		nr_sectors--;
+		if (nr_sectors) {
 			pgoff += sectorsize;
+			ASSERT(pgoff < PAGE_SIZE);
 			goto next_block;
 		}
 	}
-- 
2.5.5

Re: [PATCH] Btrfs: fix segment fault when doing dio read

[Nikolay Borisov]

On  7.04.2017 23:11, Liu Bo wrote:
> Commit 2dabb3248453 ("Btrfs: Direct I/O read: Work on sectorsized blocks")
> introduced this bug during iterating bio pages in dio read's endio hook,
> and it could end up with segment fault of the dio reading task.
> 
> So the reason is 'if (nr_sectors--)', and it makes the code assume that
> there is one more block in the same page, so page offset is increased and
> the bio which is created to repair the bad block then has an incorrect
> bvec.bv_offset, and a later access of the page content would throw a
> segment fault.
> 
> This also adds ASSERT to check page offset against page size.
> 
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  fs/btrfs/inode.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index c875e68..5e71f1e 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7972,8 +7972,10 @@ static int __btrfs_correct_data_nocsum(struct inode *inode,
>  
>  		start += sectorsize;
>  
> -		if (nr_sectors--) {
> +		nr_sectors--;
Why not if(--nr_sectors)? I know it's more of a style issue but it will
reduce the size of the diff ?
> +		if (nr_sectors) {
>  			pgoff += sectorsize;
> +			ASSERT(pgoff < PAGE_SIZE);
>  			goto next_block_or_try_again;
>  		}
>  	}
> @@ -8074,8 +8076,10 @@ static int __btrfs_subio_endio_read(struct inode *inode,
>  
>  		ASSERT(nr_sectors);
>  
> -		if (--nr_sectors) {
> +		nr_sectors--;
> +		if (nr_sectors) {
>  			pgoff += sectorsize;
> +			ASSERT(pgoff < PAGE_SIZE);
>  			goto next_block;
>  		}
>  	}
> 

Re: [PATCH] Btrfs: fix segment fault when doing dio read

[David Sterba]

On Sun, Apr 09, 2017 at 08:08:05PM +0300, Nikolay Borisov wrote:
> 
> 
> On  7.04.2017 23:11, Liu Bo wrote:
> > Commit 2dabb3248453 ("Btrfs: Direct I/O read: Work on sectorsized blocks")
> > introduced this bug during iterating bio pages in dio read's endio hook,
> > and it could end up with segment fault of the dio reading task.
> > 
> > So the reason is 'if (nr_sectors--)', and it makes the code assume that
> > there is one more block in the same page, so page offset is increased and
> > the bio which is created to repair the bad block then has an incorrect
> > bvec.bv_offset, and a later access of the page content would throw a
> > segment fault.
> > 
> > This also adds ASSERT to check page offset against page size.
> > 
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >  fs/btrfs/inode.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index c875e68..5e71f1e 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -7972,8 +7972,10 @@ static int __btrfs_correct_data_nocsum(struct inode *inode,
> >  
> >  		start += sectorsize;
> >  
> > -		if (nr_sectors--) {
> > +		nr_sectors--;
> Why not if(--nr_sectors)? I know it's more of a style issue but it will
> reduce the size of the diff ?

I don't think diff size is a problem here. Actually, this instance is
the only one I can find [1] that uses the decrement inside if, so the
change makes it consistent

<smpl>
@@
expression E;
@@

(
* if (E--)
  { ... }
|
* if (E++)
  { ... }
)
</smp>

(run as: spatch -sp_file smpl.cocci -dir fs/btrfs/)

Re: [PATCH] Btrfs: fix segment fault when doing dio read

[Liu Bo]

On Sun, Apr 09, 2017 at 08:08:05PM +0300, Nikolay Borisov wrote:
> 
> 
> On  7.04.2017 23:11, Liu Bo wrote:
> > Commit 2dabb3248453 ("Btrfs: Direct I/O read: Work on sectorsized blocks")
> > introduced this bug during iterating bio pages in dio read's endio hook,
> > and it could end up with segment fault of the dio reading task.
> > 
> > So the reason is 'if (nr_sectors--)', and it makes the code assume that
> > there is one more block in the same page, so page offset is increased and
> > the bio which is created to repair the bad block then has an incorrect
> > bvec.bv_offset, and a later access of the page content would throw a
> > segment fault.
> > 
> > This also adds ASSERT to check page offset against page size.
> > 
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >  fs/btrfs/inode.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index c875e68..5e71f1e 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -7972,8 +7972,10 @@ static int __btrfs_correct_data_nocsum(struct inode *inode,
> >  
> >  		start += sectorsize;
> >  
> > -		if (nr_sectors--) {
> > +		nr_sectors--;
> Why not if(--nr_sectors)? I know it's more of a style issue but it will
> reduce the size of the diff ?

It's error-prone, as shown by how the bug in the original commit was introduced.

Thanks,

-liubo

> > +		if (nr_sectors) {
> >  			pgoff += sectorsize;
> > +			ASSERT(pgoff < PAGE_SIZE);
> >  			goto next_block_or_try_again;
> >  		}
> >  	}
> > @@ -8074,8 +8076,10 @@ static int __btrfs_subio_endio_read(struct inode *inode,
> >  
> >  		ASSERT(nr_sectors);
> >  
> > -		if (--nr_sectors) {
> > +		nr_sectors--;
> > +		if (nr_sectors) {
> >  			pgoff += sectorsize;
> > +			ASSERT(pgoff < PAGE_SIZE);
> >  			goto next_block;
> >  		}
> >  	}
>