Re: [PATCH] xfs: fix bmv_count confusion w/ shared extents

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Jan 26, 2017 at 11:33:03AM -0600, Eric Sandeen wrote:
> On 1/25/17 9:11 PM, Darrick J. Wong wrote:
> > In a bmapx call, bmv_count is the total size of the array, including the
> > zeroth element that userspace uses to supply the search key.  The output
> > array starts at offset 1 so that we can set up the user for the next
> > invocation.  Since we now can split an extent into multiple bmap records
> > due to shared/unshared status, we have to be careful that we don't
> > overflow the output array.
> > 
> > In the original patch f86f403794b ("xfs: teach get_bmapx about shared
> > extents and the CoW fork") I used cur_ext (the output index) to check
> > for overflows, albeit with an off-by-one error.  Since nexleft describes
> > the number of unfilled slots in the output, we can rip all that out and
> > use nexleft for the check directly.
> > 
> > Failure to do this causes heap corruption in bmapx callers such as
> > xfs_io and xfs_scrub.  xfs/328 can reproduce this problem.
> > 
> > Suggested-by: Eric Sandeen <sandeen@sandeen.net>
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Yup, I think this is better, thanks.  Comments around the
> whole inject_map business would be nice, but *shrug* doesn't
> have to be in this patch.
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> > ---
> > v2: simplify the loop accounting to use nexleft for the output checks
> > ---
> >  fs/xfs/xfs_bmap_util.c |   10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index b9abce5..fc6bdaf 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -697,8 +697,7 @@ xfs_getbmap(
> >  			goto out_free_map;
> >  		ASSERT(nmap <= subnex);
> >  
> > -		for (i = 0; i < nmap && nexleft && bmv->bmv_length &&
> > -				cur_ext < bmv->bmv_count; i++) {
> > +		for (i = 0; i < nmap && nexleft && bmv->bmv_length; i++) {

NAK.  I forgot that nexleft is min(bmv_count-1, di_nextents), which
means that that if we have one partially shared bmbt extent and
bmv_count = 1000, we only return the first part of that bmbt extent to
the user.  Worse yet, we also return with bmv_entries < bmv_count-1,
which leads xfs_io to stop calling bmapx prematurely.

That leads to xfs/280 regressing, so I'm going to resubmit the v1 of
this patch, but with improved commenting so that nobody else will miss
this again.

--D

> >  			out[cur_ext].bmv_oflags = 0;
> >  			if (map[i].br_state == XFS_EXT_UNWRITTEN)
> >  				out[cur_ext].bmv_oflags |= BMV_OF_PREALLOC;
> > @@ -760,16 +759,15 @@ xfs_getbmap(
> >  				continue;
> >  			}
> >  
> > +			nexleft--;
> >  			if (inject_map.br_startblock != NULLFSBLOCK) {
> >  				map[i] = inject_map;
> >  				i--;
> > -			} else
> > -				nexleft--;
> > +			}
> >  			bmv->bmv_entries++;
> >  			cur_ext++;
> >  		}
> > -	} while (nmap && nexleft && bmv->bmv_length &&
> > -		 cur_ext < bmv->bmv_count);
> > +	} while (nmap && nexleft && bmv->bmv_length);
> >  
> >   out_free_map:
> >  	kmem_free(map);
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >