Re: [PATCH 3/3] xfs_scrub: handle concurrent directory updates during name scan

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Jan 12, 2021 at 04:45:35PM +0530, Chandan Babu R wrote:
> 
> On 09 Jan 2021 at 11:58, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > The name scanner in xfs_scrub cannot lock a namespace (dirent or xattr)
> > and the kernel does not provide a stable cursor interface, which means
> > that we can see the same byte sequence multiple times during a scan.
> > This isn't a confusing name error since the kernel enforces uniqueness
> > on the byte sequence, so all we need to do here is update the old entry.
> >
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  scrub/unicrash.c |   16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> >
> > diff --git a/scrub/unicrash.c b/scrub/unicrash.c
> > index de3217c2..f5407b5e 100644
> > --- a/scrub/unicrash.c
> > +++ b/scrub/unicrash.c
> > @@ -68,7 +68,7 @@ struct name_entry {
> >
> >  	xfs_ino_t		ino;
> >
> > -	/* Raw UTF8 name */
> > +	/* Raw dirent name */
> >  	size_t			namelen;
> >  	char			name[0];
> >  };
> > @@ -627,6 +627,20 @@ unicrash_add(
> >  	uc->buckets[bucket] = new_entry;
> >
> >  	while (entry != NULL) {
> > +		/*
> > +		 * If we see the same byte sequence then someone's modifying
> > +		 * the namespace while we're scanning it.  Update the existing
> > +		 * entry's inode mapping and erase the new entry from existence.
> > +		 */
> > +		if (new_entry->namelen == entry->namelen &&
> > +		    !memcmp(new_entry->name, entry->name, entry->namelen)) {
> > +			entry->ino = new_entry->ino;
> > +			uc->buckets[bucket] = new_entry->next;
> > +			name_entry_free(new_entry);
> > +			*badflags = 0;
> > +			continue;
> 
> If the above condition evaluates to true, the memory pointed to by "new_entry"
> is freed. The "continue" statement would cause the while loop to be executed
> once more. At this stage, "entry" will still have the previously held non-NULL
> value and hence the while loop is executed once more causing the invalid
> address in "new_entry" to be dereferenced.

Oops, good catch!  Will fix.

--D

> > +		}
> > +
> >  		/* Same normalization? */
> >  		if (new_entry->normstrlen == entry->normstrlen &&
> >  		    !u_strcmp(new_entry->normstr, entry->normstr) &&
> 
> 
> --
> chandan