[PATCH] xfs: remote attribute overwrite causes transaction overrun

[PATCH] xfs: remote attribute overwrite causes transaction overrun

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Commit e461fcb ("xfs: remote attribute lookups require the value
length") passes the remote attribute length in the xfs_da_args
structure on lookup so that CRC calculations and validity checking
can be performed correctly by related code. This, unfortunately has
the side effect of changing the args->valuelen parameter in cases
where it shouldn't.

That is, when we replace a remote attribute, the incoming
replacement stores the value and length in args->value and
args->valuelen, but then the lookup which finds the existing remote
attribute overwrites args->valuelen with the length of the remote
attribute being replaced. Hence when we go to create the new
attribute, we create it of the size of the existing remote
attribute, not the size it is supposed to be. When the new attribute
is much smaller than the old attribute, this results in a
transaction overrun and an ASSERT() failure on a debug kernel:

XFS: Assertion failed: tp->t_blk_res_used <= tp->t_blk_res, file: fs/xfs/xfs_trans.c, line: 331

Fix this by keeping the remote attribute value length separate to
the attribute value length in the xfs_da_args structure. The enables
us to pass the length of the remote attribute to be removed without
overwriting the new attribute's length.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_attr.c        |  7 ++++++-
 fs/xfs/xfs_attr_leaf.c   | 21 +++++++++++----------
 fs/xfs/xfs_attr_remote.c | 11 ++++++++---
 fs/xfs/xfs_da_btree.h    |  2 ++
 4 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index 01b6a01..dbaa674 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -213,7 +213,7 @@ xfs_attr_calc_size(
 		 * Out of line attribute, cannot double split, but
 		 * make room for the attribute value itself.
 		 */
-		uint	dblocks = XFS_B_TO_FSB(mp, valuelen);
+		uint	dblocks = xfs_attr3_rmt_blocks(mp, valuelen);
 		nblks += dblocks;
 		nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK);
 	}
@@ -703,6 +703,7 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
 		args->index2 = args->index;
 		args->rmtblkno2 = args->rmtblkno;
 		args->rmtblkcnt2 = args->rmtblkcnt;
+		args->rmtvaluelen2 = args->rmtvaluelen;
 	}
 
 	/*
@@ -794,6 +795,7 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
 		args->blkno = args->blkno2;
 		args->rmtblkno = args->rmtblkno2;
 		args->rmtblkcnt = args->rmtblkcnt2;
+		args->rmtvaluelen = args->rmtvaluelen2;
 		if (args->rmtblkno) {
 			error = xfs_attr_rmtval_remove(args);
 			if (error)
@@ -1004,8 +1006,10 @@ restart:
 		args->index2 = args->index;
 		args->rmtblkno2 = args->rmtblkno;
 		args->rmtblkcnt2 = args->rmtblkcnt;
+		args->rmtvaluelen2 = args->rmtvaluelen;
 		args->rmtblkno = 0;
 		args->rmtblkcnt = 0;
+		args->rmtvaluelen = 0;
 	}
 
 	retval = xfs_attr3_leaf_add(blk->bp, state->args);
@@ -1133,6 +1137,7 @@ restart:
 		args->blkno = args->blkno2;
 		args->rmtblkno = args->rmtblkno2;
 		args->rmtblkcnt = args->rmtblkcnt2;
+		args->rmtvaluelen = args->rmtvaluelen2;
 		if (args->rmtblkno) {
 			error = xfs_attr_rmtval_remove(args);
 			if (error)
diff --git a/fs/xfs/xfs_attr_leaf.c b/fs/xfs/xfs_attr_leaf.c
index fe9587f..511c283 100644
--- a/fs/xfs/xfs_attr_leaf.c
+++ b/fs/xfs/xfs_attr_leaf.c
@@ -1229,6 +1229,7 @@ xfs_attr3_leaf_add_work(
 		name_rmt->valueblk = 0;
 		args->rmtblkno = 1;
 		args->rmtblkcnt = xfs_attr3_rmt_blocks(mp, args->valuelen);
+		args->rmtvaluelen = args->valuelen;
 	}
 	xfs_trans_log_buf(args->trans, bp,
 	     XFS_DA_LOGRANGE(leaf, xfs_attr3_leaf_name(leaf, args->index),
@@ -2167,11 +2168,11 @@ xfs_attr3_leaf_lookup_int(
 			if (!xfs_attr_namesp_match(args->flags, entry->flags))
 				continue;
 			args->index = probe;
-			args->valuelen = be32_to_cpu(name_rmt->valuelen);
+			args->rmtvaluelen = be32_to_cpu(name_rmt->valuelen);
 			args->rmtblkno = be32_to_cpu(name_rmt->valueblk);
 			args->rmtblkcnt = xfs_attr3_rmt_blocks(
 							args->dp->i_mount,
-							args->valuelen);
+							args->rmtvaluelen);
 			return XFS_ERROR(EEXIST);
 		}
 	}
@@ -2220,19 +2221,19 @@ xfs_attr3_leaf_getvalue(
 		name_rmt = xfs_attr3_leaf_name_remote(leaf, args->index);
 		ASSERT(name_rmt->namelen == args->namelen);
 		ASSERT(memcmp(args->name, name_rmt->name, args->namelen) == 0);
-		valuelen = be32_to_cpu(name_rmt->valuelen);
+		args->rmtvaluelen = be32_to_cpu(name_rmt->valuelen);
 		args->rmtblkno = be32_to_cpu(name_rmt->valueblk);
 		args->rmtblkcnt = xfs_attr3_rmt_blocks(args->dp->i_mount,
-						       valuelen);
+						       args->rmtvaluelen);
 		if (args->flags & ATTR_KERNOVAL) {
-			args->valuelen = valuelen;
+			args->valuelen = args->rmtvaluelen;
 			return 0;
 		}
-		if (args->valuelen < valuelen) {
-			args->valuelen = valuelen;
+		if (args->valuelen < args->rmtvaluelen) {
+			args->valuelen = args->rmtvaluelen;
 			return XFS_ERROR(ERANGE);
 		}
-		args->valuelen = valuelen;
+		args->valuelen = args->rmtvaluelen;
 	}
 	return 0;
 }
@@ -2519,7 +2520,7 @@ xfs_attr3_leaf_clearflag(
 		ASSERT((entry->flags & XFS_ATTR_LOCAL) == 0);
 		name_rmt = xfs_attr3_leaf_name_remote(leaf, args->index);
 		name_rmt->valueblk = cpu_to_be32(args->rmtblkno);
-		name_rmt->valuelen = cpu_to_be32(args->valuelen);
+		name_rmt->valuelen = cpu_to_be32(args->rmtvaluelen);
 		xfs_trans_log_buf(args->trans, bp,
 			 XFS_DA_LOGRANGE(leaf, name_rmt, sizeof(*name_rmt)));
 	}
@@ -2677,7 +2678,7 @@ xfs_attr3_leaf_flipflags(
 		ASSERT((entry1->flags & XFS_ATTR_LOCAL) == 0);
 		name_rmt = xfs_attr3_leaf_name_remote(leaf1, args->index);
 		name_rmt->valueblk = cpu_to_be32(args->rmtblkno);
-		name_rmt->valuelen = cpu_to_be32(args->valuelen);
+		name_rmt->valuelen = cpu_to_be32(args->rmtvaluelen);
 		xfs_trans_log_buf(args->trans, bp1,
 			 XFS_DA_LOGRANGE(leaf1, name_rmt, sizeof(*name_rmt)));
 	}
diff --git a/fs/xfs/xfs_attr_remote.c b/fs/xfs/xfs_attr_remote.c
index 6e37823..2324c66 100644
--- a/fs/xfs/xfs_attr_remote.c
+++ b/fs/xfs/xfs_attr_remote.c
@@ -337,7 +337,7 @@ xfs_attr_rmtval_get(
 	struct xfs_buf		*bp;
 	xfs_dablk_t		lblkno = args->rmtblkno;
 	__uint8_t		*dst = args->value;
-	int			valuelen = args->valuelen;
+	int			valuelen;
 	int			nmap;
 	int			error;
 	int			blkcnt = args->rmtblkcnt;
@@ -348,6 +348,11 @@ xfs_attr_rmtval_get(
 
 	ASSERT(!(args->flags & ATTR_KERNOVAL));
 
+	/* remote value might be different size to the buffer supplied. */
+	if (args->rmtvaluelen = args->valuelen)
+		args->valuelen = args->rmtvaluelen;
+	valuelen = args->valuelen;
+
 	while (valuelen > 0) {
 		nmap = ATTR_RMTVALUE_MAPSIZE;
 		error = xfs_bmapi_read(args->dp, (xfs_fileoff_t)lblkno,
@@ -415,7 +420,7 @@ xfs_attr_rmtval_set(
 	 * attributes have headers, we can't just do a straight byte to FSB
 	 * conversion and have to take the header space into account.
 	 */
-	blkcnt = xfs_attr3_rmt_blocks(mp, args->valuelen);
+	blkcnt = xfs_attr3_rmt_blocks(mp, args->rmtvaluelen);
 	error = xfs_bmap_first_unused(args->trans, args->dp, blkcnt, &lfileoff,
 						   XFS_ATTR_FORK);
 	if (error)
@@ -480,7 +485,7 @@ xfs_attr_rmtval_set(
 	 */
 	lblkno = args->rmtblkno;
 	blkcnt = args->rmtblkcnt;
-	valuelen = args->valuelen;
+	valuelen = args->rmtvaluelen;
 	while (valuelen > 0) {
 		struct xfs_buf	*bp;
 		xfs_daddr_t	dblkno;
diff --git a/fs/xfs/xfs_da_btree.h b/fs/xfs/xfs_da_btree.h
index 6e95ea7..201c609 100644
--- a/fs/xfs/xfs_da_btree.h
+++ b/fs/xfs/xfs_da_btree.h
@@ -60,10 +60,12 @@ typedef struct xfs_da_args {
 	int		index;		/* index of attr of interest in blk */
 	xfs_dablk_t	rmtblkno;	/* remote attr value starting blkno */
 	int		rmtblkcnt;	/* remote attr value block count */
+	int		rmtvaluelen;	/* remote attr value length in bytes */
 	xfs_dablk_t	blkno2;		/* blkno of 2nd attr leaf of interest */
 	int		index2;		/* index of 2nd attr in blk */
 	xfs_dablk_t	rmtblkno2;	/* remote attr value starting blkno */
 	int		rmtblkcnt2;	/* remote attr value block count */
+	int		rmtvaluelen2;	/* remote attr value length in bytes */
 	int		op_flags;	/* operation flags */
 	enum xfs_dacmp	cmpresult;	/* name compare result for lookups */
 } xfs_da_args_t;
-- 
1.9.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: remote attribute overwrite causes transaction overrun

[Jeff Liu]

On 04/22 2014 14:59 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Commit e461fcb ("xfs: remote attribute lookups require the value
> length") passes the remote attribute length in the xfs_da_args
> structure on lookup so that CRC calculations and validity checking
> can be performed correctly by related code. This, unfortunately has
> the side effect of changing the args->valuelen parameter in cases
> where it shouldn't.
> 
> That is, when we replace a remote attribute, the incoming
> replacement stores the value and length in args->value and
> args->valuelen, but then the lookup which finds the existing remote
> attribute overwrites args->valuelen with the length of the remote
> attribute being replaced. Hence when we go to create the new
> attribute, we create it of the size of the existing remote
> attribute, not the size it is supposed to be. When the new attribute
> is much smaller than the old attribute, this results in a
> transaction overrun and an ASSERT() failure on a debug kernel:
> 
> XFS: Assertion failed: tp->t_blk_res_used <= tp->t_blk_res, file: fs/xfs/xfs_trans.c, line: 331
> 
> Fix this by keeping the remote attribute value length separate to
> the attribute value length in the xfs_da_args structure. The enables
> us to pass the length of the remote attribute to be removed without
> overwriting the new attribute's length.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_attr.c        |  7 ++++++-
>  fs/xfs/xfs_attr_leaf.c   | 21 +++++++++++----------
>  fs/xfs/xfs_attr_remote.c | 11 ++++++++---
>  fs/xfs/xfs_da_btree.h    |  2 ++
>  4 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
> index 01b6a01..dbaa674 100644
> --- a/fs/xfs/xfs_attr.c
> +++ b/fs/xfs/xfs_attr.c
> @@ -213,7 +213,7 @@ xfs_attr_calc_size(
>  		 * Out of line attribute, cannot double split, but
>  		 * make room for the attribute value itself.
>  		 */
> -		uint	dblocks = XFS_B_TO_FSB(mp, valuelen);
> +		uint	dblocks = xfs_attr3_rmt_blocks(mp, valuelen);
>  		nblks += dblocks;
>  		nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK);
>  	}
> @@ -703,6 +703,7 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
>  		args->index2 = args->index;
>  		args->rmtblkno2 = args->rmtblkno;
>  		args->rmtblkcnt2 = args->rmtblkcnt;
> +		args->rmtvaluelen2 = args->rmtvaluelen;
>  	}
>  
>  	/*
> @@ -794,6 +795,7 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
>  		args->blkno = args->blkno2;
>  		args->rmtblkno = args->rmtblkno2;
>  		args->rmtblkcnt = args->rmtblkcnt2;
> +		args->rmtvaluelen = args->rmtvaluelen2;
>  		if (args->rmtblkno) {
>  			error = xfs_attr_rmtval_remove(args);
>  			if (error)
> @@ -1004,8 +1006,10 @@ restart:
>  		args->index2 = args->index;
>  		args->rmtblkno2 = args->rmtblkno;
>  		args->rmtblkcnt2 = args->rmtblkcnt;
> +		args->rmtvaluelen2 = args->rmtvaluelen;
>  		args->rmtblkno = 0;
>  		args->rmtblkcnt = 0;
> +		args->rmtvaluelen = 0;
>  	}
>  
>  	retval = xfs_attr3_leaf_add(blk->bp, state->args);
> @@ -1133,6 +1137,7 @@ restart:
>  		args->blkno = args->blkno2;
>  		args->rmtblkno = args->rmtblkno2;
>  		args->rmtblkcnt = args->rmtblkcnt2;
> +		args->rmtvaluelen = args->rmtvaluelen2;
>  		if (args->rmtblkno) {
>  			error = xfs_attr_rmtval_remove(args);
>  			if (error)
> diff --git a/fs/xfs/xfs_attr_leaf.c b/fs/xfs/xfs_attr_leaf.c
> index fe9587f..511c283 100644
> --- a/fs/xfs/xfs_attr_leaf.c
> +++ b/fs/xfs/xfs_attr_leaf.c
> @@ -1229,6 +1229,7 @@ xfs_attr3_leaf_add_work(
>  		name_rmt->valueblk = 0;
>  		args->rmtblkno = 1;
>  		args->rmtblkcnt = xfs_attr3_rmt_blocks(mp, args->valuelen);
> +		args->rmtvaluelen = args->valuelen;
>  	}
>  	xfs_trans_log_buf(args->trans, bp,
>  	     XFS_DA_LOGRANGE(leaf, xfs_attr3_leaf_name(leaf, args->index),
> @@ -2167,11 +2168,11 @@ xfs_attr3_leaf_lookup_int(
>  			if (!xfs_attr_namesp_match(args->flags, entry->flags))
>  				continue;
>  			args->index = probe;
> -			args->valuelen = be32_to_cpu(name_rmt->valuelen);
> +			args->rmtvaluelen = be32_to_cpu(name_rmt->valuelen);
>  			args->rmtblkno = be32_to_cpu(name_rmt->valueblk);
>  			args->rmtblkcnt = xfs_attr3_rmt_blocks(
>  							args->dp->i_mount,
> -							args->valuelen);
> +							args->rmtvaluelen);
>  			return XFS_ERROR(EEXIST);
>  		}
>  	}
> @@ -2220,19 +2221,19 @@ xfs_attr3_leaf_getvalue(
>  		name_rmt = xfs_attr3_leaf_name_remote(leaf, args->index);
>  		ASSERT(name_rmt->namelen == args->namelen);
>  		ASSERT(memcmp(args->name, name_rmt->name, args->namelen) == 0);
> -		valuelen = be32_to_cpu(name_rmt->valuelen);
> +		args->rmtvaluelen = be32_to_cpu(name_rmt->valuelen);
>  		args->rmtblkno = be32_to_cpu(name_rmt->valueblk);
>  		args->rmtblkcnt = xfs_attr3_rmt_blocks(args->dp->i_mount,
> -						       valuelen);
> +						       args->rmtvaluelen);
>  		if (args->flags & ATTR_KERNOVAL) {
> -			args->valuelen = valuelen;
> +			args->valuelen = args->rmtvaluelen;
>  			return 0;
>  		}
> -		if (args->valuelen < valuelen) {
> -			args->valuelen = valuelen;
> +		if (args->valuelen < args->rmtvaluelen) {
> +			args->valuelen = args->rmtvaluelen;
>  			return XFS_ERROR(ERANGE);
>  		}
> -		args->valuelen = valuelen;
> +		args->valuelen = args->rmtvaluelen;
>  	}
>  	return 0;
>  }
> @@ -2519,7 +2520,7 @@ xfs_attr3_leaf_clearflag(
>  		ASSERT((entry->flags & XFS_ATTR_LOCAL) == 0);
>  		name_rmt = xfs_attr3_leaf_name_remote(leaf, args->index);
>  		name_rmt->valueblk = cpu_to_be32(args->rmtblkno);
> -		name_rmt->valuelen = cpu_to_be32(args->valuelen);
> +		name_rmt->valuelen = cpu_to_be32(args->rmtvaluelen);
>  		xfs_trans_log_buf(args->trans, bp,
>  			 XFS_DA_LOGRANGE(leaf, name_rmt, sizeof(*name_rmt)));
>  	}
> @@ -2677,7 +2678,7 @@ xfs_attr3_leaf_flipflags(
>  		ASSERT((entry1->flags & XFS_ATTR_LOCAL) == 0);
>  		name_rmt = xfs_attr3_leaf_name_remote(leaf1, args->index);
>  		name_rmt->valueblk = cpu_to_be32(args->rmtblkno);
> -		name_rmt->valuelen = cpu_to_be32(args->valuelen);
> +		name_rmt->valuelen = cpu_to_be32(args->rmtvaluelen);
>  		xfs_trans_log_buf(args->trans, bp1,
>  			 XFS_DA_LOGRANGE(leaf1, name_rmt, sizeof(*name_rmt)));
>  	}
> diff --git a/fs/xfs/xfs_attr_remote.c b/fs/xfs/xfs_attr_remote.c
> index 6e37823..2324c66 100644
> --- a/fs/xfs/xfs_attr_remote.c
> +++ b/fs/xfs/xfs_attr_remote.c
> @@ -337,7 +337,7 @@ xfs_attr_rmtval_get(
>  	struct xfs_buf		*bp;
>  	xfs_dablk_t		lblkno = args->rmtblkno;
>  	__uint8_t		*dst = args->value;
> -	int			valuelen = args->valuelen;
> +	int			valuelen;
>  	int			nmap;
>  	int			error;
>  	int			blkcnt = args->rmtblkcnt;
> @@ -348,6 +348,11 @@ xfs_attr_rmtval_get(
>  
>  	ASSERT(!(args->flags & ATTR_KERNOVAL));
>  
> +	/* remote value might be different size to the buffer supplied. */
> +	if (args->rmtvaluelen = args->valuelen)
			     ^^^
Here is a typo...


Thanks,
-Jeff

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: remote attribute overwrite causes transaction overrun

[Dave Chinner]

On Tue, Apr 22, 2014 at 08:00:55PM +0800, Jeff Liu wrote:
> 
> On 04/22 2014 14:59 PM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Commit e461fcb ("xfs: remote attribute lookups require the value
> > length") passes the remote attribute length in the xfs_da_args
> > structure on lookup so that CRC calculations and validity checking
> > can be performed correctly by related code. This, unfortunately has
> > the side effect of changing the args->valuelen parameter in cases
> > where it shouldn't.
> > 
> > That is, when we replace a remote attribute, the incoming
> > replacement stores the value and length in args->value and
> > args->valuelen, but then the lookup which finds the existing remote
> > attribute overwrites args->valuelen with the length of the remote
> > attribute being replaced. Hence when we go to create the new
> > attribute, we create it of the size of the existing remote
> > attribute, not the size it is supposed to be. When the new attribute
> > is much smaller than the old attribute, this results in a
> > transaction overrun and an ASSERT() failure on a debug kernel:
> > 
> > XFS: Assertion failed: tp->t_blk_res_used <= tp->t_blk_res, file: fs/xfs/xfs_trans.c, line: 331
> > 
> > Fix this by keeping the remote attribute value length separate to
> > the attribute value length in the xfs_da_args structure. The enables
> > us to pass the length of the remote attribute to be removed without
> > overwriting the new attribute's length.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
....
> > @@ -348,6 +348,11 @@ xfs_attr_rmtval_get(
> >  
> >  	ASSERT(!(args->flags & ATTR_KERNOVAL));
> >  
> > +	/* remote value might be different size to the buffer supplied. */
> > +	if (args->rmtvaluelen = args->valuelen)
> 			     ^^^
> Here is a typo...

Oh, well spotted.

But, hold on a minute, shouldn't gcc be catching those sorts of
typos?

/me groans

$ gcc t.c
$ gcc -Wall t.c
t.c: In function ¿main¿:
t.c:10:2: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
  if (foo = bar)
  ^
$

Only if -Wparentheses is specified.

Why the hell are we suppressing useful warnings on the kernel build?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: remote attribute overwrite causes transaction overrun

[Dave Chinner]

On Wed, Apr 23, 2014 at 09:46:28AM +1000, Dave Chinner wrote:
> On Tue, Apr 22, 2014 at 08:00:55PM +0800, Jeff Liu wrote:
> > 
> > On 04/22 2014 14:59 PM, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Commit e461fcb ("xfs: remote attribute lookups require the value
> > > length") passes the remote attribute length in the xfs_da_args
> > > structure on lookup so that CRC calculations and validity checking
> > > can be performed correctly by related code. This, unfortunately has
> > > the side effect of changing the args->valuelen parameter in cases
> > > where it shouldn't.
> > > 
> > > That is, when we replace a remote attribute, the incoming
> > > replacement stores the value and length in args->value and
> > > args->valuelen, but then the lookup which finds the existing remote
> > > attribute overwrites args->valuelen with the length of the remote
> > > attribute being replaced. Hence when we go to create the new
> > > attribute, we create it of the size of the existing remote
> > > attribute, not the size it is supposed to be. When the new attribute
> > > is much smaller than the old attribute, this results in a
> > > transaction overrun and an ASSERT() failure on a debug kernel:
> > > 
> > > XFS: Assertion failed: tp->t_blk_res_used <= tp->t_blk_res, file: fs/xfs/xfs_trans.c, line: 331
> > > 
> > > Fix this by keeping the remote attribute value length separate to
> > > the attribute value length in the xfs_da_args structure. The enables
> > > us to pass the length of the remote attribute to be removed without
> > > overwriting the new attribute's length.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ....
> > > @@ -348,6 +348,11 @@ xfs_attr_rmtval_get(
> > >  
> > >  	ASSERT(!(args->flags & ATTR_KERNOVAL));
> > >  
> > > +	/* remote value might be different size to the buffer supplied. */
> > > +	if (args->rmtvaluelen = args->valuelen)
> > 			     ^^^
> > Here is a typo...
> 
> Oh, well spotted.
> 
> But, hold on a minute, shouldn't gcc be catching those sorts of
> typos?
> 
> /me groans
> 
> $ gcc t.c
> $ gcc -Wall t.c
> t.c: In function ¿main¿:
> t.c:10:2: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
>   if (foo = bar)
>   ^
> $
> 
> Only if -Wparentheses is specified.
> 
> Why the hell are we suppressing useful warnings on the kernel build?

Hmmm - looks like I missed the output of the warning when I built
against 3.15-rc2 because it was a rebuild the world and there's tons
of output when that happens...

<sigh>

Time to add ccflags-y += -Werror to the makefile....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: remote attribute overwrite causes transaction overrun

[Eric Sandeen]

On 4/22/14, 7:00 PM, Dave Chinner wrote:
> On Wed, Apr 23, 2014 at 09:46:28AM +1000, Dave Chinner wrote:
>> On Tue, Apr 22, 2014 at 08:00:55PM +0800, Jeff Liu wrote:
>>>
>>> On 04/22 2014 14:59 PM, Dave Chinner wrote:

...
  
>>>> +	/* remote value might be different size to the buffer supplied. */
>>>> +	if (args->rmtvaluelen = args->valuelen)
>>> 			     ^^^
>>> Here is a typo...
>>
>> Oh, well spotted.
>>
>> But, hold on a minute, shouldn't gcc be catching those sorts of
>> typos?
>>
>> /me groans
>>
>> $ gcc t.c
>> $ gcc -Wall t.c
>> t.c: In function ¿main¿:
>> t.c:10:2: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
>>   if (foo = bar)
>>   ^
>> $
>>
>> Only if -Wparentheses is specified.
>>
>> Why the hell are we suppressing useful warnings on the kernel build?
> 
> Hmmm - looks like I missed the output of the warning when I built
> against 3.15-rc2 because it was a rebuild the world and there's tons
> of output when that happens...
> 
> <sigh>
> 
> Time to add ccflags-y += -Werror to the makefile....

Probably only under a couple specific arches...?  How are we for i.e.
stupid printf format warnings about long vs. longlong on 32/64 arches...?

Or maybe you can -Werror, -Wno$FOO?

-Eric
 
> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: remote attribute overwrite causes transaction overrun

[Dave Chinner]

On Tue, Apr 22, 2014 at 10:04:58PM -0500, Eric Sandeen wrote:
> On 4/22/14, 7:00 PM, Dave Chinner wrote:
> > On Wed, Apr 23, 2014 at 09:46:28AM +1000, Dave Chinner wrote:
> >> On Tue, Apr 22, 2014 at 08:00:55PM +0800, Jeff Liu wrote:
> >>>
> >>> On 04/22 2014 14:59 PM, Dave Chinner wrote:
....
> >>>> +	/* remote value might be different size to the buffer supplied. */
> >>>> +	if (args->rmtvaluelen = args->valuelen)
> >>> 			     ^^^
> >>> Here is a typo...
> >>
> >> Oh, well spotted.
> >>
> >> But, hold on a minute, shouldn't gcc be catching those sorts of
> >> typos?
....
> > Time to add ccflags-y += -Werror to the makefile....
> 
> Probably only under a couple specific arches...?  How are we for i.e.
> stupid printf format warnings about long vs. longlong on 32/64 arches...?
> 
> Or maybe you can -Werror, -Wno$FOO?

Personally, if other arches are throwing warnings that we don't know
about, they need to be fixed. The only way I'll get to know about
them is to have somebody else's build break, because nobody is
reporting warnings to us right now....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: remote attribute overwrite causes transaction overrun

[Jeff Liu]

On 04/23 2014 07:46 AM, Dave Chinner wrote:
> On Tue, Apr 22, 2014 at 08:00:55PM +0800, Jeff Liu wrote:
>>
>> On 04/22 2014 14:59 PM, Dave Chinner wrote:
>>> From: Dave Chinner <dchinner@redhat.com>
>>>
>>> Commit e461fcb ("xfs: remote attribute lookups require the value
>>> length") passes the remote attribute length in the xfs_da_args
>>> structure on lookup so that CRC calculations and validity checking
>>> can be performed correctly by related code. This, unfortunately has
>>> the side effect of changing the args->valuelen parameter in cases
>>> where it shouldn't.
>>>
>>> That is, when we replace a remote attribute, the incoming
>>> replacement stores the value and length in args->value and
>>> args->valuelen, but then the lookup which finds the existing remote
>>> attribute overwrites args->valuelen with the length of the remote
>>> attribute being replaced. Hence when we go to create the new
>>> attribute, we create it of the size of the existing remote
>>> attribute, not the size it is supposed to be. When the new attribute
>>> is much smaller than the old attribute, this results in a
>>> transaction overrun and an ASSERT() failure on a debug kernel:
>>>
>>> XFS: Assertion failed: tp->t_blk_res_used <= tp->t_blk_res, file: fs/xfs/xfs_trans.c, line: 331
>>>
>>> Fix this by keeping the remote attribute value length separate to
>>> the attribute value length in the xfs_da_args structure. The enables
>>> us to pass the length of the remote attribute to be removed without
>>> overwriting the new attribute's length.
>>>
>>> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ....
>>> @@ -348,6 +348,11 @@ xfs_attr_rmtval_get(
>>>  
>>>  	ASSERT(!(args->flags & ATTR_KERNOVAL));
>>>  
>>> +	/* remote value might be different size to the buffer supplied. */
>>> +	if (args->rmtvaluelen = args->valuelen)
>> 			     ^^^
>> Here is a typo...
> 
> Oh, well spotted.
> 
> But, hold on a minute, shouldn't gcc be catching those sorts of
> typos?
> 
> /me groans
> 
> $ gcc t.c
> $ gcc -Wall t.c
> t.c: In function ¿main¿:
> t.c:10:2: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
>   if (foo = bar)
>   ^
> $
> 
> Only if -Wparentheses is specified.
> 
> Why the hell are we suppressing useful warnings on the kernel build?
> 

Well, maybe it's due to the different gcc release? I can hit this warning via
gcc-4.8.1 which is the default version on Ubuntu 13.10 as below:

# make SUBDIRS=fs/xfs -j4
...
fs/xfs/xfs_attr_remote.c: In function ‘xfs_attr_rmtval_get’:
fs/xfs/xfs_attr_remote.c:352:2: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
  if (args->rmtvaluelen = args->valuelen)
  ^
  CC [M]  fs/xfs/xfs_bmap_btree.o
  CC [M]  fs/xfs/xfs_btree.o
...


Thanks,
-Jeff

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: remote attribute overwrite causes transaction overrun

[Brian Foster]

On Tue, Apr 22, 2014 at 04:59:09PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Commit e461fcb ("xfs: remote attribute lookups require the value
> length") passes the remote attribute length in the xfs_da_args
> structure on lookup so that CRC calculations and validity checking
> can be performed correctly by related code. This, unfortunately has
> the side effect of changing the args->valuelen parameter in cases
> where it shouldn't.
> 
> That is, when we replace a remote attribute, the incoming
> replacement stores the value and length in args->value and
> args->valuelen, but then the lookup which finds the existing remote
> attribute overwrites args->valuelen with the length of the remote
> attribute being replaced. Hence when we go to create the new
> attribute, we create it of the size of the existing remote
> attribute, not the size it is supposed to be. When the new attribute
> is much smaller than the old attribute, this results in a
> transaction overrun and an ASSERT() failure on a debug kernel:
> 
> XFS: Assertion failed: tp->t_blk_res_used <= tp->t_blk_res, file: fs/xfs/xfs_trans.c, line: 331
> 
> Fix this by keeping the remote attribute value length separate to
> the attribute value length in the xfs_da_args structure. The enables
> us to pass the length of the remote attribute to be removed without
> overwriting the new attribute's length.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_attr.c        |  7 ++++++-
>  fs/xfs/xfs_attr_leaf.c   | 21 +++++++++++----------
>  fs/xfs/xfs_attr_remote.c | 11 ++++++++---
>  fs/xfs/xfs_da_btree.h    |  2 ++
>  4 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
> index 01b6a01..dbaa674 100644
> --- a/fs/xfs/xfs_attr.c
> +++ b/fs/xfs/xfs_attr.c
> @@ -213,7 +213,7 @@ xfs_attr_calc_size(
>  		 * Out of line attribute, cannot double split, but
>  		 * make room for the attribute value itself.
>  		 */
> -		uint	dblocks = XFS_B_TO_FSB(mp, valuelen);
> +		uint	dblocks = xfs_attr3_rmt_blocks(mp, valuelen);
>  		nblks += dblocks;
>  		nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK);
>  	}
> @@ -703,6 +703,7 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
>  		args->index2 = args->index;
>  		args->rmtblkno2 = args->rmtblkno;
>  		args->rmtblkcnt2 = args->rmtblkcnt;
> +		args->rmtvaluelen2 = args->rmtvaluelen;
>  	}

Why don't we zero out the first set of values here similar to the node
case?

>  
>  	/*
> @@ -794,6 +795,7 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
>  		args->blkno = args->blkno2;
>  		args->rmtblkno = args->rmtblkno2;
>  		args->rmtblkcnt = args->rmtblkcnt2;
> +		args->rmtvaluelen = args->rmtvaluelen2;
>  		if (args->rmtblkno) {
>  			error = xfs_attr_rmtval_remove(args);
>  			if (error)
> @@ -1004,8 +1006,10 @@ restart:
>  		args->index2 = args->index;
>  		args->rmtblkno2 = args->rmtblkno;
>  		args->rmtblkcnt2 = args->rmtblkcnt;
> +		args->rmtvaluelen2 = args->rmtvaluelen;
>  		args->rmtblkno = 0;
>  		args->rmtblkcnt = 0;
> +		args->rmtvaluelen = 0;
>  	}
>  
>  	retval = xfs_attr3_leaf_add(blk->bp, state->args);
> @@ -1133,6 +1137,7 @@ restart:
>  		args->blkno = args->blkno2;
>  		args->rmtblkno = args->rmtblkno2;
>  		args->rmtblkcnt = args->rmtblkcnt2;
> +		args->rmtvaluelen = args->rmtvaluelen2;
>  		if (args->rmtblkno) {
>  			error = xfs_attr_rmtval_remove(args);
>  			if (error)
> diff --git a/fs/xfs/xfs_attr_leaf.c b/fs/xfs/xfs_attr_leaf.c
> index fe9587f..511c283 100644
> --- a/fs/xfs/xfs_attr_leaf.c
> +++ b/fs/xfs/xfs_attr_leaf.c
> @@ -1229,6 +1229,7 @@ xfs_attr3_leaf_add_work(
>  		name_rmt->valueblk = 0;
>  		args->rmtblkno = 1;
>  		args->rmtblkcnt = xfs_attr3_rmt_blocks(mp, args->valuelen);
> +		args->rmtvaluelen = args->valuelen;
>  	}
>  	xfs_trans_log_buf(args->trans, bp,
>  	     XFS_DA_LOGRANGE(leaf, xfs_attr3_leaf_name(leaf, args->index),
> @@ -2167,11 +2168,11 @@ xfs_attr3_leaf_lookup_int(
>  			if (!xfs_attr_namesp_match(args->flags, entry->flags))
>  				continue;
>  			args->index = probe;
> -			args->valuelen = be32_to_cpu(name_rmt->valuelen);
> +			args->rmtvaluelen = be32_to_cpu(name_rmt->valuelen);
>  			args->rmtblkno = be32_to_cpu(name_rmt->valueblk);
>  			args->rmtblkcnt = xfs_attr3_rmt_blocks(
>  							args->dp->i_mount,
> -							args->valuelen);
> +							args->rmtvaluelen);
>  			return XFS_ERROR(EEXIST);
>  		}
>  	}
> @@ -2220,19 +2221,19 @@ xfs_attr3_leaf_getvalue(
>  		name_rmt = xfs_attr3_leaf_name_remote(leaf, args->index);
>  		ASSERT(name_rmt->namelen == args->namelen);
>  		ASSERT(memcmp(args->name, name_rmt->name, args->namelen) == 0);
> -		valuelen = be32_to_cpu(name_rmt->valuelen);
> +		args->rmtvaluelen = be32_to_cpu(name_rmt->valuelen);
>  		args->rmtblkno = be32_to_cpu(name_rmt->valueblk);
>  		args->rmtblkcnt = xfs_attr3_rmt_blocks(args->dp->i_mount,
> -						       valuelen);
> +						       args->rmtvaluelen);
>  		if (args->flags & ATTR_KERNOVAL) {
> -			args->valuelen = valuelen;
> +			args->valuelen = args->rmtvaluelen;
>  			return 0;
>  		}
> -		if (args->valuelen < valuelen) {
> -			args->valuelen = valuelen;
> +		if (args->valuelen < args->rmtvaluelen) {
> +			args->valuelen = args->rmtvaluelen;
>  			return XFS_ERROR(ERANGE);
>  		}
> -		args->valuelen = valuelen;
> +		args->valuelen = args->rmtvaluelen;
>  	}
>  	return 0;
>  }
> @@ -2519,7 +2520,7 @@ xfs_attr3_leaf_clearflag(
>  		ASSERT((entry->flags & XFS_ATTR_LOCAL) == 0);
>  		name_rmt = xfs_attr3_leaf_name_remote(leaf, args->index);
>  		name_rmt->valueblk = cpu_to_be32(args->rmtblkno);
> -		name_rmt->valuelen = cpu_to_be32(args->valuelen);
> +		name_rmt->valuelen = cpu_to_be32(args->rmtvaluelen);
>  		xfs_trans_log_buf(args->trans, bp,
>  			 XFS_DA_LOGRANGE(leaf, name_rmt, sizeof(*name_rmt)));
>  	}
> @@ -2677,7 +2678,7 @@ xfs_attr3_leaf_flipflags(
>  		ASSERT((entry1->flags & XFS_ATTR_LOCAL) == 0);
>  		name_rmt = xfs_attr3_leaf_name_remote(leaf1, args->index);
>  		name_rmt->valueblk = cpu_to_be32(args->rmtblkno);
> -		name_rmt->valuelen = cpu_to_be32(args->valuelen);
> +		name_rmt->valuelen = cpu_to_be32(args->rmtvaluelen);
>  		xfs_trans_log_buf(args->trans, bp1,
>  			 XFS_DA_LOGRANGE(leaf1, name_rmt, sizeof(*name_rmt)));
>  	}
> diff --git a/fs/xfs/xfs_attr_remote.c b/fs/xfs/xfs_attr_remote.c
> index 6e37823..2324c66 100644
> --- a/fs/xfs/xfs_attr_remote.c
> +++ b/fs/xfs/xfs_attr_remote.c
> @@ -337,7 +337,7 @@ xfs_attr_rmtval_get(
>  	struct xfs_buf		*bp;
>  	xfs_dablk_t		lblkno = args->rmtblkno;
>  	__uint8_t		*dst = args->value;
> -	int			valuelen = args->valuelen;
> +	int			valuelen;
>  	int			nmap;
>  	int			error;
>  	int			blkcnt = args->rmtblkcnt;
> @@ -348,6 +348,11 @@ xfs_attr_rmtval_get(
>  
>  	ASSERT(!(args->flags & ATTR_KERNOVAL));
>  
> +	/* remote value might be different size to the buffer supplied. */
> +	if (args->rmtvaluelen = args->valuelen)
> +		args->valuelen = args->rmtvaluelen;
> +	valuelen = args->valuelen;
> +

Jeff already called this out as a potential typo...

I'm guessing the intent is to handle the case where we call from
xfs_attr3_leaf_list_int(). The other callers call
xfs_attr3_leaf_getvalue() first, which sets rmtvaluelen and fixes up
valuelen or returns an error (if short). The list_int() case looks like
it queries the remote length just the same, but only sets valuelen...
Why not just set both valuelen and rmtvaluelen there?

Brian

>  	while (valuelen > 0) {
>  		nmap = ATTR_RMTVALUE_MAPSIZE;
>  		error = xfs_bmapi_read(args->dp, (xfs_fileoff_t)lblkno,
> @@ -415,7 +420,7 @@ xfs_attr_rmtval_set(
>  	 * attributes have headers, we can't just do a straight byte to FSB
>  	 * conversion and have to take the header space into account.
>  	 */
> -	blkcnt = xfs_attr3_rmt_blocks(mp, args->valuelen);
> +	blkcnt = xfs_attr3_rmt_blocks(mp, args->rmtvaluelen);
>  	error = xfs_bmap_first_unused(args->trans, args->dp, blkcnt, &lfileoff,
>  						   XFS_ATTR_FORK);
>  	if (error)
> @@ -480,7 +485,7 @@ xfs_attr_rmtval_set(
>  	 */
>  	lblkno = args->rmtblkno;
>  	blkcnt = args->rmtblkcnt;
> -	valuelen = args->valuelen;
> +	valuelen = args->rmtvaluelen;
>  	while (valuelen > 0) {
>  		struct xfs_buf	*bp;
>  		xfs_daddr_t	dblkno;
> diff --git a/fs/xfs/xfs_da_btree.h b/fs/xfs/xfs_da_btree.h
> index 6e95ea7..201c609 100644
> --- a/fs/xfs/xfs_da_btree.h
> +++ b/fs/xfs/xfs_da_btree.h
> @@ -60,10 +60,12 @@ typedef struct xfs_da_args {
>  	int		index;		/* index of attr of interest in blk */
>  	xfs_dablk_t	rmtblkno;	/* remote attr value starting blkno */
>  	int		rmtblkcnt;	/* remote attr value block count */
> +	int		rmtvaluelen;	/* remote attr value length in bytes */
>  	xfs_dablk_t	blkno2;		/* blkno of 2nd attr leaf of interest */
>  	int		index2;		/* index of 2nd attr in blk */
>  	xfs_dablk_t	rmtblkno2;	/* remote attr value starting blkno */
>  	int		rmtblkcnt2;	/* remote attr value block count */
> +	int		rmtvaluelen2;	/* remote attr value length in bytes */
>  	int		op_flags;	/* operation flags */
>  	enum xfs_dacmp	cmpresult;	/* name compare result for lookups */
>  } xfs_da_args_t;
> -- 
> 1.9.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: remote attribute overwrite causes transaction overrun

[Dave Chinner]

On Tue, Apr 22, 2014 at 10:17:44AM -0400, Brian Foster wrote:
> On Tue, Apr 22, 2014 at 04:59:09PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Commit e461fcb ("xfs: remote attribute lookups require the value
> > length") passes the remote attribute length in the xfs_da_args
> > structure on lookup so that CRC calculations and validity checking
> > can be performed correctly by related code. This, unfortunately has
> > the side effect of changing the args->valuelen parameter in cases
> > where it shouldn't.
> > 
> > That is, when we replace a remote attribute, the incoming
> > replacement stores the value and length in args->value and
> > args->valuelen, but then the lookup which finds the existing remote
> > attribute overwrites args->valuelen with the length of the remote
> > attribute being replaced. Hence when we go to create the new
> > attribute, we create it of the size of the existing remote
> > attribute, not the size it is supposed to be. When the new attribute
> > is much smaller than the old attribute, this results in a
> > transaction overrun and an ASSERT() failure on a debug kernel:
> > 
> > XFS: Assertion failed: tp->t_blk_res_used <= tp->t_blk_res, file: fs/xfs/xfs_trans.c, line: 331
> > 
> > Fix this by keeping the remote attribute value length separate to
> > the attribute value length in the xfs_da_args structure. The enables
> > us to pass the length of the remote attribute to be removed without
> > overwriting the new attribute's length.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_attr.c        |  7 ++++++-
> >  fs/xfs/xfs_attr_leaf.c   | 21 +++++++++++----------
> >  fs/xfs/xfs_attr_remote.c | 11 ++++++++---
> >  fs/xfs/xfs_da_btree.h    |  2 ++
> >  4 files changed, 27 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
> > index 01b6a01..dbaa674 100644
> > --- a/fs/xfs/xfs_attr.c
> > +++ b/fs/xfs/xfs_attr.c
> > @@ -213,7 +213,7 @@ xfs_attr_calc_size(
> >  		 * Out of line attribute, cannot double split, but
> >  		 * make room for the attribute value itself.
> >  		 */
> > -		uint	dblocks = XFS_B_TO_FSB(mp, valuelen);
> > +		uint	dblocks = xfs_attr3_rmt_blocks(mp, valuelen);
> >  		nblks += dblocks;
> >  		nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK);
> >  	}
> > @@ -703,6 +703,7 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
> >  		args->index2 = args->index;
> >  		args->rmtblkno2 = args->rmtblkno;
> >  		args->rmtblkcnt2 = args->rmtblkcnt;
> > +		args->rmtvaluelen2 = args->rmtvaluelen;
> >  	}
> 
> Why don't we zero out the first set of values here similar to the node
> case?

I hadn't looked. I was just doing what was consistent with the
current code.

/me looks.

xfs_attr3_leaf_add -> xfs_attr3_leaf_add_work overwrites the rmt*
variables with new values if the newly added attribute needs to be
made remote. But, if it is local, it doesn't zero them.

So, yes, there is potential for the wrong thing to be done later
in this same function as it checks args->rmtblkno assuming that it
is only set when the new attribute is remote.

Different bug, well spotted. :/

I'll add zeroing here.

[FWIW, I've always disliked and been confused by the set of
rmtblkno/rmtblkno2 variables in the xfs_da_args. Mainly their naming
is horrible. Some day I'll write a patch to make it clear what the
two sets of variables are actually for, and maybe write helper
functions for switching between the two sets... ]

> > diff --git a/fs/xfs/xfs_attr_remote.c b/fs/xfs/xfs_attr_remote.c
> > index 6e37823..2324c66 100644
> > --- a/fs/xfs/xfs_attr_remote.c
> > +++ b/fs/xfs/xfs_attr_remote.c
> > @@ -337,7 +337,7 @@ xfs_attr_rmtval_get(
> >  	struct xfs_buf		*bp;
> >  	xfs_dablk_t		lblkno = args->rmtblkno;
> >  	__uint8_t		*dst = args->value;
> > -	int			valuelen = args->valuelen;
> > +	int			valuelen;
> >  	int			nmap;
> >  	int			error;
> >  	int			blkcnt = args->rmtblkcnt;
> > @@ -348,6 +348,11 @@ xfs_attr_rmtval_get(
> >  
> >  	ASSERT(!(args->flags & ATTR_KERNOVAL));
> >  
> > +	/* remote value might be different size to the buffer supplied. */
> > +	if (args->rmtvaluelen = args->valuelen)
> > +		args->valuelen = args->rmtvaluelen;
> > +	valuelen = args->valuelen;
> > +
> 
> Jeff already called this out as a potential typo...
> 
> I'm guessing the intent is to handle the case where we call from
> xfs_attr3_leaf_list_int().

Not really, it was just defensive to ensure that if args->valuelen
and args->rmtvaluelen are different we don't overrun the buffer
supplied by the caller, which is only args->valuelen in size...

> The other callers call
> xfs_attr3_leaf_getvalue() first, which sets rmtvaluelen and fixes up
> valuelen or returns an error (if short). The list_int() case looks like
> it queries the remote length just the same, but only sets valuelen...
> Why not just set both valuelen and rmtvaluelen there?

Seems reasonable. I'll replace the above check with an ASSERT so
that we catch any future mismatches....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[RFC PATCH] xfs: remote attribute overwrite causes transaction overrun

[Sheng Yong]

hi, Greg and Dave Chinner,

This patch is to address CVE-2015-0274, which is caused by commit 7ae077802
("xfs: remote attribute lookups require the value length"). Dave Chinner,
could you review this patch, please? Thank you :-)

Thanks,
Sheng

Sheng Yong (1):
  xfs: remote attribute overwrite causes transaction overrun

 fs/xfs/xfs_attr.c        | 24 +++++++++++++++++++++++-
 fs/xfs/xfs_attr_leaf.c   | 22 ++++++++++++----------
 fs/xfs/xfs_attr_remote.c |  8 +++++---
 fs/xfs/xfs_da_btree.h    |  2 ++
 4 files changed, 42 insertions(+), 14 deletions(-)

-- 
1.8.3.4

[PATCH] xfs: remote attribute overwrite causes transaction overrun

[Sheng Yong]

commit 8275cdd0e7ac550dcce2b3ef6d2fb3b808c1ae59 upstream.

Commit e461fcb ("xfs: remote attribute lookups require the value
length") passes the remote attribute length in the xfs_da_args
structure on lookup so that CRC calculations and validity checking
can be performed correctly by related code. This, unfortunately has
the side effect of changing the args->valuelen parameter in cases
where it shouldn't.

That is, when we replace a remote attribute, the incoming
replacement stores the value and length in args->value and
args->valuelen, but then the lookup which finds the existing remote
attribute overwrites args->valuelen with the length of the remote
attribute being replaced. Hence when we go to create the new
attribute, we create it of the size of the existing remote
attribute, not the size it is supposed to be. When the new attribute
is much smaller than the old attribute, this results in a
transaction overrun and an ASSERT() failure on a debug kernel:

XFS: Assertion failed: tp->t_blk_res_used <= tp->t_blk_res, file: fs/xfs/xfs_trans.c, line: 331

Fix this by keeping the remote attribute value length separate to
the attribute value length in the xfs_da_args structure. The enables
us to pass the length of the remote attribute to be removed without
overwriting the new attribute's length.

Also, ensure that when we save remote block contexts for a later
rename we zero the original state variables so that we don't confuse
the state of the attribute to be removes with the state of the new
attribute that we just added. [Spotted by Brain Foster.]

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Signed-off-by: Dave Chinner <david@fromorbit.com>
[shengyong: backport to 3.10
 - Addresse: CVE-2015-0274
 - adjust context
 - fs/xfs/xfs_attr_list.c comes from fs/xfs/xfs_attr.c and
   fs/xfs/xfs_attr_leaf.c in linux 3.12]
Signed-off-by: Sheng Yong <shengyong1@huawei.com>
---
 fs/xfs/xfs_attr.c        | 24 +++++++++++++++++++++++-
 fs/xfs/xfs_attr_leaf.c   | 22 ++++++++++++----------
 fs/xfs/xfs_attr_remote.c |  8 +++++---
 fs/xfs/xfs_da_btree.h    |  2 ++
 4 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index 20fe3fe..0ccc319 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -211,7 +211,7 @@ xfs_attr_calc_size(
 		 * Out of line attribute, cannot double split, but
 		 * make room for the attribute value itself.
 		 */
-		uint	dblocks = XFS_B_TO_FSB(mp, valuelen);
+		uint	dblocks = xfs_attr3_rmt_blocks(mp, valuelen);
 		nblks += dblocks;
 		nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK);
 	}
@@ -919,11 +919,22 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
 
 		trace_xfs_attr_leaf_replace(args);
 
+		/* save the attribute state for later removal*/
 		args->op_flags |= XFS_DA_OP_RENAME;	/* an atomic rename */
 		args->blkno2 = args->blkno;		/* set 2nd entry info*/
 		args->index2 = args->index;
 		args->rmtblkno2 = args->rmtblkno;
 		args->rmtblkcnt2 = args->rmtblkcnt;
+		args->rmtvaluelen2 = args->rmtvaluelen;
+
+		/*
+		 * clear the remote attr state now that it is saved so that the
+		 * values reflect the state of the attribute we are about to
+		 * add, not the attribute we just found and will remove later.
+		 */
+		args->rmtblkno = 0;
+		args->rmtblkcnt = 0;
+		args->rmtvaluelen = 0;
 	}
 
 	/*
@@ -1015,6 +1026,7 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
 		args->blkno = args->blkno2;
 		args->rmtblkno = args->rmtblkno2;
 		args->rmtblkcnt = args->rmtblkcnt2;
+		args->rmtvaluelen = args->rmtvaluelen2;
 		if (args->rmtblkno) {
 			error = xfs_attr_rmtval_remove(args);
 			if (error)
@@ -1242,13 +1254,22 @@ restart:
 
 		trace_xfs_attr_node_replace(args);
 
+		/* save the attribute state for later removal*/
 		args->op_flags |= XFS_DA_OP_RENAME;	/* atomic rename op */
 		args->blkno2 = args->blkno;		/* set 2nd entry info*/
 		args->index2 = args->index;
 		args->rmtblkno2 = args->rmtblkno;
 		args->rmtblkcnt2 = args->rmtblkcnt;
+		args->rmtvaluelen2 = args->rmtvaluelen;
+
+		/*
+		 * clear the remote attr state now that it is saved so that the
+		 * values reflect the state of the attribute we are about to
+		 * add, not the attribute we just found and will remove later.
+		 */
 		args->rmtblkno = 0;
 		args->rmtblkcnt = 0;
+		args->rmtvaluelen = 0;
 	}
 
 	retval = xfs_attr3_leaf_add(blk->bp, state->args);
@@ -1375,6 +1396,7 @@ restart:
 		args->blkno = args->blkno2;
 		args->rmtblkno = args->rmtblkno2;
 		args->rmtblkcnt = args->rmtblkcnt2;
+		args->rmtvaluelen = args->rmtvaluelen2;
 		if (args->rmtblkno) {
 			error = xfs_attr_rmtval_remove(args);
 			if (error)
diff --git a/fs/xfs/xfs_attr_leaf.c b/fs/xfs/xfs_attr_leaf.c
index 31d3cd1..29054db 100644
--- a/fs/xfs/xfs_attr_leaf.c
+++ b/fs/xfs/xfs_attr_leaf.c
@@ -1413,6 +1413,7 @@ xfs_attr3_leaf_add_work(
 		name_rmt->valueblk = 0;
 		args->rmtblkno = 1;
 		args->rmtblkcnt = xfs_attr3_rmt_blocks(mp, args->valuelen);
+		args->rmtvaluelen = args->valuelen;
 	}
 	xfs_trans_log_buf(args->trans, bp,
 	     XFS_DA_LOGRANGE(leaf, xfs_attr3_leaf_name(leaf, args->index),
@@ -2352,11 +2353,11 @@ xfs_attr3_leaf_lookup_int(
 			if (!xfs_attr_namesp_match(args->flags, entry->flags))
 				continue;
 			args->index = probe;
-			args->valuelen = be32_to_cpu(name_rmt->valuelen);
+			args->rmtvaluelen = be32_to_cpu(name_rmt->valuelen);
 			args->rmtblkno = be32_to_cpu(name_rmt->valueblk);
 			args->rmtblkcnt = xfs_attr3_rmt_blocks(
 							args->dp->i_mount,
-							args->valuelen);
+							args->rmtvaluelen);
 			return XFS_ERROR(EEXIST);
 		}
 	}
@@ -2405,19 +2406,19 @@ xfs_attr3_leaf_getvalue(
 		name_rmt = xfs_attr3_leaf_name_remote(leaf, args->index);
 		ASSERT(name_rmt->namelen == args->namelen);
 		ASSERT(memcmp(args->name, name_rmt->name, args->namelen) == 0);
-		valuelen = be32_to_cpu(name_rmt->valuelen);
+		args->rmtvaluelen = be32_to_cpu(name_rmt->valuelen);
 		args->rmtblkno = be32_to_cpu(name_rmt->valueblk);
 		args->rmtblkcnt = xfs_attr3_rmt_blocks(args->dp->i_mount,
-						       valuelen);
+						       args->rmtvaluelen);
 		if (args->flags & ATTR_KERNOVAL) {
-			args->valuelen = valuelen;
+			args->valuelen = args->rmtvaluelen;
 			return 0;
 		}
-		if (args->valuelen < valuelen) {
-			args->valuelen = valuelen;
+		if (args->valuelen < args->rmtvaluelen) {
+			args->valuelen = args->rmtvaluelen;
 			return XFS_ERROR(ERANGE);
 		}
-		args->valuelen = valuelen;
+		args->valuelen = args->rmtvaluelen;
 	}
 	return 0;
 }
@@ -2732,6 +2733,7 @@ xfs_attr3_leaf_list_int(
 				args.dp = context->dp;
 				args.whichfork = XFS_ATTR_FORK;
 				args.valuelen = valuelen;
+				args.rmtvaluelen = valuelen;
 				args.value = kmem_alloc(valuelen, KM_SLEEP | KM_NOFS);
 				args.rmtblkno = be32_to_cpu(name_rmt->valueblk);
 				args.rmtblkcnt = xfs_attr3_rmt_blocks(
@@ -2828,7 +2830,7 @@ xfs_attr3_leaf_clearflag(
 		ASSERT((entry->flags & XFS_ATTR_LOCAL) == 0);
 		name_rmt = xfs_attr3_leaf_name_remote(leaf, args->index);
 		name_rmt->valueblk = cpu_to_be32(args->rmtblkno);
-		name_rmt->valuelen = cpu_to_be32(args->valuelen);
+		name_rmt->valuelen = cpu_to_be32(args->rmtvaluelen);
 		xfs_trans_log_buf(args->trans, bp,
 			 XFS_DA_LOGRANGE(leaf, name_rmt, sizeof(*name_rmt)));
 	}
@@ -2986,7 +2988,7 @@ xfs_attr3_leaf_flipflags(
 		ASSERT((entry1->flags & XFS_ATTR_LOCAL) == 0);
 		name_rmt = xfs_attr3_leaf_name_remote(leaf1, args->index);
 		name_rmt->valueblk = cpu_to_be32(args->rmtblkno);
-		name_rmt->valuelen = cpu_to_be32(args->valuelen);
+		name_rmt->valuelen = cpu_to_be32(args->rmtvaluelen);
 		xfs_trans_log_buf(args->trans, bp1,
 			 XFS_DA_LOGRANGE(leaf1, name_rmt, sizeof(*name_rmt)));
 	}
diff --git a/fs/xfs/xfs_attr_remote.c b/fs/xfs/xfs_attr_remote.c
index ef6b0c1..2006e0d 100644
--- a/fs/xfs/xfs_attr_remote.c
+++ b/fs/xfs/xfs_attr_remote.c
@@ -338,7 +338,7 @@ xfs_attr_rmtval_get(
 	struct xfs_buf		*bp;
 	xfs_dablk_t		lblkno = args->rmtblkno;
 	char			*dst = args->value;
-	int			valuelen = args->valuelen;
+	int			valuelen;
 	int			nmap;
 	int			error;
 	int			blkcnt = args->rmtblkcnt;
@@ -348,7 +348,9 @@ xfs_attr_rmtval_get(
 	trace_xfs_attr_rmtval_get(args);
 
 	ASSERT(!(args->flags & ATTR_KERNOVAL));
+	ASSERT(args->rmtvaluelen == args->valuelen);
 
+	valuelen = args->rmtvaluelen;
 	while (valuelen > 0) {
 		nmap = ATTR_RMTVALUE_MAPSIZE;
 		error = xfs_bmapi_read(args->dp, (xfs_fileoff_t)lblkno,
@@ -416,7 +418,7 @@ xfs_attr_rmtval_set(
 	 * attributes have headers, we can't just do a straight byte to FSB
 	 * conversion and have to take the header space into account.
 	 */
-	blkcnt = xfs_attr3_rmt_blocks(mp, args->valuelen);
+	blkcnt = xfs_attr3_rmt_blocks(mp, args->rmtvaluelen);
 	error = xfs_bmap_first_unused(args->trans, args->dp, blkcnt, &lfileoff,
 						   XFS_ATTR_FORK);
 	if (error)
@@ -481,7 +483,7 @@ xfs_attr_rmtval_set(
 	 */
 	lblkno = args->rmtblkno;
 	blkcnt = args->rmtblkcnt;
-	valuelen = args->valuelen;
+	valuelen = args->rmtvaluelen;
 	while (valuelen > 0) {
 		struct xfs_buf	*bp;
 		xfs_daddr_t	dblkno;
diff --git a/fs/xfs/xfs_da_btree.h b/fs/xfs/xfs_da_btree.h
index 6fb3371c..0c4772e 100644
--- a/fs/xfs/xfs_da_btree.h
+++ b/fs/xfs/xfs_da_btree.h
@@ -191,10 +191,12 @@ typedef struct xfs_da_args {
 	int		index;		/* index of attr of interest in blk */
 	xfs_dablk_t	rmtblkno;	/* remote attr value starting blkno */
 	int		rmtblkcnt;	/* remote attr value block count */
+	int		rmtvaluelen;	/* remote attr value length in bytes */
 	xfs_dablk_t	blkno2;		/* blkno of 2nd attr leaf of interest */
 	int		index2;		/* index of 2nd attr in blk */
 	xfs_dablk_t	rmtblkno2;	/* remote attr value starting blkno */
 	int		rmtblkcnt2;	/* remote attr value block count */
+	int		rmtvaluelen2;	/* remote attr value length in bytes */
 	int		op_flags;	/* operation flags */
 	enum xfs_dacmp	cmpresult;	/* name compare result for lookups */
 } xfs_da_args_t;
-- 
1.8.3.4

Re: [PATCH] xfs: remote attribute overwrite causes transaction overrun

[Dave Chinner]

On Wed, Apr 29, 2015 at 01:55:25AM +0000, Sheng Yong wrote:
> commit 8275cdd0e7ac550dcce2b3ef6d2fb3b808c1ae59 upstream.
> 
> Commit e461fcb ("xfs: remote attribute lookups require the value
> length") passes the remote attribute length in the xfs_da_args
> structure on lookup so that CRC calculations and validity checking
> can be performed correctly by related code. This, unfortunately has
> the side effect of changing the args->valuelen parameter in cases
> where it shouldn't.
> 
> That is, when we replace a remote attribute, the incoming
> replacement stores the value and length in args->value and
> args->valuelen, but then the lookup which finds the existing remote
> attribute overwrites args->valuelen with the length of the remote
> attribute being replaced. Hence when we go to create the new
> attribute, we create it of the size of the existing remote
> attribute, not the size it is supposed to be. When the new attribute
> is much smaller than the old attribute, this results in a
> transaction overrun and an ASSERT() failure on a debug kernel:
> 
> XFS: Assertion failed: tp->t_blk_res_used <= tp->t_blk_res, file: fs/xfs/xfs_trans.c, line: 331
> 
> Fix this by keeping the remote attribute value length separate to
> the attribute value length in the xfs_da_args structure. The enables
> us to pass the length of the remote attribute to be removed without
> overwriting the new attribute's length.
> 
> Also, ensure that when we save remote block contexts for a later
> rename we zero the original state variables so that we don't confuse
> the state of the attribute to be removes with the state of the new
> attribute that we just added. [Spotted by Brain Foster.]
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> Signed-off-by: Dave Chinner <david@fromorbit.com>
> [shengyong: backport to 3.10
>  - Addresse: CVE-2015-0274
>  - adjust context
>  - fs/xfs/xfs_attr_list.c comes from fs/xfs/xfs_attr.c and
>    fs/xfs/xfs_attr_leaf.c in linux 3.12]
> Signed-off-by: Sheng Yong <shengyong1@huawei.com>

You are backporting to 3.10?

Check when Commit e461fcb ("xfs: remote attribute lookups require
the value length") was introduced:

$ git describe --contains e461fcb
for-linus-v3.11-rc1~53
$

Seems to me like the problem the CVE describes didn't exist in 3.10...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: remote attribute overwrite causes transaction overrun

[Sheng Yong]

Hi, Dave,
Thanks for your review.

On 4/29/2015 3:43 PM, Dave Chinner wrote:
> On Wed, Apr 29, 2015 at 01:55:25AM +0000, Sheng Yong wrote:
>> commit 8275cdd0e7ac550dcce2b3ef6d2fb3b808c1ae59 upstream.
>>
>> Commit e461fcb ("xfs: remote attribute lookups require the value
>> length") passes the remote attribute length in the xfs_da_args
>> structure on lookup so that CRC calculations and validity checking
>> can be performed correctly by related code. This, unfortunately has
>> the side effect of changing the args->valuelen parameter in cases
>> where it shouldn't.
>>
>> That is, when we replace a remote attribute, the incoming
>> replacement stores the value and length in args->value and
>> args->valuelen, but then the lookup which finds the existing remote
>> attribute overwrites args->valuelen with the length of the remote
>> attribute being replaced. Hence when we go to create the new
>> attribute, we create it of the size of the existing remote
>> attribute, not the size it is supposed to be. When the new attribute
>> is much smaller than the old attribute, this results in a
>> transaction overrun and an ASSERT() failure on a debug kernel:
>>
>> XFS: Assertion failed: tp->t_blk_res_used <= tp->t_blk_res, file: fs/xfs/xfs_trans.c, line: 331
>>
>> Fix this by keeping the remote attribute value length separate to
>> the attribute value length in the xfs_da_args structure. The enables
>> us to pass the length of the remote attribute to be removed without
>> overwriting the new attribute's length.
>>
>> Also, ensure that when we save remote block contexts for a later
>> rename we zero the original state variables so that we don't confuse
>> the state of the attribute to be removes with the state of the new
>> attribute that we just added. [Spotted by Brain Foster.]
>>
>> Signed-off-by: Dave Chinner <dchinner@redhat.com>
>> Reviewed-by: Brian Foster <bfoster@redhat.com>
>> Signed-off-by: Dave Chinner <david@fromorbit.com>
>> [shengyong: backport to 3.10
>>  - Addresse: CVE-2015-0274
>>  - adjust context
>>  - fs/xfs/xfs_attr_list.c comes from fs/xfs/xfs_attr.c and
>>    fs/xfs/xfs_attr_leaf.c in linux 3.12]
>> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
> 
> You are backporting to 3.10?
> 
> Check when Commit e461fcb ("xfs: remote attribute lookups require
> the value length") was introduced:
You are right, commit e461fcb is not merged into 3.10, but as I mentioned in
the cover letter, commit 7ae077802 cherry-picked from e461fcb, and the cherry-pick
one is merged into 3.10 :-)

linux-stable$ git log --oneline 7ae077802 -n 1
7ae0778 xfs: remote attribute lookups require the value length

linux-stable$ git describe --contains 7ae077802
v3.10-rc3~5^2

thanks,
Sheng
> 
> $ git describe --contains e461fcb
> for-linus-v3.11-rc1~53
> $
> 
> Seems to me like the problem the CVE describes didn't exist in 3.10...
> 
> Cheers,
> 
> Dave.
> 

Re: [PATCH] xfs: remote attribute overwrite causes transaction overrun

[Greg KH]

On Wed, Apr 29, 2015 at 05:05:17PM +0800, Sheng Yong wrote:
> Hi, Dave,
> Thanks for your review.
> 
> On 4/29/2015 3:43 PM, Dave Chinner wrote:
> > On Wed, Apr 29, 2015 at 01:55:25AM +0000, Sheng Yong wrote:
> >> commit 8275cdd0e7ac550dcce2b3ef6d2fb3b808c1ae59 upstream.
> >>
> >> Commit e461fcb ("xfs: remote attribute lookups require the value
> >> length") passes the remote attribute length in the xfs_da_args
> >> structure on lookup so that CRC calculations and validity checking
> >> can be performed correctly by related code. This, unfortunately has
> >> the side effect of changing the args->valuelen parameter in cases
> >> where it shouldn't.
> >>
> >> That is, when we replace a remote attribute, the incoming
> >> replacement stores the value and length in args->value and
> >> args->valuelen, but then the lookup which finds the existing remote
> >> attribute overwrites args->valuelen with the length of the remote
> >> attribute being replaced. Hence when we go to create the new
> >> attribute, we create it of the size of the existing remote
> >> attribute, not the size it is supposed to be. When the new attribute
> >> is much smaller than the old attribute, this results in a
> >> transaction overrun and an ASSERT() failure on a debug kernel:
> >>
> >> XFS: Assertion failed: tp->t_blk_res_used <= tp->t_blk_res, file: fs/xfs/xfs_trans.c, line: 331
> >>
> >> Fix this by keeping the remote attribute value length separate to
> >> the attribute value length in the xfs_da_args structure. The enables
> >> us to pass the length of the remote attribute to be removed without
> >> overwriting the new attribute's length.
> >>
> >> Also, ensure that when we save remote block contexts for a later
> >> rename we zero the original state variables so that we don't confuse
> >> the state of the attribute to be removes with the state of the new
> >> attribute that we just added. [Spotted by Brain Foster.]
> >>
> >> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> >> Reviewed-by: Brian Foster <bfoster@redhat.com>
> >> Signed-off-by: Dave Chinner <david@fromorbit.com>
> >> [shengyong: backport to 3.10
> >>  - Addresse: CVE-2015-0274
> >>  - adjust context
> >>  - fs/xfs/xfs_attr_list.c comes from fs/xfs/xfs_attr.c and
> >>    fs/xfs/xfs_attr_leaf.c in linux 3.12]
> >> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
> > 
> > You are backporting to 3.10?
> > 
> > Check when Commit e461fcb ("xfs: remote attribute lookups require
> > the value length") was introduced:
> You are right, commit e461fcb is not merged into 3.10, but as I mentioned in
> the cover letter, commit 7ae077802 cherry-picked from e461fcb, and the cherry-pick
> one is merged into 3.10 :-)
> 
> linux-stable$ git log --oneline 7ae077802 -n 1
> 7ae0778 xfs: remote attribute lookups require the value length
> 
> linux-stable$ git describe --contains 7ae077802
> v3.10-rc3~5^2

I don't understand this at all, what do you mean?

confused,

greg k-h

Re: [PATCH] xfs: remote attribute overwrite causes transaction overrun

[Sheng Yong]

Hi, Greg

On 5/3/2015 2:36 AM, Greg KH wrote:
> On Wed, Apr 29, 2015 at 05:05:17PM +0800, Sheng Yong wrote:
>> Hi, Dave,
>> Thanks for your review.
>>
>> On 4/29/2015 3:43 PM, Dave Chinner wrote:
>>> On Wed, Apr 29, 2015 at 01:55:25AM +0000, Sheng Yong wrote:
>>>> commit 8275cdd0e7ac550dcce2b3ef6d2fb3b808c1ae59 upstream.
>>>>
>>>> Commit e461fcb ("xfs: remote attribute lookups require the value
>>>> length") passes the remote attribute length in the xfs_da_args
>>>> structure on lookup so that CRC calculations and validity checking
>>>> can be performed correctly by related code. This, unfortunately has
>>>> the side effect of changing the args->valuelen parameter in cases
>>>> where it shouldn't.
>>>>
>>>> That is, when we replace a remote attribute, the incoming
>>>> replacement stores the value and length in args->value and
>>>> args->valuelen, but then the lookup which finds the existing remote
>>>> attribute overwrites args->valuelen with the length of the remote
>>>> attribute being replaced. Hence when we go to create the new
>>>> attribute, we create it of the size of the existing remote
>>>> attribute, not the size it is supposed to be. When the new attribute
>>>> is much smaller than the old attribute, this results in a
>>>> transaction overrun and an ASSERT() failure on a debug kernel:
>>>>
>>>> XFS: Assertion failed: tp->t_blk_res_used <= tp->t_blk_res, file: fs/xfs/xfs_trans.c, line: 331
>>>>
>>>> Fix this by keeping the remote attribute value length separate to
>>>> the attribute value length in the xfs_da_args structure. The enables
>>>> us to pass the length of the remote attribute to be removed without
>>>> overwriting the new attribute's length.
>>>>
>>>> Also, ensure that when we save remote block contexts for a later
>>>> rename we zero the original state variables so that we don't confuse
>>>> the state of the attribute to be removes with the state of the new
>>>> attribute that we just added. [Spotted by Brain Foster.]
>>>>
>>>> Signed-off-by: Dave Chinner <dchinner@redhat.com>
>>>> Reviewed-by: Brian Foster <bfoster@redhat.com>
>>>> Signed-off-by: Dave Chinner <david@fromorbit.com>
>>>> [shengyong: backport to 3.10
>>>>  - Addresse: CVE-2015-0274
>>>>  - adjust context
>>>>  - fs/xfs/xfs_attr_list.c comes from fs/xfs/xfs_attr.c and
>>>>    fs/xfs/xfs_attr_leaf.c in linux 3.12]
>>>> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
>>>
>>> You are backporting to 3.10?
>>>
>>> Check when Commit e461fcb ("xfs: remote attribute lookups require
>>> the value length") was introduced:
>> You are right, commit e461fcb is not merged into 3.10, but as I mentioned in
>> the cover letter, commit 7ae077802 cherry-picked from e461fcb, and the cherry-pick
>> one is merged into 3.10 :-)
>>
>> linux-stable$ git log --oneline 7ae077802 -n 1
>> 7ae0778 xfs: remote attribute lookups require the value length
>>
>> linux-stable$ git describe --contains 7ae077802
>> v3.10-rc3~5^2
> 
> I don't understand this at all, what do you mean?
> 
> confused,
CVE-2015-0274 is caused by commit e461fcb ("xfs: remote attribute lookups require
the value length"), which was introduced in 3.11. It should have had nothing to
do with 3.10-stable. However, when we checked 3.10, we found that this commit was
check-picked from (maybe) the xfs tree. The patch ("xfs: remote attribute lookups
require the value length") was also included in 3.10, and its commit is 7ae077802.
So 3.10-stable is affected by the CVE.

thanks,
Sheng
> 
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe stable" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

Re: [PATCH] xfs: remote attribute overwrite causes transaction overrun

[Dave Chinner]

On Mon, May 04, 2015 at 09:45:36AM +0800, Sheng Yong wrote:
> CVE-2015-0274 is caused by commit e461fcb ("xfs: remote attribute
> lookups require the value length"), which was introduced in 3.11.
> It should have had nothing to do with 3.10-stable. However, when
> we checked 3.10, we found that this commit was check-picked from
> (maybe) the xfs tree. The patch ("xfs: remote attribute lookups
> require the value length") was also included in 3.10, and its
> commit is 7ae077802.  So 3.10-stable is affected by the CVE.

So aparently it is needed if there is any bug exposed, which I can't
tell from the complete lack of information in the CVE about it.

Indeed, I originally found the problem on a CONFIG_XFS_DEBUG=y
kernel, which flags lots of issues that have no effect on run-time
behaviour of production systems. A transaction overrun does *not*
cause a production system to fail, so I'm really not sure that there
is a problem that needs to be fixed here.

Regardless of whether there is a bug to be fixed or not, what
regression testing have you done on your backport to ensure you
haven't introduced any new problems?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com