diff mbox series

[2/3] xfs: complain if anyone tries to create a too-large buffer log item

Message ID 157845709897.84011.1433283906403215456.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: fix buf log item memory corruption on non-amd64 | expand

Commit Message

Darrick J. Wong Jan. 8, 2020, 4:18 a.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Complain if someone calls xfs_buf_item_init on a buffer that is larger
than the dirty bitmap can handle, or tries to log a region that's past
the end of the dirty bitmap.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_buf_item.c |   23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

Comments

Christoph Hellwig Jan. 8, 2020, 8:51 a.m. UTC | #1 
On Tue, Jan 07, 2020 at 08:18:19PM -0800, Darrick J. Wong wrote:
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 3984779e5911..bfbe8a5b8959 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -761,18 +761,25 @@ xfs_buf_item_init(
>  	 * buffer. This makes the implementation as simple as possible.
>  	 */
>  	error = xfs_buf_item_get_format(bip, bp->b_map_count);
> -	ASSERT(error == 0);
> -	if (error) {	/* to stop gcc throwing set-but-unused warnings */
> -		kmem_cache_free(xfs_buf_item_zone, bip);
> -		return error;
> +	if (error) {
> +		xfs_err(mp, "could not allocate buffer item, err=%d", error);
> +		goto err;
>  	}

The error handling here is weird, as xfs_buf_item_get_format can't fail
to start with.  I'd rather see a prep patch removing the bogus check
for the kmem_zalloc and change the return value from
xfs_buf_item_get_format to void.

Otherwise the patch looks good.
Darrick J. Wong Jan. 8, 2020, 5:22 p.m. UTC | #2 
On Wed, Jan 08, 2020 at 12:51:31AM -0800, Christoph Hellwig wrote:
> On Tue, Jan 07, 2020 at 08:18:19PM -0800, Darrick J. Wong wrote:
> > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> > index 3984779e5911..bfbe8a5b8959 100644
> > --- a/fs/xfs/xfs_buf_item.c
> > +++ b/fs/xfs/xfs_buf_item.c
> > @@ -761,18 +761,25 @@ xfs_buf_item_init(
> >  	 * buffer. This makes the implementation as simple as possible.
> >  	 */
> >  	error = xfs_buf_item_get_format(bip, bp->b_map_count);
> > -	ASSERT(error == 0);
> > -	if (error) {	/* to stop gcc throwing set-but-unused warnings */
> > -		kmem_cache_free(xfs_buf_item_zone, bip);
> > -		return error;
> > +	if (error) {
> > +		xfs_err(mp, "could not allocate buffer item, err=%d", error);
> > +		goto err;
> >  	}
> 
> The error handling here is weird, as xfs_buf_item_get_format can't fail
> to start with.  I'd rather see a prep patch removing the bogus check
> for the kmem_zalloc and change the return value from
> xfs_buf_item_get_format to void.
> 
> Otherwise the patch looks good.

Ok.

--D
diff mbox series

Patch

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 3984779e5911..bfbe8a5b8959 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -761,18 +761,25 @@  xfs_buf_item_init(
 	 * buffer. This makes the implementation as simple as possible.
 	 */
 	error = xfs_buf_item_get_format(bip, bp->b_map_count);
-	ASSERT(error == 0);
-	if (error) {	/* to stop gcc throwing set-but-unused warnings */
-		kmem_cache_free(xfs_buf_item_zone, bip);
-		return error;
+	if (error) {
+		xfs_err(mp, "could not allocate buffer item, err=%d", error);
+		goto err;
 	}
 
-
 	for (i = 0; i < bip->bli_format_count; i++) {
 		chunks = DIV_ROUND_UP(BBTOB(bp->b_maps[i].bm_len),
 				      XFS_BLF_CHUNK);
 		map_size = DIV_ROUND_UP(chunks, NBWORD);
 
+		if (map_size > XFS_BLF_DATAMAP_SIZE) {
+			xfs_err(mp,
+	"buffer item dirty bitmap (%u uints) too small to reflect %u bytes!",
+					map_size,
+					BBTOB(bp->b_maps[i].bm_len));
+			error = -EFSCORRUPTED;
+			goto err;
+		}
+
 		bip->bli_formats[i].blf_type = XFS_LI_BUF;
 		bip->bli_formats[i].blf_blkno = bp->b_maps[i].bm_bn;
 		bip->bli_formats[i].blf_len = bp->b_maps[i].bm_len;
@@ -782,6 +789,9 @@  xfs_buf_item_init(
 	bp->b_log_item = bip;
 	xfs_buf_hold(bp);
 	return 0;
+err:
+	kmem_cache_free(xfs_buf_item_zone, bip);
+	return error;
 }
 
 
@@ -805,6 +815,9 @@  xfs_buf_item_log_segment(
 	uint		end_bit;
 	uint		mask;
 
+	ASSERT(first < XFS_BLF_DATAMAP_SIZE * XFS_BLF_CHUNK * NBWORD);
+	ASSERT(last < XFS_BLF_DATAMAP_SIZE * XFS_BLF_CHUNK * NBWORD);
+
 	/*
 	 * Convert byte offsets to bit numbers.
 	 */