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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
