[PATCHSET 0/3] various: random fixes

[PATCHSET 0/3] various: random fixes

[Darrick J. Wong]

Hi all,

Here are some random fixes to the utilities: the first shuts up valgrind
warnings about uninitialized userspace memory being passed into kernel
ioctls; the second fixes warnings about libicu not being released at the
end of scrub; and the third fixes false collision reports during scrub
phase 5 if someone is replacing directory items while the scanner is
running.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=random-fixes

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes
---
 libhandle/handle.c |    7 ++++++-
 scrub/inodes.c     |    1 +
 scrub/spacemap.c   |    2 +-
 scrub/unicrash.c   |   33 ++++++++++++++++++++++++++++++++-
 scrub/unicrash.h   |    4 ++++
 scrub/xfs_scrub.c  |    6 ++++++
 6 files changed, 50 insertions(+), 3 deletions(-)

[PATCH 1/3] misc: fix valgrind complaints

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Zero the memory that we pass to the kernel via ioctls so that we never
pass userspace heap/stack garbage around.  This silences valgrind
complaints about uninitialized padding areas.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 libhandle/handle.c |    7 ++++++-
 scrub/inodes.c     |    1 +
 scrub/spacemap.c   |    2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)


diff --git a/libhandle/handle.c b/libhandle/handle.c
index 5c1686b3..a6b35b09 100644
--- a/libhandle/handle.c
+++ b/libhandle/handle.c
@@ -235,9 +235,12 @@ obj_to_handle(
 {
 	char		hbuf [MAXHANSIZ];
 	int		ret;
-	uint32_t	handlen;
+	uint32_t	handlen = 0;
 	xfs_fsop_handlereq_t hreq;
 
+	memset(&hreq, 0, sizeof(hreq));
+	memset(hbuf, 0, MAXHANSIZ);
+
 	if (opcode == XFS_IOC_FD_TO_HANDLE) {
 		hreq.fd      = obj.fd;
 		hreq.path    = NULL;
@@ -280,6 +283,7 @@ open_by_fshandle(
 	if ((fsfd = handle_to_fsfd(fshanp, &path)) < 0)
 		return -1;
 
+	memset(&hreq, 0, sizeof(hreq));
 	hreq.fd       = 0;
 	hreq.path     = NULL;
 	hreq.oflags   = rw | O_LARGEFILE;
@@ -387,6 +391,7 @@ attr_list_by_handle(
 	if ((fd = handle_to_fsfd(hanp, &path)) < 0)
 		return -1;
 
+	memset(&alhreq, 0, sizeof(alhreq));
 	alhreq.hreq.fd       = 0;
 	alhreq.hreq.path     = NULL;
 	alhreq.hreq.oflags   = O_LARGEFILE;
diff --git a/scrub/inodes.c b/scrub/inodes.c
index 4550db83..f2bce16f 100644
--- a/scrub/inodes.c
+++ b/scrub/inodes.c
@@ -129,6 +129,7 @@ scan_ag_inodes(
 				minor(ctx->fsinfo.fs_datadev),
 				agno);
 
+	memset(&handle, 0, sizeof(handle));
 	memcpy(&handle.ha_fsid, ctx->fshandle, sizeof(handle.ha_fsid));
 	handle.ha_fid.fid_len = sizeof(xfs_fid_t) -
 			sizeof(handle.ha_fid.fid_len);
diff --git a/scrub/spacemap.c b/scrub/spacemap.c
index 9653916d..9362710e 100644
--- a/scrub/spacemap.c
+++ b/scrub/spacemap.c
@@ -47,7 +47,7 @@ scrub_iterate_fsmap(
 	int			i;
 	int			error;
 
-	head = malloc(fsmap_sizeof(FSMAP_NR));
+	head = calloc(1, fsmap_sizeof(FSMAP_NR));
 	if (!head)
 		return errno;
 

[PATCH 2/3] xfs_scrub: load and unload libicu properly

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Make sure we actually load and unload libicu properly.  This isn't
strictly required since the library can bootstrap itself, but unloading
means fewer things for valgrind to complain about.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 scrub/unicrash.c  |   17 +++++++++++++++++
 scrub/unicrash.h  |    4 ++++
 scrub/xfs_scrub.c |    6 ++++++
 3 files changed, 27 insertions(+)


diff --git a/scrub/unicrash.c b/scrub/unicrash.c
index d5d2cf20..de3217c2 100644
--- a/scrub/unicrash.c
+++ b/scrub/unicrash.c
@@ -722,3 +722,20 @@ unicrash_check_fs_label(
 	return __unicrash_check_name(uc, dsc, _("filesystem label"),
 			label, 0);
 }
+
+/* Load libicu and initialize it. */
+bool
+unicrash_load(void)
+{
+	UErrorCode		uerr = U_ZERO_ERROR;
+
+	u_init(&uerr);
+	return U_FAILURE(uerr);
+}
+
+/* Unload libicu once we're done with it. */
+void
+unicrash_unload(void)
+{
+	u_cleanup();
+}
diff --git a/scrub/unicrash.h b/scrub/unicrash.h
index c3a7f385..32cae3d4 100644
--- a/scrub/unicrash.h
+++ b/scrub/unicrash.h
@@ -25,6 +25,8 @@ int unicrash_check_xattr_name(struct unicrash *uc, struct descr *dsc,
 		const char *attrname);
 int unicrash_check_fs_label(struct unicrash *uc, struct descr *dsc,
 		const char *label);
+bool unicrash_load(void);
+void unicrash_unload(void);
 #else
 # define unicrash_dir_init(u, c, b)		(0)
 # define unicrash_xattr_init(u, c, b)		(0)
@@ -33,6 +35,8 @@ int unicrash_check_fs_label(struct unicrash *uc, struct descr *dsc,
 # define unicrash_check_dir_name(u, d, n)	(0)
 # define unicrash_check_xattr_name(u, d, n)	(0)
 # define unicrash_check_fs_label(u, d, n)	(0)
+# define unicrash_init()			(0)
+# define unicrash_unload()			do { } while (0)
 #endif /* HAVE_LIBICU */
 
 #endif /* XFS_SCRUB_UNICRASH_H_ */
diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
index 1edeb150..6b202912 100644
--- a/scrub/xfs_scrub.c
+++ b/scrub/xfs_scrub.c
@@ -603,6 +603,11 @@ main(
 	setlocale(LC_ALL, "");
 	bindtextdomain(PACKAGE, LOCALEDIR);
 	textdomain(PACKAGE);
+	if (unicrash_load()) {
+		fprintf(stderr,
+			_("%s: could initialize Unicode library.\n"), progname);
+		goto out;
+	}
 
 	pthread_mutex_init(&ctx.lock, NULL);
 	ctx.mode = SCRUB_MODE_REPAIR;
@@ -788,6 +793,7 @@ main(
 	phase_end(&all_pi, 0);
 	if (progress_fp)
 		fclose(progress_fp);
+	unicrash_unload();
 
 	/*
 	 * If we're being run as a service, the return code must fit the LSB

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

[Darrick J. Wong]

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;
+		}
+
 		/* Same normalization? */
 		if (new_entry->normstrlen == entry->normstrlen &&
 		    !u_strcmp(new_entry->normstr, entry->normstr) &&

Re: [PATCH 1/3] misc: fix valgrind complaints

[Chandan Babu R]

On 09 Jan 2021 at 11:58, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Zero the memory that we pass to the kernel via ioctls so that we never
> pass userspace heap/stack garbage around.  This silences valgrind
> complaints about uninitialized padding areas.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  libhandle/handle.c |    7 ++++++-
>  scrub/inodes.c     |    1 +
>  scrub/spacemap.c   |    2 +-
>  3 files changed, 8 insertions(+), 2 deletions(-)
>
>
> diff --git a/libhandle/handle.c b/libhandle/handle.c
> index 5c1686b3..a6b35b09 100644
> --- a/libhandle/handle.c
> +++ b/libhandle/handle.c
> @@ -235,9 +235,12 @@ obj_to_handle(
>  {
>  	char		hbuf [MAXHANSIZ];
>  	int		ret;
> -	uint32_t	handlen;
> +	uint32_t	handlen = 0;
>  	xfs_fsop_handlereq_t hreq;
>
> +	memset(&hreq, 0, sizeof(hreq));
> +	memset(hbuf, 0, MAXHANSIZ);
> +
>  	if (opcode == XFS_IOC_FD_TO_HANDLE) {
>  		hreq.fd      = obj.fd;
>  		hreq.path    = NULL;
> @@ -280,6 +283,7 @@ open_by_fshandle(
>  	if ((fsfd = handle_to_fsfd(fshanp, &path)) < 0)
>  		return -1;
>
> +	memset(&hreq, 0, sizeof(hreq));
>  	hreq.fd       = 0;
>  	hreq.path     = NULL;
>  	hreq.oflags   = rw | O_LARGEFILE;
> @@ -387,6 +391,7 @@ attr_list_by_handle(
>  	if ((fd = handle_to_fsfd(hanp, &path)) < 0)
>  		return -1;
>
> +	memset(&alhreq, 0, sizeof(alhreq));
>  	alhreq.hreq.fd       = 0;
>  	alhreq.hreq.path     = NULL;
>  	alhreq.hreq.oflags   = O_LARGEFILE;
> diff --git a/scrub/inodes.c b/scrub/inodes.c
> index 4550db83..f2bce16f 100644
> --- a/scrub/inodes.c
> +++ b/scrub/inodes.c
> @@ -129,6 +129,7 @@ scan_ag_inodes(
>  				minor(ctx->fsinfo.fs_datadev),
>  				agno);
>
> +	memset(&handle, 0, sizeof(handle));
>  	memcpy(&handle.ha_fsid, ctx->fshandle, sizeof(handle.ha_fsid));
>  	handle.ha_fid.fid_len = sizeof(xfs_fid_t) -
>  			sizeof(handle.ha_fid.fid_len);
> diff --git a/scrub/spacemap.c b/scrub/spacemap.c
> index 9653916d..9362710e 100644
> --- a/scrub/spacemap.c
> +++ b/scrub/spacemap.c
> @@ -47,7 +47,7 @@ scrub_iterate_fsmap(
>  	int			i;
>  	int			error;
>
> -	head = malloc(fsmap_sizeof(FSMAP_NR));
> +	head = calloc(1, fsmap_sizeof(FSMAP_NR));
>  	if (!head)
>  		return errno;
>

Minor nit: The "memset(head, 0, sizeof(*head))" statement following the above
call to calloc() can now be removed.

--
chandan

Re: [PATCH 2/3] xfs_scrub: load and unload libicu properly

[Chandan Babu R]

On 09 Jan 2021 at 11:58, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Make sure we actually load and unload libicu properly.  This isn't
> strictly required since the library can bootstrap itself, but unloading
> means fewer things for valgrind to complain about.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  scrub/unicrash.c  |   17 +++++++++++++++++
>  scrub/unicrash.h  |    4 ++++
>  scrub/xfs_scrub.c |    6 ++++++
>  3 files changed, 27 insertions(+)
>
>
> diff --git a/scrub/unicrash.c b/scrub/unicrash.c
> index d5d2cf20..de3217c2 100644
> --- a/scrub/unicrash.c
> +++ b/scrub/unicrash.c
> @@ -722,3 +722,20 @@ unicrash_check_fs_label(
>  	return __unicrash_check_name(uc, dsc, _("filesystem label"),
>  			label, 0);
>  }
> +
> +/* Load libicu and initialize it. */
> +bool
> +unicrash_load(void)
> +{
> +	UErrorCode		uerr = U_ZERO_ERROR;
> +
> +	u_init(&uerr);
> +	return U_FAILURE(uerr);
> +}
> +
> +/* Unload libicu once we're done with it. */
> +void
> +unicrash_unload(void)
> +{
> +	u_cleanup();
> +}
> diff --git a/scrub/unicrash.h b/scrub/unicrash.h
> index c3a7f385..32cae3d4 100644
> --- a/scrub/unicrash.h
> +++ b/scrub/unicrash.h
> @@ -25,6 +25,8 @@ int unicrash_check_xattr_name(struct unicrash *uc, struct descr *dsc,
>  		const char *attrname);
>  int unicrash_check_fs_label(struct unicrash *uc, struct descr *dsc,
>  		const char *label);
> +bool unicrash_load(void);
> +void unicrash_unload(void);
>  #else
>  # define unicrash_dir_init(u, c, b)		(0)
>  # define unicrash_xattr_init(u, c, b)		(0)
> @@ -33,6 +35,8 @@ int unicrash_check_fs_label(struct unicrash *uc, struct descr *dsc,
>  # define unicrash_check_dir_name(u, d, n)	(0)
>  # define unicrash_check_xattr_name(u, d, n)	(0)
>  # define unicrash_check_fs_label(u, d, n)	(0)
> +# define unicrash_init()			(0)

The above should probably be defining unicrash_load().

> +# define unicrash_unload()			do { } while (0)
>  #endif /* HAVE_LIBICU */
>  
>  #endif /* XFS_SCRUB_UNICRASH_H_ */
> diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
> index 1edeb150..6b202912 100644
> --- a/scrub/xfs_scrub.c
> +++ b/scrub/xfs_scrub.c
> @@ -603,6 +603,11 @@ main(
>  	setlocale(LC_ALL, "");
>  	bindtextdomain(PACKAGE, LOCALEDIR);
>  	textdomain(PACKAGE);
> +	if (unicrash_load()) {
> +		fprintf(stderr,
> +			_("%s: could initialize Unicode library.\n"), progname);
> +		goto out;
> +	}
>  
>  	pthread_mutex_init(&ctx.lock, NULL);
>  	ctx.mode = SCRUB_MODE_REPAIR;
> @@ -788,6 +793,7 @@ main(
>  	phase_end(&all_pi, 0);
>  	if (progress_fp)
>  		fclose(progress_fp);
> +	unicrash_unload();
>  
>  	/*
>  	 * If we're being run as a service, the return code must fit the LSB


-- 
chandan

Re: [PATCH 1/3] misc: fix valgrind complaints

[Christoph Hellwig]

On Fri, Jan 08, 2021 at 10:28:40PM -0800, Darrick J. Wong wrote:
>  	char		hbuf [MAXHANSIZ];
>  	int		ret;
> -	uint32_t	handlen;
> +	uint32_t	handlen = 0;
>  	xfs_fsop_handlereq_t hreq;
>  
> +	memset(&hreq, 0, sizeof(hreq));
> +	memset(hbuf, 0, MAXHANSIZ);

Using empty initializers at declaration time is simpler and sometimes
more efficient.  But either way will work fine.

Re: [PATCH 2/3] xfs_scrub: load and unload libicu properly

[Darrick J. Wong]

On Mon, Jan 11, 2021 at 07:45:15PM +0530, Chandan Babu R wrote:
> 
> On 09 Jan 2021 at 11:58, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Make sure we actually load and unload libicu properly.  This isn't
> > strictly required since the library can bootstrap itself, but unloading
> > means fewer things for valgrind to complain about.
> >
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  scrub/unicrash.c  |   17 +++++++++++++++++
> >  scrub/unicrash.h  |    4 ++++
> >  scrub/xfs_scrub.c |    6 ++++++
> >  3 files changed, 27 insertions(+)
> >
> >
> > diff --git a/scrub/unicrash.c b/scrub/unicrash.c
> > index d5d2cf20..de3217c2 100644
> > --- a/scrub/unicrash.c
> > +++ b/scrub/unicrash.c
> > @@ -722,3 +722,20 @@ unicrash_check_fs_label(
> >  	return __unicrash_check_name(uc, dsc, _("filesystem label"),
> >  			label, 0);
> >  }
> > +
> > +/* Load libicu and initialize it. */
> > +bool
> > +unicrash_load(void)
> > +{
> > +	UErrorCode		uerr = U_ZERO_ERROR;
> > +
> > +	u_init(&uerr);
> > +	return U_FAILURE(uerr);
> > +}
> > +
> > +/* Unload libicu once we're done with it. */
> > +void
> > +unicrash_unload(void)
> > +{
> > +	u_cleanup();
> > +}
> > diff --git a/scrub/unicrash.h b/scrub/unicrash.h
> > index c3a7f385..32cae3d4 100644
> > --- a/scrub/unicrash.h
> > +++ b/scrub/unicrash.h
> > @@ -25,6 +25,8 @@ int unicrash_check_xattr_name(struct unicrash *uc, struct descr *dsc,
> >  		const char *attrname);
> >  int unicrash_check_fs_label(struct unicrash *uc, struct descr *dsc,
> >  		const char *label);
> > +bool unicrash_load(void);
> > +void unicrash_unload(void);
> >  #else
> >  # define unicrash_dir_init(u, c, b)		(0)
> >  # define unicrash_xattr_init(u, c, b)		(0)
> > @@ -33,6 +35,8 @@ int unicrash_check_fs_label(struct unicrash *uc, struct descr *dsc,
> >  # define unicrash_check_dir_name(u, d, n)	(0)
> >  # define unicrash_check_xattr_name(u, d, n)	(0)
> >  # define unicrash_check_fs_label(u, d, n)	(0)
> > +# define unicrash_init()			(0)
> 
> The above should probably be defining unicrash_load().

Yep, thanks for catching that.

--D

> > +# define unicrash_unload()			do { } while (0)
> >  #endif /* HAVE_LIBICU */
> >  
> >  #endif /* XFS_SCRUB_UNICRASH_H_ */
> > diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
> > index 1edeb150..6b202912 100644
> > --- a/scrub/xfs_scrub.c
> > +++ b/scrub/xfs_scrub.c
> > @@ -603,6 +603,11 @@ main(
> >  	setlocale(LC_ALL, "");
> >  	bindtextdomain(PACKAGE, LOCALEDIR);
> >  	textdomain(PACKAGE);
> > +	if (unicrash_load()) {
> > +		fprintf(stderr,
> > +			_("%s: could initialize Unicode library.\n"), progname);
> > +		goto out;
> > +	}
> >  
> >  	pthread_mutex_init(&ctx.lock, NULL);
> >  	ctx.mode = SCRUB_MODE_REPAIR;
> > @@ -788,6 +793,7 @@ main(
> >  	phase_end(&all_pi, 0);
> >  	if (progress_fp)
> >  		fclose(progress_fp);
> > +	unicrash_unload();
> >  
> >  	/*
> >  	 * If we're being run as a service, the return code must fit the LSB
> 
> 
> -- 
> chandan

Re: [PATCH 1/3] misc: fix valgrind complaints

[Darrick J. Wong]

On Mon, Jan 11, 2021 at 07:08:27PM +0530, Chandan Babu R wrote:
> 
> On 09 Jan 2021 at 11:58, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Zero the memory that we pass to the kernel via ioctls so that we never
> > pass userspace heap/stack garbage around.  This silences valgrind
> > complaints about uninitialized padding areas.
> >
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  libhandle/handle.c |    7 ++++++-
> >  scrub/inodes.c     |    1 +
> >  scrub/spacemap.c   |    2 +-
> >  3 files changed, 8 insertions(+), 2 deletions(-)
> >
> >
> > diff --git a/libhandle/handle.c b/libhandle/handle.c
> > index 5c1686b3..a6b35b09 100644
> > --- a/libhandle/handle.c
> > +++ b/libhandle/handle.c
> > @@ -235,9 +235,12 @@ obj_to_handle(
> >  {
> >  	char		hbuf [MAXHANSIZ];
> >  	int		ret;
> > -	uint32_t	handlen;
> > +	uint32_t	handlen = 0;
> >  	xfs_fsop_handlereq_t hreq;
> >
> > +	memset(&hreq, 0, sizeof(hreq));
> > +	memset(hbuf, 0, MAXHANSIZ);
> > +
> >  	if (opcode == XFS_IOC_FD_TO_HANDLE) {
> >  		hreq.fd      = obj.fd;
> >  		hreq.path    = NULL;
> > @@ -280,6 +283,7 @@ open_by_fshandle(
> >  	if ((fsfd = handle_to_fsfd(fshanp, &path)) < 0)
> >  		return -1;
> >
> > +	memset(&hreq, 0, sizeof(hreq));
> >  	hreq.fd       = 0;
> >  	hreq.path     = NULL;
> >  	hreq.oflags   = rw | O_LARGEFILE;
> > @@ -387,6 +391,7 @@ attr_list_by_handle(
> >  	if ((fd = handle_to_fsfd(hanp, &path)) < 0)
> >  		return -1;
> >
> > +	memset(&alhreq, 0, sizeof(alhreq));
> >  	alhreq.hreq.fd       = 0;
> >  	alhreq.hreq.path     = NULL;
> >  	alhreq.hreq.oflags   = O_LARGEFILE;
> > diff --git a/scrub/inodes.c b/scrub/inodes.c
> > index 4550db83..f2bce16f 100644
> > --- a/scrub/inodes.c
> > +++ b/scrub/inodes.c
> > @@ -129,6 +129,7 @@ scan_ag_inodes(
> >  				minor(ctx->fsinfo.fs_datadev),
> >  				agno);
> >
> > +	memset(&handle, 0, sizeof(handle));
> >  	memcpy(&handle.ha_fsid, ctx->fshandle, sizeof(handle.ha_fsid));
> >  	handle.ha_fid.fid_len = sizeof(xfs_fid_t) -
> >  			sizeof(handle.ha_fid.fid_len);
> > diff --git a/scrub/spacemap.c b/scrub/spacemap.c
> > index 9653916d..9362710e 100644
> > --- a/scrub/spacemap.c
> > +++ b/scrub/spacemap.c
> > @@ -47,7 +47,7 @@ scrub_iterate_fsmap(
> >  	int			i;
> >  	int			error;
> >
> > -	head = malloc(fsmap_sizeof(FSMAP_NR));
> > +	head = calloc(1, fsmap_sizeof(FSMAP_NR));
> >  	if (!head)
> >  		return errno;
> >
> 
> Minor nit: The "memset(head, 0, sizeof(*head))" statement following the above
> call to calloc() can now be removed.

FIxed, thanks.

--D

> --
> chandan

Re: [PATCH 1/3] misc: fix valgrind complaints

[Darrick J. Wong]

On Mon, Jan 11, 2021 at 05:27:46PM +0000, Christoph Hellwig wrote:
> On Fri, Jan 08, 2021 at 10:28:40PM -0800, Darrick J. Wong wrote:
> >  	char		hbuf [MAXHANSIZ];
> >  	int		ret;
> > -	uint32_t	handlen;
> > +	uint32_t	handlen = 0;
> >  	xfs_fsop_handlereq_t hreq;
> >  
> > +	memset(&hreq, 0, sizeof(hreq));
> > +	memset(hbuf, 0, MAXHANSIZ);
> 
> Using empty initializers at declaration time is simpler and sometimes
> more efficient.  But either way will work fine.

I'll fix that then, and get rid of two more typedef usages.

--D

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

[Chandan Babu R]

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

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

[Darrick J. Wong]

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