Re: [PATCH] xfs: fix off-by-one-block in xfs_discard_folio()

[Dave Chinner <david@fromorbit.com>]

On Tue, Feb 28, 2023 at 05:08:25PM -0800, Darrick J. Wong wrote:
> On Wed, Mar 01, 2023 at 12:04:17PM +1100, Dave Chinner wrote:
> > On Tue, Feb 28, 2023 at 04:47:01PM -0800, Darrick J. Wong wrote:
> > > On Wed, Mar 01, 2023 at 11:17:06AM +1100, Dave Chinner wrote:
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > > 
> > > > The recent writeback corruption fixes changed the code in
> > > > xfs_discard_folio() to calculate a byte range to for punching
> > > > delalloc extents. A mistake was made in using round_up(pos) for the
> > > > end offset, because when pos points at the first byte of a block, it
> > > > does not get rounded up to point to the end byte of the block. hence
> > > > the punch range is short, and this leads to unexpected behaviour in
> > > > certain cases in xfs_bmap_punch_delalloc_range.
> > > > 
> > > > e.g. pos = 0 means we call xfs_bmap_punch_delalloc_range(0,0), so
> > > > there is no previous extent and it rounds up the punch to the end of
> > > > the delalloc extent it found at offset 0, not the end of the range
> > > > given to xfs_bmap_punch_delalloc_range().
> > > > 
> > > > Fix this by handling the zero block offset case correctly.
> > > > 
> > > > Fixes: 7348b322332d ("xfs: xfs_bmap_punch_delalloc_range() should take a byte range")
> > > > Reported-by: Pengfei Xu <pengfei.xu@intel.com>
> > > > Found-by: Brian Foster <bfoster@redhat.com>
> > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > ---
> > > >  fs/xfs/xfs_aops.c | 14 ++++++++++++--
> > > >  1 file changed, 12 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> > > > index 41734202796f..429f63cfd7d4 100644
> > > > --- a/fs/xfs/xfs_aops.c
> > > > +++ b/fs/xfs/xfs_aops.c
> > > > @@ -466,6 +466,7 @@ xfs_discard_folio(
> > > >  {
> > > >  	struct xfs_inode	*ip = XFS_I(folio->mapping->host);
> > > >  	struct xfs_mount	*mp = ip->i_mount;
> > > > +	xfs_off_t		end_off;
> > > >  	int			error;
> > > >  
> > > >  	if (xfs_is_shutdown(mp))
> > > > @@ -475,8 +476,17 @@ xfs_discard_folio(
> > > >  		"page discard on page "PTR_FMT", inode 0x%llx, pos %llu.",
> > > >  			folio, ip->i_ino, pos);
> > > >  
> > > > -	error = xfs_bmap_punch_delalloc_range(ip, pos,
> > > > -			round_up(pos, folio_size(folio)));
> > > > +	/*
> > > > +	 * Need to be careful with the case where the pos passed in points to
> > > > +	 * the first byte of the folio - rounding up won't change the value,
> > > > +	 * but in all cases here we need to end offset to point to the start
> > > > +	 * of the next folio.
> > > > +	 */
> > > > +	if (pos == folio_pos(folio))
> > > > +		end_off = pos + folio_size(folio);
> > > > +	else
> > > > +		end_off = round_up(pos, folio_size(folio));
> > > 
> > > Can this construct be simplified to:
> > > 
> > > 	end_off = round_up(pos + 1, folio_size(folio));
> > 
> > I thought about that first, but I really, really dislike sprinkling
> > magic "+ 1" corrections into the code to address non-obvious
> > unexplained off-by-one problems.
> > 
> > 
> > > If pos is the first byte of the folio, it'll round end_off to the start
> > > of the next folio.  If pos is (somehow) the last byte of the folio, the
> > > first argument to round_up is already the first byte of the next folio,
> > > and rounding won't change it.
> > 
> > Yup, and that's exactly the problem I had with doing this - it
> > relies on the implicit behaviour that by moving last byte of a block
> > to the first byte of the next block, round_up() won't change the end
> > offset.  i.e. the correct functioning of the code is just as
> > non-obvious with a magic "+ 1" as the incorrect functioning was
> > without it.
> > 
> > Look at it this way: I didn't realise it was wrong when I wrote the
> > code, and I couldn't find the bug round_up() introduced when reading
> > the code even after the problem had been bisected to this exact
> > change. The code I wrote is bad, and adding a magic "+ 1" to fix the
> > bug doesn't make the code any better.
> > 
> > Given this is a slow path, so I see no point in optimising the code
> > for efficiency. IMO, clarity of the logic and calculation being made
> > is far more important - obviously correct logic is better than
> > relying on the effect of a magic "+ 1" on some other function to
> > acheive the same thing....
> 
> <nod> Just making sure I wasn't missing something.
> 
> By the way, was this reported to the list?

The original report is here:

https://bugzilla.kernel.org/show_bug.cgi?id=217030 

And in discussion of the initial fix I sent out, Brian found the
off-by-one here:

https://lore.kernel.org/linux-xfs/Y+vOfaxIWX1c%2Fyy9@bfoster/

I still plan to fix up the patchset that removes ->discard_folio and
retain pages dirty in memory after writeback mapping failures, but I
figured getting the bug fix out was more important...

-Dave.

-- 
Dave Chinner
david@fromorbit.com