linux-xfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org, hch@infradead.org
Subject: Re: [PATCH 2/6] xfs: fix memory corruption during remote attr value buffer invalidation
Date: Tue, 14 Jan 2020 00:40:11 -0800	[thread overview]
Message-ID: <20200114084011.GB10888@infradead.org> (raw)
In-Reply-To: <157898350371.1566005.2641685060877851666.stgit@magnolia>

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

> -			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.

> @@ -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.

  reply	other threads:[~2020-01-14  8:40 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-14  6:31 [PATCH v3 0/6] xfs: fix buf log item memory corruption on non-amd64 Darrick J. Wong
2020-01-14  6:31 ` [PATCH 1/6] xfs: refactor remote attr value buffer invalidation Darrick J. Wong
2020-01-14  8:30   ` Christoph Hellwig
2020-01-14  6:31 ` [PATCH 2/6] xfs: fix memory corruption during " Darrick J. Wong
2020-01-14  8:40   ` Christoph Hellwig [this message]
2020-01-14 23:02     ` Darrick J. Wong
2020-01-14  6:31 ` [PATCH 3/6] xfs: clean up xfs_buf_item_get_format return value Darrick J. Wong
2020-01-14  6:31 ` [PATCH 4/6] xfs: complain if anyone tries to create a too-large buffer log item Darrick J. Wong
2020-01-14  6:32 ` [PATCH 5/6] xfs: make struct xfs_buf_log_format have a consistent size Darrick J. Wong
2020-01-14  8:41   ` Christoph Hellwig
2020-01-14  6:32 ` [PATCH 6/6] xfs: check log iovec size to make sure it's plausibly a buffer log format Darrick J. Wong
2020-01-14  8:42   ` Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200114084011.GB10888@infradead.org \
    --to=hch@infradead.org \
    --cc=darrick.wong@oracle.com \
    --cc=linux-xfs@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).