diff mbox series

[1/2] xfs: ensure tmp_logflags is initialized in xfs_bmap_del_extent_real

Message ID 20231128053202.29007-2-zhangjiachen.jaycee@bytedance.com
State New
Headers
Series Fixes for ENOSPC xfs_remove |

Commit Message

Jiachen Zhang Nov. 28, 2023, 5:32 a.m. UTC 
  In the case of returning -ENOSPC, ensure tmp_logflags 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.

Fixes: 1b24b633aafe ("xfs: move some more code into xfs_bmap_del_extent_real")
Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>
---
 fs/xfs/libxfs/xfs_bmap.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Christoph Hellwig Nov. 28, 2023, 8:19 a.m. UTC | #1 
On Tue, Nov 28, 2023 at 01:32:01PM +0800, Jiachen Zhang wrote:
> In the case of returning -ENOSPC, ensure tmp_logflags 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.

This looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

But I wonder if removing the local flags variable and always directly
assigning to *logflagsp might be more robust in the long run.
Darrick J. Wong Nov. 28, 2023, 4:19 p.m. UTC | #2 
On Tue, Nov 28, 2023 at 12:19:23AM -0800, Christoph Hellwig wrote:
> On Tue, Nov 28, 2023 at 01:32:01PM +0800, Jiachen Zhang wrote:
> > In the case of returning -ENOSPC, ensure tmp_logflags 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.
> 
> This looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> But I wonder if removing the local flags variable and always directly
> assigning to *logflagsp might be more robust in the long run.

Yes, I think it's better to eliminate opportunities for subtle logic
bombs by not open-coding variable aliasing.  Perhaps this function
should set *logflagsp = 0 at the start of the function so that we don't
have to deal with uninitialized outparams, especially since the caller
uses it even on an error return.

--D
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index be62acffad6c..7cb395a38507 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5048,8 +5048,10 @@  xfs_bmap_del_extent_real(
 	if (tp->t_blk_res == 0 &&
 	    ifp->if_format == XFS_DINODE_FMT_EXTENTS &&
 	    ifp->if_nextents >= XFS_IFORK_MAXEXT(ip, whichfork) &&
-	    del->br_startoff > got.br_startoff && del_endoff < got_endoff)
-		return -ENOSPC;
+	    del->br_startoff > got.br_startoff && del_endoff < got_endoff) {
+		error = -ENOSPC;
+		goto done;
+	}
 
 	flags = XFS_ILOG_CORE;
 	if (whichfork == XFS_DATA_FORK && XFS_IS_REALTIME_INODE(ip)) {