| Message ID | f5b8a2a9-e691-3bf5-c2c7-f4986a933454@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs_repair: fix bad next_unlinked field | expand |
On Mon, Feb 10, 2020 at 09:42:28AM -0600, Eric Sandeen wrote: > As of xfsprogs-4.17 we started testing whether the di_next_unlinked field > on an inode is valid in the inode verifiers. However, this field is never > tested or repaired during inode processing. > > So if, for example, we had a completely zeroed-out inode, we'd detect and > fix the broken magic and version, but the invalid di_next_unlinked field > would not be touched, fail the write verifier, and prevent the inode from > being properly repaired or even written out. > > Fix this by checking the di_next_unlinked inode field for validity and > clearing it if it is invalid. > > Reported-by: John Jore <john@jore.no> > Fixes: 2949b4677 ("xfs: don't accept inode buffers with suspicious unlinked chains") > Signed-off-by: Eric Sandeen <sandeen@redhat.com> Seems reasonable, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > > diff --git a/repair/dinode.c b/repair/dinode.c > index 8af2cb25..c5d2f350 100644 > --- a/repair/dinode.c > +++ b/repair/dinode.c > @@ -2272,6 +2272,7 @@ process_dinode_int(xfs_mount_t *mp, > const int is_free = 0; > const int is_used = 1; > blkmap_t *dblkmap = NULL; > + xfs_agino_t unlinked_ino; > > *dirty = *isa_dir = 0; > *used = is_used; > @@ -2351,6 +2352,23 @@ process_dinode_int(xfs_mount_t *mp, > } > } > > + unlinked_ino = be32_to_cpu(dino->di_next_unlinked); > + if (!xfs_verify_agino_or_null(mp, agno, unlinked_ino)) { > + retval = 1; > + if (!uncertain) > + do_warn(_("bad next_unlinked 0x%x on inode %" PRIu64 "%c"), > + (__s32)dino->di_next_unlinked, lino, > + verify_mode ? '\n' : ','); > + if (!verify_mode) { > + if (!no_modify) { > + do_warn(_(" resetting next_unlinked\n")); > + clear_dinode_unlinked(mp, dino); > + *dirty = 1; > + } else > + do_warn(_(" would reset next_unlinked\n")); > + } > + } > + > /* > * We don't bother checking the CRC here - we cannot guarantee that when > * we are called here that the inode has not already been modified in >
> + unlinked_ino = be32_to_cpu(dino->di_next_unlinked); > + if (!xfs_verify_agino_or_null(mp, agno, unlinked_ino)) { > + retval = 1; > + if (!uncertain) > + do_warn(_("bad next_unlinked 0x%x on inode %" PRIu64 "%c"), > + (__s32)dino->di_next_unlinked, lino, ^^^^ shouldn't we be using be32_to_cpu() here, instead of a direct casting to __s32? Cheers.
Hi and thanks for this one. Ran it twice. No errors were found on the second run. Let me know if you need a dump or anything for validation purposes? John --- From: Eric Sandeen <sandeen@redhat.com> Sent: 11 February 2020 02:42 To: linux-xfs Cc: John Jore Subject: [PATCH] xfs_repair: fix bad next_unlinked field As of xfsprogs-4.17 we started testing whether the di_next_unlinked field on an inode is valid in the inode verifiers. However, this field is never tested or repaired during inode processing. So if, for example, we had a completely zeroed-out inode, we'd detect and fix the broken magic and version, but the invalid di_next_unlinked field would not be touched, fail the write verifier, and prevent the inode from being properly repaired or even written out. Fix this by checking the di_next_unlinked inode field for validity and clearing it if it is invalid. Reported-by: John Jore <john@jore.no> Fixes: 2949b4677 ("xfs: don't accept inode buffers with suspicious unlinked chains") Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- diff --git a/repair/dinode.c b/repair/dinode.c index 8af2cb25..c5d2f350 100644 --- a/repair/dinode.c +++ b/repair/dinode.c @@ -2272,6 +2272,7 @@ process_dinode_int(xfs_mount_t *mp, const int is_free = 0; const int is_used = 1; blkmap_t *dblkmap = NULL; + xfs_agino_t unlinked_ino; *dirty = *isa_dir = 0; *used = is_used; @@ -2351,6 +2352,23 @@ process_dinode_int(xfs_mount_t *mp, } } + unlinked_ino = be32_to_cpu(dino->di_next_unlinked); + if (!xfs_verify_agino_or_null(mp, agno, unlinked_ino)) { + retval = 1; + if (!uncertain) + do_warn(_("bad next_unlinked 0x%x on inode %" PRIu64 "%c"), + (__s32)dino->di_next_unlinked, lino, + verify_mode ? '\n' : ','); + if (!verify_mode) { + if (!no_modify) { + do_warn(_(" resetting next_unlinked\n")); + clear_dinode_unlinked(mp, dino); + *dirty = 1; + } else + do_warn(_(" would reset next_unlinked\n")); + } + } + /* * We don't bother checking the CRC here - we cannot guarantee that when * we are called here that the inode has not already been modified in
On 2/11/20 4:11 AM, John Jore wrote: > Hi and thanks for this one. > > Ran it twice. No errors were found on the second run. > > Let me know if you need a dump or anything for validation purposes? Nah it's all good, thanks for the report. -Eric > > John > > --- > From: Eric Sandeen <sandeen@redhat.com> > Sent: 11 February 2020 02:42 > To: linux-xfs > Cc: John Jore > Subject: [PATCH] xfs_repair: fix bad next_unlinked field > > As of xfsprogs-4.17 we started testing whether the di_next_unlinked field > on an inode is valid in the inode verifiers. However, this field is never > tested or repaired during inode processing. > > So if, for example, we had a completely zeroed-out inode, we'd detect and > fix the broken magic and version, but the invalid di_next_unlinked field > would not be touched, fail the write verifier, and prevent the inode from > being properly repaired or even written out. > > Fix this by checking the di_next_unlinked inode field for validity and > clearing it if it is invalid. > > Reported-by: John Jore <john@jore.no> > Fixes: 2949b4677 ("xfs: don't accept inode buffers with suspicious unlinked chains") > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > diff --git a/repair/dinode.c b/repair/dinode.c > index 8af2cb25..c5d2f350 100644 > --- a/repair/dinode.c > +++ b/repair/dinode.c > @@ -2272,6 +2272,7 @@ process_dinode_int(xfs_mount_t *mp, > const int is_free = 0; > const int is_used = 1; > blkmap_t *dblkmap = NULL; > + xfs_agino_t unlinked_ino; > > *dirty = *isa_dir = 0; > *used = is_used; > @@ -2351,6 +2352,23 @@ process_dinode_int(xfs_mount_t *mp, > } > } > > + unlinked_ino = be32_to_cpu(dino->di_next_unlinked); > + if (!xfs_verify_agino_or_null(mp, agno, unlinked_ino)) { > + retval = 1; > + if (!uncertain) > + do_warn(_("bad next_unlinked 0x%x on inode %" PRIu64 "%c"), > + (__s32)dino->di_next_unlinked, lino, > + verify_mode ? '\n' : ','); > + if (!verify_mode) { > + if (!no_modify) { > + do_warn(_(" resetting next_unlinked\n")); > + clear_dinode_unlinked(mp, dino); > + *dirty = 1; > + } else > + do_warn(_(" would reset next_unlinked\n")); > + } > + } > + > /* > * We don't bother checking the CRC here - we cannot guarantee that when > * we are called here that the inode has not already been modified in > > >
On 2/11/20 3:08 AM, Carlos Maiolino wrote: >> + unlinked_ino = be32_to_cpu(dino->di_next_unlinked); >> + if (!xfs_verify_agino_or_null(mp, agno, unlinked_ino)) { >> + retval = 1; >> + if (!uncertain) >> + do_warn(_("bad next_unlinked 0x%x on inode %" PRIu64 "%c"), >> + (__s32)dino->di_next_unlinked, lino, > ^^^^ > shouldn't we be using be32_to_cpu() > here, instead of a direct casting to > __s32? Yes, good catch. I was looking at the version check which just does (__s8) but of course that doesn't need the conversion. I'll fix it here, thanks! -Eric
diff --git a/repair/dinode.c b/repair/dinode.c index 8af2cb25..c5d2f350 100644 --- a/repair/dinode.c +++ b/repair/dinode.c @@ -2272,6 +2272,7 @@ process_dinode_int(xfs_mount_t *mp, const int is_free = 0; const int is_used = 1; blkmap_t *dblkmap = NULL; + xfs_agino_t unlinked_ino; *dirty = *isa_dir = 0; *used = is_used; @@ -2351,6 +2352,23 @@ process_dinode_int(xfs_mount_t *mp, } } + unlinked_ino = be32_to_cpu(dino->di_next_unlinked); + if (!xfs_verify_agino_or_null(mp, agno, unlinked_ino)) { + retval = 1; + if (!uncertain) + do_warn(_("bad next_unlinked 0x%x on inode %" PRIu64 "%c"), + (__s32)dino->di_next_unlinked, lino, + verify_mode ? '\n' : ','); + if (!verify_mode) { + if (!no_modify) { + do_warn(_(" resetting next_unlinked\n")); + clear_dinode_unlinked(mp, dino); + *dirty = 1; + } else + do_warn(_(" would reset next_unlinked\n")); + } + } + /* * We don't bother checking the CRC here - we cannot guarantee that when * we are called here that the inode has not already been modified in
As of xfsprogs-4.17 we started testing whether the di_next_unlinked field on an inode is valid in the inode verifiers. However, this field is never tested or repaired during inode processing. So if, for example, we had a completely zeroed-out inode, we'd detect and fix the broken magic and version, but the invalid di_next_unlinked field would not be touched, fail the write verifier, and prevent the inode from being properly repaired or even written out. Fix this by checking the di_next_unlinked inode field for validity and clearing it if it is invalid. Reported-by: John Jore <john@jore.no> Fixes: 2949b4677 ("xfs: don't accept inode buffers with suspicious unlinked chains") Signed-off-by: Eric Sandeen <sandeen@redhat.com> ---