Message ID | 20231128053202.29007-2-zhangjiachen.jaycee@bytedance.com (mailing list archive) |
---|---|
State | Deferred, archived |
Headers | show |
Series | Fixes for ENOSPC xfs_remove | expand |
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.
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 --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)) {
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(-)