Re: [PATCH 09/11] xfs_repair: don't assert if we run across a dir entry with null ino ptr

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, May 04, 2018 at 03:45:18PM -0700, Darrick J. Wong wrote:
> On Fri, May 04, 2018 at 05:33:35PM -0500, Eric Sandeen wrote:
> > 
> > 
> > On 4/17/18 9:47 PM, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > If we encounter a directory with an entry that points to inode zero,
> > > we'll crash due to an ASSERT during process_inode_chunk.  Instead, just
> > > set the in-core parent to NULLFSINO so that phase 6 will reset it for
> > > us.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > So .... this is .... probably ok?  but the ASSERT makes me think that
> > process_dinode is never supposed to return a 0 parent if isa_dir is set,
> > and so I'm a little worried about breaking that assumption up and just
> > catching it later, here.  Usually the asserts mean "the code should never
> > let this happen and if it does, some prior code is wrong"
> > 
> > Looking at the calls below it, it seems like we generally call verify_inum
> > which would set NULLFSINO if the inum is bad.
> > 
> > But in process_dir2_data
> > 
> > [17:25]  <sandeen>                 } else if (verify_inum(mp, ent_ino)) {
> > [17:25]  <sandeen>                         clearreason = _("invalid");
> > [17:26]  <sandeen> so we probably saw that it's bad, but:
> > [17:26]  <sandeen>                  * We have a special dot & dotdot fixer-upper below which can
> > [17:26]  <sandeen>                  * sort out the proper inode number, so don't clear it.
> > [17:26]  <sandeen>                         clearreason = NULL;
> > [17:26]  <sandeen> and then:
> > [17:26]  <sandeen>                  * Special .. entry processing.
> > [17:26]  <sandeen>                                 *parent = ent_ino;
> > [17:26]  <sandeen> \o/
> > 
> > so ... I wonder if it wouldn't be more consistent to find the place under
> > process_inode() where we willingly/wrongly set *parent to an invalid value,
> > and handle the problem there?  Do you know what path you were on to hit this?
> 
> /me doesn't remember, it was either the sf dir scrub or the dir block
> offline repair fuzztest setting the parent pointer to zero.

Aha, it's xfs/386 bu[1].inumber = 0.  I've found the part of
process_dir2_data that inexplicably doesn't check for null parent
pointers & blows up; will rework this patch.

--D

>o
> --D
> 
> > -Eric
> > > ---
> > >  repair/dino_chunks.c |    3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > 
> > > diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
> > > index 17de95f..2d34079 100644
> > > --- a/repair/dino_chunks.c
> > > +++ b/repair/dino_chunks.c
> > > @@ -874,7 +874,8 @@ process_inode_chunk(
> > >  			 * be solid then.
> > >  			 */
> > >  			if (!ino_discovery)  {
> > > -				ASSERT(parent != 0);
> > > +				if (parent == 0)
> > > +					parent = NULLFSINO;
> > >  				set_inode_parent(ino_rec, irec_offset, parent);
> > >  				ASSERT(parent ==
> > >  					get_inode_parent(ino_rec, irec_offset));
> > > 
> > > --
> > > 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
> > > 
> > --
> > 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
> --
> 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