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

[David Sterba <dsterba@suse.cz>]

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