xfs
[Top] [All Lists]

Re: [PATCH V3] xfs_repair: add support for validating dirent ftype field

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH V3] xfs_repair: add support for validating dirent ftype field
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Sat, 25 Jan 2014 09:30:53 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <52E26DD2.5020808@xxxxxxxxxx>
References: <1390517573-29898-1-git-send-email-david@xxxxxxxxxxxxx> <52E26DD2.5020808@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Fri, Jan 24, 2014 at 08:42:42AM -0500, Brian Foster wrote:
> On 01/23/2014 05:52 PM, Dave Chinner wrote:
> > @@ -2189,6 +2238,59 @@ out_fix:
> >   * shortform directory v2 processing routines -- entry verification and
> >   * bad entry deletion (pruning).
> >   */
> > +static struct xfs_dir2_sf_entry *
> > +shortform_dir2_junk(
> > +   struct xfs_mount        *mp,
> > +   struct xfs_dir2_sf_hdr  *sfp,
> > +   struct xfs_dir2_sf_entry *sfep,
> > +   xfs_ino_t               lino,
> > +   int                     *max_size,
> > +   int                     *index,
> > +   int                     *bytes_deleted,
> > +   int                     *ino_dirty)
> > +{
> > +   struct xfs_dir2_sf_entry *tmp_sfep;
> > +   int                     tmp_len;
> > +   int                     tmp_elen;
> > +
> > +   if (lino == orphanage_ino)
> > +           orphanage_ino = 0;
> > +   if (no_modify) {
> > +           do_warn(_("would junk entry\n"));
> > +           return NULL;
> 
> Argh, sorry I missed this last time. ;) This looks like a problem. In
> the no_modify case, we return NULL and 'continue' in the caller, which
> skips the loop end logic and leads to bad things.
> 
> We could calculate and return the next entry here, but it might be
> cleaner to use a goto instead of continue in the callers and not
> duplicate the logic.

Argh, my bad. I missed that too. As it is, we already calculate the
next entry in this function - tmp_sfep

> 
> > +   }
> > +
> > +   tmp_elen = xfs_dir3_sf_entsize(mp, sfp,
> > +                                   sfep->namelen);
> > +   tmp_sfep = (xfs_dir2_sf_entry_t *)
> > +           ((__psint_t) sfep + tmp_elen);
> > +   tmp_len = *max_size - ((__psint_t) tmp_sfep
> > +                           - (__psint_t) sfp);
> > +   *max_size -= tmp_elen;
> > +   *bytes_deleted += tmp_elen;
> > +
> > +   memmove(sfep, tmp_sfep, tmp_len);

What this code is doing is moving all the subsequent entries after
the one being junked down over the entry we are junking...

> > +
> > +   sfp->count -= 1;
> > +   memset((void *)((__psint_t)sfep + tmp_len), 0,
> > +                   tmp_elen);

And then zeroing the remaining bytes of the region that is no
longer used.

So the no modify check needs to happen after calculating tmp_sfep
and return that...

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>