In the case of returning -ENOSPC, ensure logflagsp is initialized by 0.
Otherwise the caller __xfs_bunmapi will set uninitialized illegal
tmp_logflags value into xfs log, which might cause unpredictable error
in the log recovery procedure.
Also, remove the flags variable and set the *logflagsp directly, so that
the code should be more robust in the long run.
Fixes: 1b24b633aafe ("xfs: move some more code into xfs_bmap_del_extent_real")
Signed-off-by: Jiachen Zhang <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
fs/xfs/libxfs/xfs_bmap.c | 73 +++++++++++++++++-----------------------
1 file changed, 31 insertions(+), 42 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index be62acffad6c..eacd7f43c952 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5010,7 +5010,6 @@ xfs_bmap_del_extent_real(
xfs_fileoff_t del_endoff; /* first offset past del */
int do_fx; /* free extent at end of routine */
int error; /* error return value */
- int flags = 0;/* inode logging flags */
struct xfs_bmbt_irec got; /* current extent entry */
xfs_fileoff_t got_endoff; /* first offset past got */
int i; /* temp state */
@@ -5023,6 +5022,8 @@ xfs_bmap_del_extent_real(
uint32_t state = xfs_bmap_fork_to_state(whichfork);
struct xfs_bmbt_irec old;
+ *logflagsp = 0;
+
mp = ip->i_mount;
XFS_STATS_INC(mp, xs_del_exlist);
@@ -5035,7 +5036,6 @@ xfs_bmap_del_extent_real(
ASSERT(got_endoff >= del_endoff);
ASSERT(!isnullstartblock(got.br_startblock));
qfield = 0;
- error = 0;
/*
* If it's the case where the directory code is running with no block
@@ -5051,13 +5051,13 @@ xfs_bmap_del_extent_real(
del->br_startoff > got.br_startoff && del_endoff < got_endoff)
return -ENOSPC;
- flags = XFS_ILOG_CORE;
+ *logflagsp = XFS_ILOG_CORE;
if (whichfork == XFS_DATA_FORK && XFS_IS_REALTIME_INODE(ip)) {
if (!(bflags & XFS_BMAPI_REMAP)) {
error = xfs_rtfree_blocks(tp, del->br_startblock,
del->br_blockcount);
if (error)
- goto done;
+ return error;
}
do_fx = 0;
@@ -5072,11 +5072,9 @@ xfs_bmap_del_extent_real(
if (cur) {
error = xfs_bmbt_lookup_eq(cur, &got, &i);
if (error)
- goto done;
- if (XFS_IS_CORRUPT(mp, i != 1)) {
- error = -EFSCORRUPTED;
- goto done;
- }
+ return error;
+ if (XFS_IS_CORRUPT(mp, i != 1))
+ return -EFSCORRUPTED;
}
if (got.br_startoff == del->br_startoff)
@@ -5093,17 +5091,15 @@ xfs_bmap_del_extent_real(
xfs_iext_prev(ifp, icur);
ifp->if_nextents--;
- flags |= XFS_ILOG_CORE;
+ *logflagsp |= XFS_ILOG_CORE;
if (!cur) {
- flags |= xfs_ilog_fext(whichfork);
+ *logflagsp |= xfs_ilog_fext(whichfork);
break;
}
if ((error = xfs_btree_delete(cur, &i)))
- goto done;
- if (XFS_IS_CORRUPT(mp, i != 1)) {
- error = -EFSCORRUPTED;
- goto done;
- }
+ return error;
+ if (XFS_IS_CORRUPT(mp, i != 1))
+ return -EFSCORRUPTED;
break;
case BMAP_LEFT_FILLING:
/*
@@ -5114,12 +5110,12 @@ xfs_bmap_del_extent_real(
got.br_blockcount -= del->br_blockcount;
xfs_iext_update_extent(ip, state, icur, &got);
if (!cur) {
- flags |= xfs_ilog_fext(whichfork);
+ *logflagsp |= xfs_ilog_fext(whichfork);
break;
}
error = xfs_bmbt_update(cur, &got);
if (error)
- goto done;
+ return error;
break;
case BMAP_RIGHT_FILLING:
/*
@@ -5128,12 +5124,12 @@ xfs_bmap_del_extent_real(
got.br_blockcount -= del->br_blockcount;
xfs_iext_update_extent(ip, state, icur, &got);
if (!cur) {
- flags |= xfs_ilog_fext(whichfork);
+ *logflagsp |= xfs_ilog_fext(whichfork);
break;
}
error = xfs_bmbt_update(cur, &got);
if (error)
- goto done;
+ return error;
break;
case 0:
/*
@@ -5150,18 +5146,18 @@ xfs_bmap_del_extent_real(
new.br_state = got.br_state;
new.br_startblock = del_endblock;
- flags |= XFS_ILOG_CORE;
+ *logflagsp |= XFS_ILOG_CORE;
if (cur) {
error = xfs_bmbt_update(cur, &got);
if (error)
- goto done;
+ return error;
error = xfs_btree_increment(cur, 0, &i);
if (error)
- goto done;
+ return error;
cur->bc_rec.b = new;
error = xfs_btree_insert(cur, &i);
if (error && error != -ENOSPC)
- goto done;
+ return error;
/*
* If get no-space back from btree insert, it tried a
* split, and we have a zero block reservation. Fix up
@@ -5174,33 +5170,28 @@ xfs_bmap_del_extent_real(
*/
error = xfs_bmbt_lookup_eq(cur, &got, &i);
if (error)
- goto done;
- if (XFS_IS_CORRUPT(mp, i != 1)) {
- error = -EFSCORRUPTED;
- goto done;
- }
+ return error;
+ if (XFS_IS_CORRUPT(mp, i != 1))
+ return -EFSCORRUPTED;
/*
* Update the btree record back
* to the original value.
*/
error = xfs_bmbt_update(cur, &old);
if (error)
- goto done;
+ return error;
/*
* Reset the extent record back
* to the original value.
*/
xfs_iext_update_extent(ip, state, icur, &old);
- flags = 0;
- error = -ENOSPC;
- goto done;
- }
- if (XFS_IS_CORRUPT(mp, i != 1)) {
- error = -EFSCORRUPTED;
- goto done;
+ *logflagsp = 0;
+ return -ENOSPC;
}
+ if (XFS_IS_CORRUPT(mp, i != 1))
+ return -EFSCORRUPTED;
} else
- flags |= xfs_ilog_fext(whichfork);
+ *logflagsp |= xfs_ilog_fext(whichfork);
ifp->if_nextents++;
xfs_iext_next(ifp, icur);
@@ -5224,7 +5215,7 @@ xfs_bmap_del_extent_real(
((bflags & XFS_BMAPI_NODISCARD) ||
del->br_state == XFS_EXT_UNWRITTEN));
if (error)
- goto done;
+ return error;
}
}
@@ -5239,9 +5230,7 @@ xfs_bmap_del_extent_real(
if (qfield && !(bflags & XFS_BMAPI_REMAP))
xfs_trans_mod_dquot_byino(tp, ip, qfield, (long)-nblks);
-done:
- *logflagsp = flags;
- return error;
+ return 0;
}
/*
--
2.20.1
On Thu, Nov 30, 2023 at 12:05:14PM +0800, Jiachen Zhang wrote:
> In the case of returning -ENOSPC, ensure logflagsp is initialized by 0.
> Otherwise the caller __xfs_bunmapi will set uninitialized illegal
> tmp_logflags value into xfs log, which might cause unpredictable error
> in the log recovery procedure.
>
> Also, remove the flags variable and set the *logflagsp directly, so that
> the code should be more robust in the long run.
>
> Fixes: 1b24b633aafe ("xfs: move some more code into xfs_bmap_del_extent_real")
> Signed-off-by: Jiachen Zhang <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
Looks good,
Reviewed-by: Darrick J. Wong <[email protected]>
--D
> ---
> fs/xfs/libxfs/xfs_bmap.c | 73 +++++++++++++++++-----------------------
> 1 file changed, 31 insertions(+), 42 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index be62acffad6c..eacd7f43c952 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5010,7 +5010,6 @@ xfs_bmap_del_extent_real(
> xfs_fileoff_t del_endoff; /* first offset past del */
> int do_fx; /* free extent at end of routine */
> int error; /* error return value */
> - int flags = 0;/* inode logging flags */
> struct xfs_bmbt_irec got; /* current extent entry */
> xfs_fileoff_t got_endoff; /* first offset past got */
> int i; /* temp state */
> @@ -5023,6 +5022,8 @@ xfs_bmap_del_extent_real(
> uint32_t state = xfs_bmap_fork_to_state(whichfork);
> struct xfs_bmbt_irec old;
>
> + *logflagsp = 0;
> +
> mp = ip->i_mount;
> XFS_STATS_INC(mp, xs_del_exlist);
>
> @@ -5035,7 +5036,6 @@ xfs_bmap_del_extent_real(
> ASSERT(got_endoff >= del_endoff);
> ASSERT(!isnullstartblock(got.br_startblock));
> qfield = 0;
> - error = 0;
>
> /*
> * If it's the case where the directory code is running with no block
> @@ -5051,13 +5051,13 @@ xfs_bmap_del_extent_real(
> del->br_startoff > got.br_startoff && del_endoff < got_endoff)
> return -ENOSPC;
>
> - flags = XFS_ILOG_CORE;
> + *logflagsp = XFS_ILOG_CORE;
> if (whichfork == XFS_DATA_FORK && XFS_IS_REALTIME_INODE(ip)) {
> if (!(bflags & XFS_BMAPI_REMAP)) {
> error = xfs_rtfree_blocks(tp, del->br_startblock,
> del->br_blockcount);
> if (error)
> - goto done;
> + return error;
> }
>
> do_fx = 0;
> @@ -5072,11 +5072,9 @@ xfs_bmap_del_extent_real(
> if (cur) {
> error = xfs_bmbt_lookup_eq(cur, &got, &i);
> if (error)
> - goto done;
> - if (XFS_IS_CORRUPT(mp, i != 1)) {
> - error = -EFSCORRUPTED;
> - goto done;
> - }
> + return error;
> + if (XFS_IS_CORRUPT(mp, i != 1))
> + return -EFSCORRUPTED;
> }
>
> if (got.br_startoff == del->br_startoff)
> @@ -5093,17 +5091,15 @@ xfs_bmap_del_extent_real(
> xfs_iext_prev(ifp, icur);
> ifp->if_nextents--;
>
> - flags |= XFS_ILOG_CORE;
> + *logflagsp |= XFS_ILOG_CORE;
> if (!cur) {
> - flags |= xfs_ilog_fext(whichfork);
> + *logflagsp |= xfs_ilog_fext(whichfork);
> break;
> }
> if ((error = xfs_btree_delete(cur, &i)))
> - goto done;
> - if (XFS_IS_CORRUPT(mp, i != 1)) {
> - error = -EFSCORRUPTED;
> - goto done;
> - }
> + return error;
> + if (XFS_IS_CORRUPT(mp, i != 1))
> + return -EFSCORRUPTED;
> break;
> case BMAP_LEFT_FILLING:
> /*
> @@ -5114,12 +5110,12 @@ xfs_bmap_del_extent_real(
> got.br_blockcount -= del->br_blockcount;
> xfs_iext_update_extent(ip, state, icur, &got);
> if (!cur) {
> - flags |= xfs_ilog_fext(whichfork);
> + *logflagsp |= xfs_ilog_fext(whichfork);
> break;
> }
> error = xfs_bmbt_update(cur, &got);
> if (error)
> - goto done;
> + return error;
> break;
> case BMAP_RIGHT_FILLING:
> /*
> @@ -5128,12 +5124,12 @@ xfs_bmap_del_extent_real(
> got.br_blockcount -= del->br_blockcount;
> xfs_iext_update_extent(ip, state, icur, &got);
> if (!cur) {
> - flags |= xfs_ilog_fext(whichfork);
> + *logflagsp |= xfs_ilog_fext(whichfork);
> break;
> }
> error = xfs_bmbt_update(cur, &got);
> if (error)
> - goto done;
> + return error;
> break;
> case 0:
> /*
> @@ -5150,18 +5146,18 @@ xfs_bmap_del_extent_real(
> new.br_state = got.br_state;
> new.br_startblock = del_endblock;
>
> - flags |= XFS_ILOG_CORE;
> + *logflagsp |= XFS_ILOG_CORE;
> if (cur) {
> error = xfs_bmbt_update(cur, &got);
> if (error)
> - goto done;
> + return error;
> error = xfs_btree_increment(cur, 0, &i);
> if (error)
> - goto done;
> + return error;
> cur->bc_rec.b = new;
> error = xfs_btree_insert(cur, &i);
> if (error && error != -ENOSPC)
> - goto done;
> + return error;
> /*
> * If get no-space back from btree insert, it tried a
> * split, and we have a zero block reservation. Fix up
> @@ -5174,33 +5170,28 @@ xfs_bmap_del_extent_real(
> */
> error = xfs_bmbt_lookup_eq(cur, &got, &i);
> if (error)
> - goto done;
> - if (XFS_IS_CORRUPT(mp, i != 1)) {
> - error = -EFSCORRUPTED;
> - goto done;
> - }
> + return error;
> + if (XFS_IS_CORRUPT(mp, i != 1))
> + return -EFSCORRUPTED;
> /*
> * Update the btree record back
> * to the original value.
> */
> error = xfs_bmbt_update(cur, &old);
> if (error)
> - goto done;
> + return error;
> /*
> * Reset the extent record back
> * to the original value.
> */
> xfs_iext_update_extent(ip, state, icur, &old);
> - flags = 0;
> - error = -ENOSPC;
> - goto done;
> - }
> - if (XFS_IS_CORRUPT(mp, i != 1)) {
> - error = -EFSCORRUPTED;
> - goto done;
> + *logflagsp = 0;
> + return -ENOSPC;
> }
> + if (XFS_IS_CORRUPT(mp, i != 1))
> + return -EFSCORRUPTED;
> } else
> - flags |= xfs_ilog_fext(whichfork);
> + *logflagsp |= xfs_ilog_fext(whichfork);
>
> ifp->if_nextents++;
> xfs_iext_next(ifp, icur);
> @@ -5224,7 +5215,7 @@ xfs_bmap_del_extent_real(
> ((bflags & XFS_BMAPI_NODISCARD) ||
> del->br_state == XFS_EXT_UNWRITTEN));
> if (error)
> - goto done;
> + return error;
> }
> }
>
> @@ -5239,9 +5230,7 @@ xfs_bmap_del_extent_real(
> if (qfield && !(bflags & XFS_BMAPI_REMAP))
> xfs_trans_mod_dquot_byino(tp, ip, qfield, (long)-nblks);
>
> -done:
> - *logflagsp = flags;
> - return error;
> + return 0;
> }
>
> /*
> --
> 2.20.1
>
>