* [PATCH] xfs_repair: dont leak buffer when discarding directories @ 2023-05-03 15:15 ` Darrick J. Wong 2023-05-10 14:00 ` Carlos Maiolino 0 siblings, 1 reply; 2+ messages in thread From: Darrick J. Wong @ 2023-05-03 15:15 UTC (permalink / raw) To: Carlos Maiolino; +Cc: xfs From: Darrick J. Wong <djwong@kernel.org> Commit 1f7c7553489c tried to reduce the memory requirements of phase 6 of repair by redesigning longform_dir2_entry_check without the bplist array. Unfortunately, none of us noticed that the code that rejects a dir block with a bad header now leaks the xfs_buf object because we no longer have a bplist to drop the buffer references. Any time we hold a buffer and decide to move on in the dabno loop, we must release the buffer. The immediate result of this error is that dir_binval complains about the recursive lock count of the buffer when we blow out the directory. However, if the block is reallocated by another thread, repair will deadlock when it tries to get the buffer and cannot take the buffer lock. Found via xfs/113 fuzzing data format directory blocks. For whatever reason this happens much more frequently when su=128k,sw=4, but this applies to everyone equally. While we're at it, make the relse at the bottom of the function run for any remaining buffer reference, even if this isn't a block format directory to avoid leaving a landmine in case we ever add a "goto fix" inside the loop for a non-block directory. Fixes: 1f7c7553489 ("repair: don't duplicate names in phase 6") Signed-off-by: Darrick J. Wong <djwong@kernel.org> --- repair/phase6.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/repair/phase6.c b/repair/phase6.c index 0be2c9c9705..48bf57359c5 100644 --- a/repair/phase6.c +++ b/repair/phase6.c @@ -2332,6 +2332,9 @@ longform_dir2_entry_check( fixit++; if (isblock) goto out_fix; + + libxfs_buf_relse(bp); + bp = NULL; continue; } } @@ -2343,6 +2346,7 @@ longform_dir2_entry_check( break; libxfs_buf_relse(bp); + bp = NULL; } fixit |= (*num_illegal != 0) || dir2_is_badino(ino) || *need_dot; @@ -2370,7 +2374,7 @@ longform_dir2_entry_check( } } out_fix: - if (isblock && bp) + if (bp) libxfs_buf_relse(bp); if (!no_modify && (fixit || dotdot_update)) { ^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] xfs_repair: dont leak buffer when discarding directories 2023-05-03 15:15 ` [PATCH] xfs_repair: dont leak buffer when discarding directories Darrick J. Wong @ 2023-05-10 14:00 ` Carlos Maiolino 0 siblings, 0 replies; 2+ messages in thread From: Carlos Maiolino @ 2023-05-10 14:00 UTC (permalink / raw) To: Darrick J. Wong; +Cc: xfs On Wed, May 03, 2023 at 08:15:15AM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Commit 1f7c7553489c tried to reduce the memory requirements of phase 6 > of repair by redesigning longform_dir2_entry_check without the bplist > array. Unfortunately, none of us noticed that the code that rejects a > dir block with a bad header now leaks the xfs_buf object because we no > longer have a bplist to drop the buffer references. Any time we hold a > buffer and decide to move on in the dabno loop, we must release the > buffer. > > The immediate result of this error is that dir_binval complains about > the recursive lock count of the buffer when we blow out the directory. > However, if the block is reallocated by another thread, repair will > deadlock when it tries to get the buffer and cannot take the buffer > lock. > > Found via xfs/113 fuzzing data format directory blocks. For whatever > reason this happens much more frequently when su=128k,sw=4, but this > applies to everyone equally. > > While we're at it, make the relse at the bottom of the function run for > any remaining buffer reference, even if this isn't a block format > directory to avoid leaving a landmine in case we ever add a "goto > fix" inside the loop for a non-block directory. > > Fixes: 1f7c7553489 ("repair: don't duplicate names in phase 6") > Signed-off-by: Darrick J. Wong <djwong@kernel.org> Looks good. Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > repair/phase6.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/repair/phase6.c b/repair/phase6.c > index 0be2c9c9705..48bf57359c5 100644 > --- a/repair/phase6.c > +++ b/repair/phase6.c > @@ -2332,6 +2332,9 @@ longform_dir2_entry_check( > fixit++; > if (isblock) > goto out_fix; > + > + libxfs_buf_relse(bp); > + bp = NULL; > continue; > } > } > @@ -2343,6 +2346,7 @@ longform_dir2_entry_check( > break; > > libxfs_buf_relse(bp); > + bp = NULL; > } > fixit |= (*num_illegal != 0) || dir2_is_badino(ino) || *need_dot; > > @@ -2370,7 +2374,7 @@ longform_dir2_entry_check( > } > } > out_fix: > - if (isblock && bp) > + if (bp) > libxfs_buf_relse(bp); > > if (!no_modify && (fixit || dotdot_update)) { -- Carlos Maiolino ^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-05-10 14:00 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <hisABZGtTerz9q4LHi-k52Q9qnsVnsnnpL0ZyXEQleLbIaF5zCFcA_URJ65VWBQpaCD_1oSQW9iBbmheoPZ8TA==@protonmail.internalid>
2023-05-03 15:15 ` [PATCH] xfs_repair: dont leak buffer when discarding directories Darrick J. Wong
2023-05-10 14:00 ` Carlos Maiolino
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).