| Message ID | 20180614221646.20017-1-mbenatto@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On 6/14/18 5:16 PM, Marco Benatto wrote: > Currently when root inode is in short-form and its parent ino > has an invalid value, process_sf_dir2() ends up not fixing it, > because if verify_inum() fails we never get to the next case which > would fix the root inode's parent pointer. > > This behavior triggers the following assert on process_dir2(): > > ASSERT((ino != mp->m_sb.sb_rootino && ino != *parent) || > (ino == mp->m_sb.sb_rootino && > (ino == *parent || need_root_dotdot == 1))); > > This patch fixes this behavior by making sure we always properly > handle rootino parent pointer in process_sf_dir2() > > Signed-off-by: Marco Benatto <mbenatto@redhat.com> This looks correct to me, thanks. FWIW the problem can be demonstrated by setting the root inode's parent to 0 on a freshly made fs: # mkfs.xfs -f -dfile,name=fsfile,size=1g # xfs_db -x -c "sb 0" -c "addr rootino" -c "write u3.sfdir3.hdr.parent.i4 0" fsfile # xfs_repair fsfile Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > repair/dir2.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/repair/dir2.c b/repair/dir2.c > index e162d2b..225f926 100644 > --- a/repair/dir2.c > +++ b/repair/dir2.c > @@ -495,8 +495,10 @@ _("corrected entry offsets in directory %" PRIu64 "\n"), > > /* > * if parent entry is bogus, null it out. we'll fix it later . > + * If the validation fails for the root inode we fix it in > + * the next else case. > */ > - if (verify_inum(mp, *parent)) { > + if (verify_inum(mp, *parent) && ino != mp->m_sb.sb_rootino) { > > do_warn( > _("bogus .. inode number (%" PRIu64 ") in directory inode %" PRIu64 ", "), > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 14, 2018 at 05:20:56PM -0500, Eric Sandeen wrote: > On 6/14/18 5:16 PM, Marco Benatto wrote: > > Currently when root inode is in short-form and its parent ino > > has an invalid value, process_sf_dir2() ends up not fixing it, > > because if verify_inum() fails we never get to the next case which > > would fix the root inode's parent pointer. > > > > This behavior triggers the following assert on process_dir2(): > > > > ASSERT((ino != mp->m_sb.sb_rootino && ino != *parent) || > > (ino == mp->m_sb.sb_rootino && > > (ino == *parent || need_root_dotdot == 1))); > > > > This patch fixes this behavior by making sure we always properly > > handle rootino parent pointer in process_sf_dir2() > > > > Signed-off-by: Marco Benatto <mbenatto@redhat.com> > > This looks correct to me, thanks. > > FWIW the problem can be demonstrated by setting the root inode's > parent to 0 on a freshly made fs: > > # mkfs.xfs -f -dfile,name=fsfile,size=1g > # xfs_db -x -c "sb 0" -c "addr rootino" -c "write u3.sfdir3.hdr.parent.i4 0" fsfile > # xfs_repair fsfile xfstest please (also to check that xfs_scrub detects it)? Cheers, Dave.
On Thu, Jun 14, 2018 at 8:35 PM, Dave Chinner <david@fromorbit.com> wrote: > On Thu, Jun 14, 2018 at 05:20:56PM -0500, Eric Sandeen wrote: >> On 6/14/18 5:16 PM, Marco Benatto wrote: >> > Currently when root inode is in short-form and its parent ino >> > has an invalid value, process_sf_dir2() ends up not fixing it, >> > because if verify_inum() fails we never get to the next case which >> > would fix the root inode's parent pointer. >> > >> > This behavior triggers the following assert on process_dir2(): >> > >> > ASSERT((ino != mp->m_sb.sb_rootino && ino != *parent) || >> > (ino == mp->m_sb.sb_rootino && >> > (ino == *parent || need_root_dotdot == 1))); >> > >> > This patch fixes this behavior by making sure we always properly >> > handle rootino parent pointer in process_sf_dir2() >> > >> > Signed-off-by: Marco Benatto <mbenatto@redhat.com> >> >> This looks correct to me, thanks. >> >> FWIW the problem can be demonstrated by setting the root inode's >> parent to 0 on a freshly made fs: >> >> # mkfs.xfs -f -dfile,name=fsfile,size=1g >> # xfs_db -x -c "sb 0" -c "addr rootino" -c "write u3.sfdir3.hdr.parent.i4 0" fsfile >> # xfs_repair fsfile > > xfstest please (also to check that xfs_scrub detects it)? Sure, working on this. I'll send you once it's done. > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com Cheers,
FYI, just sent out "[PATCH] xfstests: Teste root inode parent pointer repairing" On Fri, Jun 15, 2018 at 2:30 PM, Marco Benatto <mbenatto@redhat.com> wrote: > On Thu, Jun 14, 2018 at 8:35 PM, Dave Chinner <david@fromorbit.com> wrote: >> On Thu, Jun 14, 2018 at 05:20:56PM -0500, Eric Sandeen wrote: >>> On 6/14/18 5:16 PM, Marco Benatto wrote: >>> > Currently when root inode is in short-form and its parent ino >>> > has an invalid value, process_sf_dir2() ends up not fixing it, >>> > because if verify_inum() fails we never get to the next case which >>> > would fix the root inode's parent pointer. >>> > >>> > This behavior triggers the following assert on process_dir2(): >>> > >>> > ASSERT((ino != mp->m_sb.sb_rootino && ino != *parent) || >>> > (ino == mp->m_sb.sb_rootino && >>> > (ino == *parent || need_root_dotdot == 1))); >>> > >>> > This patch fixes this behavior by making sure we always properly >>> > handle rootino parent pointer in process_sf_dir2() >>> > >>> > Signed-off-by: Marco Benatto <mbenatto@redhat.com> >>> >>> This looks correct to me, thanks. >>> >>> FWIW the problem can be demonstrated by setting the root inode's >>> parent to 0 on a freshly made fs: >>> >>> # mkfs.xfs -f -dfile,name=fsfile,size=1g >>> # xfs_db -x -c "sb 0" -c "addr rootino" -c "write u3.sfdir3.hdr.parent.i4 0" fsfile >>> # xfs_repair fsfile >> >> xfstest please (also to check that xfs_scrub detects it)? > > Sure, working on this. I'll send you once it's done. > >> >> Cheers, >> >> Dave. >> -- >> Dave Chinner >> david@fromorbit.com > > > > Cheers, > > -- > Marco Benatto
diff --git a/repair/dir2.c b/repair/dir2.c index e162d2b..225f926 100644 --- a/repair/dir2.c +++ b/repair/dir2.c @@ -495,8 +495,10 @@ _("corrected entry offsets in directory %" PRIu64 "\n"), /* * if parent entry is bogus, null it out. we'll fix it later . + * If the validation fails for the root inode we fix it in + * the next else case. */ - if (verify_inum(mp, *parent)) { + if (verify_inum(mp, *parent) && ino != mp->m_sb.sb_rootino) { do_warn( _("bogus .. inode number (%" PRIu64 ") in directory inode %" PRIu64 ", "),
Currently when root inode is in short-form and its parent ino has an invalid value, process_sf_dir2() ends up not fixing it, because if verify_inum() fails we never get to the next case which would fix the root inode's parent pointer. This behavior triggers the following assert on process_dir2(): ASSERT((ino != mp->m_sb.sb_rootino && ino != *parent) || (ino == mp->m_sb.sb_rootino && (ino == *parent || need_root_dotdot == 1))); This patch fixes this behavior by making sure we always properly handle rootino parent pointer in process_sf_dir2() Signed-off-by: Marco Benatto <mbenatto@redhat.com> --- repair/dir2.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)