| Message ID | 1491595870-9633-1-git-send-email-bo.li.liu@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > } > } > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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/) -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; > > } > > } > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; } }
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(-)