[PATCH 0/5] xfs_repair fixes, part 1

[Eric Sandeen]

I've been running a modified fsfuzzer and looking at the results
of xfs_repair of the corrupt images; plenty of things pop out.

I'm calling this "part one" because I expect that I'll find
more, but will send things out in digestable chunks as I
accumulate them...

Most of these are obviously correct and trivial; a coupl
require more thought & review.

Thanks,
-Eric

[Eric Sandeen]

process_shortform_attr uses the "junkit" error to
track whether an error was found, but by assigning
it directly to the result of valuecheck, previous
errors are ignored, leading to unrepairable errors
of the form i.e.

"entry has INCOMPLETE flag on in shortform attribute"
or
"entry contains illegal character in shortform attribute name"

Signed-off-by: Eric Sandeen <***@redhat.com>
---
repair/attr_repair.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index a27a3ec..d60b664 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -914,7 +914,8 @@ process_shortform_attr(

/* Only check values for root security attributes */
if (currententry->flags & XFS_ATTR_ROOT)
- junkit = valuecheck(mp, (char *)&currententry->nameval[0],
+ junkit |= valuecheck(mp,
+ (char *)&currententry->nameval[0],
NULL, currententry->namelen,
currententry->valuelen);

[Brian Foster]

Post by Eric Sandeen
process_shortform_attr uses the "junkit" error to
track whether an error was found, but by assigning
it directly to the result of valuecheck, previous
errors are ignored, leading to unrepairable errors
of the form i.e.
"entry has INCOMPLETE flag on in shortform attribute"
or
"entry contains illegal character in shortform attribute name"
---
repair/attr_repair.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index a27a3ec..d60b664 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -914,7 +914,8 @@ process_shortform_attr(
/* Only check values for root security attributes */
if (currententry->flags & XFS_ATTR_ROOT)
- junkit = valuecheck(mp, (char *)&currententry->nameval[0],
+ junkit |= valuecheck(mp,
+ (char *)&currententry->nameval[0],
NULL, currententry->namelen,
currententry->valuelen);
--
1.7.1
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[Christoph Hellwig]

Post by Eric Sandeen
process_shortform_attr uses the "junkit" error to
track whether an error was found, but by assigning
it directly to the result of valuecheck, previous
errors are ignored, leading to unrepairable errors
of the form i.e.
"entry has INCOMPLETE flag on in shortform attribute"
or
"entry contains illegal character in shortform attribute name"

Looks good,

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

[Eric Sandeen]

xfs_dir3_dirent_get_ftype() gets the file type
off disk, but ASSERTs if it's invalid:

ASSERT(type < XFS_DIR3_FT_MAX);

This might be cut & paste from
xfs_dir3_dirent_put_ftype which should be checking
that it's not been passed bad values, but we
shouldn't ASSERT on bad values read from disk.

Signed-off-by: Eric Sandeen <***@redhat.com>
---
include/xfs_da_format.h | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/include/xfs_da_format.h b/include/xfs_da_format.h
index 89a1a21..11f1420 100644
--- a/include/xfs_da_format.h
+++ b/include/xfs_da_format.h
@@ -561,7 +561,6 @@ xfs_dir3_dirent_get_ftype(
if (xfs_sb_version_hasftype(&mp->m_sb)) {
__uint8_t type = dep->name[dep->namelen];

- ASSERT(type < XFS_DIR3_FT_MAX);
if (type < XFS_DIR3_FT_MAX)
return type;

[Dave Chinner]

Post by Eric Sandeen
xfs_dir3_dirent_get_ftype() gets the file type
ASSERT(type < XFS_DIR3_FT_MAX);
This might be cut & paste from
xfs_dir3_dirent_put_ftype which should be checking
that it's not been passed bad values, but we
shouldn't ASSERT on bad values read from disk.
---
include/xfs_da_format.h | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/include/xfs_da_format.h b/include/xfs_da_format.h
index 89a1a21..11f1420 100644
--- a/include/xfs_da_format.h
+++ b/include/xfs_da_format.h
@@ -561,7 +561,6 @@ xfs_dir3_dirent_get_ftype(
if (xfs_sb_version_hasftype(&mp->m_sb)) {
__uint8_t type = dep->name[dep->namelen];
- ASSERT(type < XFS_DIR3_FT_MAX);
if (type < XFS_DIR3_FT_MAX)
return type;

Needs to be fixed kernel-side first.

Cheers,

Dave.

[Eric Sandeen]

...

Post by Dave Chinner

Post by Eric Sandeen
diff --git a/include/xfs_da_format.h b/include/xfs_da_format.h
index 89a1a21..11f1420 100644
--- a/include/xfs_da_format.h
+++ b/include/xfs_da_format.h
@@ -561,7 +561,6 @@ xfs_dir3_dirent_get_ftype(
if (xfs_sb_version_hasftype(&mp->m_sb)) {
__uint8_t type = dep->name[dep->namelen];
- ASSERT(type < XFS_DIR3_FT_MAX);
if (type < XFS_DIR3_FT_MAX)
return type;

Needs to be fixed kernel-side first.

xfs_dir3_dirent_get_ftype doesn't exist in kernelspace :)

bleah, why do they have different names... Ok, will send.

-Eric

Post by Dave Chinner
Cheers,
Dave.

[Dave Chinner]

Post by Eric Sandeen
...

Post by Dave Chinner

Post by Eric Sandeen
diff --git a/include/xfs_da_format.h b/include/xfs_da_format.h
index 89a1a21..11f1420 100644
--- a/include/xfs_da_format.h
+++ b/include/xfs_da_format.h
@@ -561,7 +561,6 @@ xfs_dir3_dirent_get_ftype(
if (xfs_sb_version_hasftype(&mp->m_sb)) {
__uint8_t type = dep->name[dep->namelen];
- ASSERT(type < XFS_DIR3_FT_MAX);
if (type < XFS_DIR3_FT_MAX)
return type;

Needs to be fixed kernel-side first.

xfs_dir3_dirent_get_ftype doesn't exist in kernelspace :)
bleah, why do they have different names... Ok, will send.

Because kernel has changed, and we need to do yet another large
kernel/user libxfs sync.

I haven't found time to do that yet. Unless you want to volunteer
for it....

Cheers,

Dave.

[Eric Sandeen]

...

Post by Dave Chinner

Post by Eric Sandeen

Post by Dave Chinner
Needs to be fixed kernel-side first.

xfs_dir3_dirent_get_ftype doesn't exist in kernelspace :)
bleah, why do they have different names... Ok, will send.

Because kernel has changed, and we need to do yet another large
kernel/user libxfs sync.
I haven't found time to do that yet. Unless you want to volunteer
for it....

I could give it a shot. Do you usually do it commit-by-commit,
diff-and-edit, or a big copy?

-Eric

[Dave Chinner]

Post by Eric Sandeen
...

Post by Dave Chinner

Post by Eric Sandeen

Post by Dave Chinner
Needs to be fixed kernel-side first.

xfs_dir3_dirent_get_ftype doesn't exist in kernelspace :)
bleah, why do they have different names... Ok, will send.

Because kernel has changed, and we need to do yet another large
kernel/user libxfs sync.
I haven't found time to do that yet. Unless you want to volunteer
for it....

I could give it a shot. Do you usually do it commit-by-commit,
diff-and-edit, or a big copy?

This one is going to need multiple phases, and in a moment you're
going to understand what all functions called into libxfs should
have a "libxfs define wrapper":

1. sync up to kernel commit before error negation
2. make sure all external function calls have libxfs
define wrappers
3. commit error negation patch and change sign of all libxfs
define wrappers
4. apply kernel libxfs rename patch and restructure
libxfs userspace and xfsprogs build to match
5. continue syncing kernel changes commit by commit.

Given the scope of changes, and the fact that some of the changes
have already been applied (i.e. bug fixes, finobt changes, etc)
step #1 is not going to be a straight forward commit-by-commit.
So I'm happy to do that as a bulk commit. Steps #2 and #3 are
relatively straight forward, too, just a bit painful as we need
to be sure we don't leak negative errors.

Step #4 is the big one; we want to end up with the kernel
fs/xfs/libxfs to be identical to the xfsprogs:/libxfs/ code and so
in userspace we've got to move headers around and rejig includes and
things like that. That's one I should probably do.

And from there, step 5 should be a simple extract/sed/commit process
of pulling patch by patch from the kernel changes that touch
fs/xfs/libxfs....

So, really, the big step right now is #1...

Cheers,

Dave.

[Eric Sandeen]

This post might be inappropriate. Click to display it.

[Eric Sandeen]

Post by Eric Sandeen
When we move files to lost+found, we're setting the
filetype to UNKNOWN. This leaves an inconsistency which
would fix ftype mismatch (0/1) in directory/child inode 5838/5839
Setting the proper ftype at the time of the move

Ah, arekm points out that Jan already sent a patch to do this:

[PATCH] repair: Set ftype for entries in lost+found

http://oss.sgi.com/archives/xfs/2014-07/msg00314.html

Sorry, missed that.

-Eric

[Eric Sandeen]

In phase 6's longform_dir2_entry_check, if we never
find a '.' entry we never add a reference to that entry;
if we subsequently rebuild it, '.' gets added, but
no ref to it is ever made. This leads to Phase 7 doing
i.e.:

Phase 7 - verify and correct link counts...
resetting inode 5184 nlinks from 2 to 1

and the next run will do:

Phase 7 - verify and correct link counts...
resetting inode 5184 nlinks from 1 to 2

So if '.' was never found, but the directory got
rebuilt, manually add the ref for it.

Signed-off-by: Eric Sandeen <***@redhat.com>
---
repair/phase6.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/repair/phase6.c b/repair/phase6.c
index f13069f..cc36a9c 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -2288,6 +2288,12 @@ out_fix:
if (bplist[i])
libxfs_putbuf(bplist[i]);
longform_dir2_rebuild(mp, ino, ip, irec, ino_offset, hashtab);
+ /*
+ * If we didn't find a dot, we never added a ref for it;
+ * it's there now after the rebuild, so mark it as reached.
+ */
+ if (*need_dot)
+ add_inode_ref(irec, ino_offset);
*num_illegal = 0;
*need_dot = 0;
} else {

[Brian Foster]

Post by Eric Sandeen
In phase 6's longform_dir2_entry_check, if we never
find a '.' entry we never add a reference to that entry;
if we subsequently rebuild it, '.' gets added, but
no ref to it is ever made. This leads to Phase 7 doing
Phase 7 - verify and correct link counts...
resetting inode 5184 nlinks from 2 to 1
Phase 7 - verify and correct link counts...
resetting inode 5184 nlinks from 1 to 2
So if '.' was never found, but the directory got
rebuilt, manually add the ref for it.
---
repair/phase6.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/repair/phase6.c b/repair/phase6.c
index f13069f..cc36a9c 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
if (bplist[i])
libxfs_putbuf(bplist[i]);
longform_dir2_rebuild(mp, ino, ip, irec, ino_offset, hashtab);
+ /*
+ * If we didn't find a dot, we never added a ref for it;
+ * it's there now after the rebuild, so mark it as reached.
+ */
+ if (*need_dot)
+ add_inode_ref(irec, ino_offset);

So if I follow this correctly, we iterate through the dir, add each name
to the hashtable and handle the inode reference count in the first
longform_dir2_entry_check() loop. If something is wrong, we call
longform_dir2_rebuild() to rebuild the dir from the hashtable of
names/inodes. We may or may not have added a reference for dot at that
point, and need_dot is set appropriately.

This seems Ok, but where is the dot entry actually added? Hmm, I see
that we handle dot in the longform_dir2_rebuild() loop by just skipping
over it...

Brian

Post by Eric Sandeen
*num_illegal = 0;
*need_dot = 0;
} else {
--
1.7.1
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[Brian Foster]

Post by Brian Foster

Post by Eric Sandeen
In phase 6's longform_dir2_entry_check, if we never
find a '.' entry we never add a reference to that entry;
if we subsequently rebuild it, '.' gets added, but
no ref to it is ever made. This leads to Phase 7 doing
Phase 7 - verify and correct link counts...
resetting inode 5184 nlinks from 2 to 1
Phase 7 - verify and correct link counts...
resetting inode 5184 nlinks from 1 to 2
So if '.' was never found, but the directory got
rebuilt, manually add the ref for it.
---
repair/phase6.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/repair/phase6.c b/repair/phase6.c
index f13069f..cc36a9c 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
if (bplist[i])
libxfs_putbuf(bplist[i]);
longform_dir2_rebuild(mp, ino, ip, irec, ino_offset, hashtab);
+ /*
+ * If we didn't find a dot, we never added a ref for it;
+ * it's there now after the rebuild, so mark it as reached.
+ */
+ if (*need_dot)
+ add_inode_ref(irec, ino_offset);

So if I follow this correctly, we iterate through the dir, add each name
to the hashtable and handle the inode reference count in the first
longform_dir2_entry_check() loop. If something is wrong, we call
longform_dir2_rebuild() to rebuild the dir from the hashtable of
names/inodes. We may or may not have added a reference for dot at that
point, and need_dot is set appropriately.
This seems Ok, but where is the dot entry actually added? Hmm, I see
that we handle dot in the longform_dir2_rebuild() loop by just skipping
over it...

It looks like this happens in process_dir_inode() after this whole
check/rebuild sequence, directory format permitting. There's also an
add_inode_ref() there. Perhaps the bug here is that we clear need_dot
when we shouldn't..?

Brian

Post by Brian Foster
Brian

Post by Eric Sandeen
*num_illegal = 0;
*need_dot = 0;
} else {
--
1.7.1
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[Eric Sandeen]

Post by Brian Foster

Post by Brian Foster

Post by Eric Sandeen
In phase 6's longform_dir2_entry_check, if we never
find a '.' entry we never add a reference to that entry;
if we subsequently rebuild it, '.' gets added, but
no ref to it is ever made. This leads to Phase 7 doing
Phase 7 - verify and correct link counts...
resetting inode 5184 nlinks from 2 to 1
Phase 7 - verify and correct link counts...
resetting inode 5184 nlinks from 1 to 2
So if '.' was never found, but the directory got
rebuilt, manually add the ref for it.
---
repair/phase6.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/repair/phase6.c b/repair/phase6.c
index f13069f..cc36a9c 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
if (bplist[i])
libxfs_putbuf(bplist[i]);
longform_dir2_rebuild(mp, ino, ip, irec, ino_offset, hashtab);
+ /*
+ * If we didn't find a dot, we never added a ref for it;
+ * it's there now after the rebuild, so mark it as reached.
+ */
+ if (*need_dot)
+ add_inode_ref(irec, ino_offset);

So if I follow this correctly, we iterate through the dir, add each name
to the hashtable and handle the inode reference count in the first
longform_dir2_entry_check() loop. If something is wrong, we call
longform_dir2_rebuild() to rebuild the dir from the hashtable of
names/inodes. We may or may not have added a reference for dot at that
point, and need_dot is set appropriately.
This seems Ok, but where is the dot entry actually added? Hmm, I see
that we handle dot in the longform_dir2_rebuild() loop by just skipping
over it...

It looks like this happens in process_dir_inode() after this whole
check/rebuild sequence, directory format permitting. There's also an
add_inode_ref() there. Perhaps the bug here is that we clear need_dot
when we shouldn't..?

If we do that, the first run says:

bad hash table for directory inode 5184 (no data entry): rebuilding
rebuilding directory inode 5184
creating missing "." entry in dir ino 5184

and then the 2nd run says:

multiple . entries in directory inode 5184: clearing entry

so, no. ;)

The issue is that add_inode_ref() is keeping track (in repair)
of reached paths to the inode, in counted_nlinks.

If we didn't find '.' originally, we didn't add that ref.

When we do:


longform_dir2_rebuild
xfs_dir_init() // creates shortform
<loop over names>
xfs_dir_createname
xfs_dir2_sf_to_block when it's big enough
add '.' entry

and then we've added the '.' but haven't added the reference repair needs
internally.

-Eric

[Eric Sandeen]

Post by Brian Foster

Post by Eric Sandeen
In phase 6's longform_dir2_entry_check, if we never
find a '.' entry we never add a reference to that entry;
if we subsequently rebuild it, '.' gets added, but
no ref to it is ever made. This leads to Phase 7 doing
Phase 7 - verify and correct link counts...
resetting inode 5184 nlinks from 2 to 1
Phase 7 - verify and correct link counts...
resetting inode 5184 nlinks from 1 to 2
So if '.' was never found, but the directory got
rebuilt, manually add the ref for it.
---
repair/phase6.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/repair/phase6.c b/repair/phase6.c
index f13069f..cc36a9c 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
if (bplist[i])
libxfs_putbuf(bplist[i]);
longform_dir2_rebuild(mp, ino, ip, irec, ino_offset, hashtab);
+ /*
+ * If we didn't find a dot, we never added a ref for it;
+ * it's there now after the rebuild, so mark it as reached.
+ */
+ if (*need_dot)
+ add_inode_ref(irec, ino_offset);

So if I follow this correctly, we iterate through the dir, add each name
to the hashtable and handle the inode reference count in the first
longform_dir2_entry_check() loop. If something is wrong, we call
longform_dir2_rebuild() to rebuild the dir from the hashtable of
names/inodes. We may or may not have added a reference for dot at that
point, and need_dot is set appropriately.
This seems Ok, but where is the dot entry actually added? Hmm, I see
that we handle dot in the longform_dir2_rebuild() loop by just skipping
over it...

longform_dir2_rebuild calls this before the loop:

/*
* Initialize a directory with its "." and ".." entries.
*/
int
xfs_dir_init()

But it doesn't actually create .; this creates a shortform dir,
and shortform dirs have no '.' - I guess the comment could use
an update.

In the loop we call xfs_dir_createname() for everything in the hash,
eventually things don't fit in shortform and we do xfs_dir2_sf_to_block,
which creates the dot:

/*
* Create entry for .
*/
dep = xfs_dir3_data_dot_entry_p(mp, hdr);
dep->inumber = cpu_to_be64(dp->i_ino);
dep->namelen = 1;
dep->name[0] = '.';

-Eric

[Eric Sandeen]

This post might be inappropriate. Click to display it.

[Brian Foster]

Post by Eric Sandeen
process_dinode_int() reports bad flags if
dino->di_flags & ~XFS_DIFLAG_ANY - i.e. if
any flags are set outside the known set. But
then instead of clearing them, it does
flags &= ~XFS_DIFLAG_ANY which keeps *only*
the bad flags. This leads to persistent,
"Bad flags set in inode XXX"
Fix this.
While we are at it, fix a couple lines which
look like they used to be continuation lines,
but are no longer.
---
repair/dinode.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/repair/dinode.c b/repair/dinode.c
index 8891e84..38a6562 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2456,7 +2456,7 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
_("Bad flags set in inode %" PRIu64 "\n"),
lino);
}
- flags &= ~XFS_DIFLAG_ANY;
+ flags &= XFS_DIFLAG_ANY;
}
if (flags & (XFS_DIFLAG_REALTIME | XFS_DIFLAG_RTINHERIT)) {
@@ -2513,11 +2513,11 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
}
if (!verify_mode && flags != be16_to_cpu(dino->di_flags)) {
if (!no_modify) {
- do_warn(_(", fixing bad flags.\n"));
+ do_warn(_("fixing bad flags.\n"));
dino->di_flags = cpu_to_be16(flags);
*dirty = 1;
} else
- do_warn(_(", would fix bad flags.\n"));
+ do_warn(_("would fix bad flags.\n"));
}
}
--
1.7.1
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[Christoph Hellwig]

Post by Eric Sandeen
process_dinode_int() reports bad flags if
dino->di_flags & ~XFS_DIFLAG_ANY - i.e. if
any flags are set outside the known set. But
then instead of clearing them, it does
flags &= ~XFS_DIFLAG_ANY which keeps *only*
the bad flags. This leads to persistent,

You know you can use up to 75 characters per line for your commit messages,
don't you? :)

Otherwise looks good,

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

[Eric Sandeen]

Post by Christoph Hellwig

Post by Eric Sandeen
process_dinode_int() reports bad flags if
dino->di_flags & ~XFS_DIFLAG_ANY - i.e. if
any flags are set outside the known set. But
then instead of clearing them, it does
flags &= ~XFS_DIFLAG_ANY which keeps *only*
the bad flags. This leads to persistent,

You know you can use up to 75 characters per line for your commit messages,
don't you? :)

hah, it's not automated at all, I guess my visual perception
of the window is shrinking. Dave, feel free to fix on commit if
inclined :)

-Eric

[Dave Chinner]

Post by Eric Sandeen

Post by Christoph Hellwig

Post by Eric Sandeen
process_dinode_int() reports bad flags if
dino->di_flags & ~XFS_DIFLAG_ANY - i.e. if
any flags are set outside the known set. But
then instead of clearing them, it does
flags &= ~XFS_DIFLAG_ANY which keeps *only*
the bad flags. This leads to persistent,

You know you can use up to 75 characters per line for your commit messages,
don't you? :)

hah, it's not automated at all, I guess my visual perception
of the window is shrinking. Dave, feel free to fix on commit if
inclined :)

I mostly do already - I tend to reflow commit messages to 68
characters (same width I use for email) when I see something like
this.

Cheers,

Dave.

[Eric Sandeen]

If we've rebuilt the root directory, ".." was taken
care of, so clear need_root_dotdot.

Otherwise it will be added twice, and a subsequent repair
will say:

entry ".." (ino 5824) in dir 5824 is a duplicate name, would junk entry

Signed-off-by: Eric Sandeen <***@redhat.com>
---

(sorry for 6/5, this just popped out and is similar to the
patch 3/5 I just sent, so probably worth doing at the same time.

diff --git a/repair/phase6.c b/repair/phase6.c
index cc36a9c..2e67c60 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -2294,6 +2297,9 @@ out_fix:
*/
if (*need_dot)
add_inode_ref(irec, ino_offset);
+ /* If we rebuilt the root dir, dot dot is in good shape */
+ if (ino == mp->m_sb.sb_rootino)
+ need_root_dotdot = 0;
*num_illegal = 0;
*need_dot = 0;
} else {