Re: [PATCH 2/6] xfs: fix memory corruption during remote attr value buffer invalidation

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

On Tue, Jan 14, 2020 at 12:40:11AM -0800, Christoph Hellwig wrote:
> > Fortunately for us, remote attribute values are written to disk with
> > xfs_bwrite(), which is to say that they are not logged.  Fix the problem
> > by removing all places where we could end up creating a buffer log item
> > for a remote attribute value and leave a note explaining why.
> 
> This is stil missing a comment that you are using a suitable helper
> for marking the buffer stale, and why rmeoving the HOLEBLOCK check
> is safe (which I now tink it is based on looking at the caller).

Oops, I forgot to update the changelog.


> > -			error = xfs_trans_read_buf(mp, args->trans,
> > +			error = xfs_trans_read_buf(mp, NULL,
> >  						   mp->m_ddev_targp,
> >  						   dblkno, dblkcnt, 0, &bp,
> >  						   &xfs_attr3_rmt_buf_ops);
> > @@ -411,7 +428,7 @@ xfs_attr_rmtval_get(
> >  			error = xfs_attr_rmtval_copyout(mp, bp, args->dp->i_ino,
> >  							&offset, &valuelen,
> >  							&dst);
> > -			xfs_trans_brelse(args->trans, bp);
> > +			xfs_buf_relse(bp);
> 
> FYI, I don't think mixing xfs_trans_read_buf and xfs_buf_relse is a good
> pattern.

Yeah, you're right.  I didn't want to go opencoding the !bp or
bp->b_error > 0 cases that happen in xfs_trans_buf_read to make this bug
fix an even bigger pile of patches, but maybe it's just time to clean up
xfs_buf_read() to return error values like most everywhere else.

> > @@ -48,8 +45,8 @@ xfs_attr3_leaf_freextent(
> >  	 * Roll through the "value", invalidating the attribute value's
> >  	 * blocks.
> >  	 */
> > -	tblkno = blkno;
> > -	tblkcnt = blkcnt;
> > +	tblkno = lp->valueblk;
> > +	tblkcnt = lp->valuelen;
> 
> Nit: these could be easily initialized on the declaration lines.  Or
> even better if you keep the old calling conventions of passing the
> blockno and count by value, in which case we don't need the extra local
> variables at all.
> 
> > @@ -174,9 +155,7 @@ xfs_attr3_leaf_inactive(
> >  	 */
> >  	error = 0;
> >  	for (lp = list, i = 0; i < count; i++, lp++) {
> > -		tmp = xfs_attr3_leaf_freextent(trans, dp,
> > -				lp->valueblk, lp->valuelen);
> > -
> > +		tmp = xfs_attr3_rmt_inactive(dp, lp);
> 
> So given that we don't touch the transaction I don't think we even
> need the memory allocation to defer the marking stale of the buffer
> until after the xfs_trans_brelse.  But that could be a separate
> patch, especially if the block/count calling conventions are kept as-is.

These last two I'll clean up in a followup patch that gets rid of the
pointless local variables in the first function and the pointless memory
allocation in the second function.

--D