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

[Chandan Babu R <chandanrlinux@gmail.com>]

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.

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


--
chandan