[PATCHSET v4 0/6] various: random fixes

[PATCHSET v4 0/6] 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.

v2: respond to reviewer comments
v3: make repair check dquot types and ids
v4: add a minor code cleanup in xfs_scrub

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  |   10 +++++----
 repair/quotacheck.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++---
 scrub/inodes.c      |   18 +++++++++++++++-
 scrub/phase5.c      |   10 +++++----
 scrub/spacemap.c    |    3 +--
 scrub/unicrash.c    |   33 ++++++++++++++++++++++++++++-
 scrub/unicrash.h    |    4 ++++
 scrub/xfs_scrub.c   |    7 ++++++
 8 files changed, 128 insertions(+), 15 deletions(-)

[PATCH 1/6] 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>
Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 libhandle/handle.c |   10 ++++++----
 scrub/inodes.c     |    2 +-
 scrub/spacemap.c   |    3 +--
 3 files changed, 8 insertions(+), 7 deletions(-)


diff --git a/libhandle/handle.c b/libhandle/handle.c
index 5c1686b3..27abc6b2 100644
--- a/libhandle/handle.c
+++ b/libhandle/handle.c
@@ -235,8 +235,10 @@ obj_to_handle(
 {
 	char		hbuf [MAXHANSIZ];
 	int		ret;
-	uint32_t	handlen;
-	xfs_fsop_handlereq_t hreq;
+	uint32_t	handlen = 0;
+	struct xfs_fsop_handlereq hreq = { };
+
+	memset(hbuf, 0, MAXHANSIZ);
 
 	if (opcode == XFS_IOC_FD_TO_HANDLE) {
 		hreq.fd      = obj.fd;
@@ -275,7 +277,7 @@ open_by_fshandle(
 {
 	int		fsfd;
 	char		*path;
-	xfs_fsop_handlereq_t hreq;
+	struct xfs_fsop_handlereq hreq = { };
 
 	if ((fsfd = handle_to_fsfd(fshanp, &path)) < 0)
 		return -1;
@@ -382,7 +384,7 @@ attr_list_by_handle(
 {
 	int		error, fd;
 	char		*path;
-	xfs_fsop_attrlist_handlereq_t alhreq;
+	struct xfs_fsop_attrlist_handlereq alhreq = { };
 
 	if ((fd = handle_to_fsfd(hanp, &path)) < 0)
 		return -1;
diff --git a/scrub/inodes.c b/scrub/inodes.c
index bdc12df3..63865113 100644
--- a/scrub/inodes.c
+++ b/scrub/inodes.c
@@ -111,7 +111,7 @@ scan_ag_inodes(
 	xfs_agnumber_t		agno,
 	void			*arg)
 {
-	struct xfs_handle	handle;
+	struct xfs_handle	handle = { };
 	char			descr[DESCR_BUFSZ];
 	struct xfs_inumbers_req	*ireq;
 	struct xfs_bulkstat_req	*breq;
diff --git a/scrub/spacemap.c b/scrub/spacemap.c
index 9653916d..a5508d56 100644
--- a/scrub/spacemap.c
+++ b/scrub/spacemap.c
@@ -47,11 +47,10 @@ scrub_iterate_fsmap(
 	int			i;
 	int			error;
 
-	head = malloc(fsmap_sizeof(FSMAP_NR));
+	head = calloc(1, fsmap_sizeof(FSMAP_NR));
 	if (!head)
 		return errno;
 
-	memset(head, 0, sizeof(*head));
 	memcpy(head->fmh_keys, keys, sizeof(struct fsmap) * 2);
 	head->fmh_count = FSMAP_NR;
 

[PATCH 2/6] xfs_scrub: detect infinite loops when scanning inodes

[Darrick J. Wong]

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

During an inode scan (aka phase 3) when we're scanning the inode btree
to find files to check, make sure that each invocation of inumbers
actually gives us an inobt record with a startino that's at least as
large as what we asked for so that we always make forward progress.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 scrub/inodes.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)


diff --git a/scrub/inodes.c b/scrub/inodes.c
index 63865113..cc73da7f 100644
--- a/scrub/inodes.c
+++ b/scrub/inodes.c
@@ -119,6 +119,7 @@ scan_ag_inodes(
 	struct scrub_ctx	*ctx = (struct scrub_ctx *)wq->wq_ctx;
 	struct xfs_bulkstat	*bs;
 	struct xfs_inumbers	*inumbers;
+	uint64_t		nextino = cvt_agino_to_ino(&ctx->mnt, agno, 0);
 	int			i;
 	int			error;
 	int			stale_count = 0;
@@ -153,6 +154,21 @@ scan_ag_inodes(
 	/* Find the inode chunk & alloc mask */
 	error = -xfrog_inumbers(&ctx->mnt, ireq);
 	while (!error && !si->aborted && ireq->hdr.ocount > 0) {
+		/*
+		 * Make sure that we always make forward progress while we
+		 * scan the inode btree.
+		 */
+		if (nextino > inumbers->xi_startino) {
+			str_corrupt(ctx, descr,
+	_("AG %u inode btree is corrupt near agino %lu, got %lu"), agno,
+				cvt_ino_to_agino(&ctx->mnt, nextino),
+				cvt_ino_to_agino(&ctx->mnt,
+						ireq->inumbers[0].xi_startino));
+			si->aborted = true;
+			break;
+		}
+		nextino = ireq->hdr.ino;
+
 		/*
 		 * We can have totally empty inode chunks on filesystems where
 		 * there are more than 64 inodes per block.  Skip these.

[PATCH 3/6] 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>
Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 scrub/unicrash.c  |   17 +++++++++++++++++
 scrub/unicrash.h  |    4 ++++
 scrub/xfs_scrub.c |    7 +++++++
 3 files changed, 28 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..755afaef 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_load()			(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..bc2e84a7 100644
--- a/scrub/xfs_scrub.c
+++ b/scrub/xfs_scrub.c
@@ -603,6 +603,12 @@ main(
 	setlocale(LC_ALL, "");
 	bindtextdomain(PACKAGE, LOCALEDIR);
 	textdomain(PACKAGE);
+	if (unicrash_load()) {
+		fprintf(stderr,
+	_("%s: couldn't initialize Unicode library.\n"),
+				progname);
+		goto out;
+	}
 
 	pthread_mutex_init(&ctx.lock, NULL);
 	ctx.mode = SCRUB_MODE_REPAIR;
@@ -788,6 +794,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 4/6] 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>
Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 scrub/unicrash.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)


diff --git a/scrub/unicrash.c b/scrub/unicrash.c
index de3217c2..cb0880c1 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;
+			return;
+		}
+
 		/* Same normalization? */
 		if (new_entry->normstrlen == entry->normstrlen &&
 		    !u_strcmp(new_entry->normstr, entry->normstr) &&

[PATCH 5/6] xfs_repair: check dquot id and type

[Darrick J. Wong]

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

Make sure that we actually check the type and id of an ondisk dquot.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/quotacheck.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 55 insertions(+), 3 deletions(-)


diff --git a/repair/quotacheck.c b/repair/quotacheck.c
index 55bcc048..0ea2deb5 100644
--- a/repair/quotacheck.c
+++ b/repair/quotacheck.c
@@ -234,12 +234,48 @@ quotacheck_adjust(
 	libxfs_irele(ip);
 }
 
+/* Check the ondisk dquot's id and type match what the incore dquot expects. */
+static bool
+qc_dquot_check_type(
+	struct xfs_mount	*mp,
+	xfs_dqtype_t		type,
+	xfs_dqid_t		id,
+	struct xfs_disk_dquot	*ddq)
+{
+	uint8_t			ddq_type;
+
+	ddq_type = ddq->d_type & XFS_DQTYPE_REC_MASK;
+
+	if (be32_to_cpu(ddq->d_id) != id)
+		return false;
+
+	/*
+	 * V5 filesystems always expect an exact type match.  V4 filesystems
+	 * expect an exact match for user dquots and for non-root group and
+	 * project dquots.
+	 */
+	if (xfs_sb_version_hascrc(&mp->m_sb) || type == XFS_DQTYPE_USER || !id)
+		return ddq_type == type;
+
+	/*
+	 * V4 filesystems support either group or project quotas, but not both
+	 * at the same time.  The non-user quota file can be switched between
+	 * group and project quota uses depending on the mount options, which
+	 * means that we can encounter the other type when we try to load quota
+	 * defaults.  Quotacheck will soon reset the the entire quota file
+	 * (including the root dquot) anyway, but don't log scary corruption
+	 * reports to dmesg.
+	 */
+	return ddq_type == XFS_DQTYPE_GROUP || ddq_type == XFS_DQTYPE_PROJ;
+}
+
 /* Compare this on-disk dquot against whatever we observed. */
 static void
 qc_check_dquot(
 	struct xfs_mount	*mp,
 	struct xfs_disk_dquot	*ddq,
-	struct qc_dquots	*dquots)
+	struct qc_dquots	*dquots,
+	xfs_dqid_t		dqid)
 {
 	struct qc_rec		*qrec;
 	struct qc_rec		empty = {
@@ -253,6 +289,22 @@ qc_check_dquot(
 	if (!qrec)
 		qrec = &empty;
 
+	if (!qc_dquot_check_type(mp, dquots->type, dqid, ddq)) {
+		const char	*dqtypestr;
+
+		dqtypestr = qflags_typestr(ddq->d_type & XFS_DQTYPE_REC_MASK);
+		if (dqtypestr)
+			do_warn(_("%s id %u saw type %s id %u\n"),
+					qflags_typestr(dquots->type), dqid,
+					dqtypestr, be32_to_cpu(ddq->d_id));
+		else
+			do_warn(_("%s id %u saw type %x id %u\n"),
+					qflags_typestr(dquots->type), dqid,
+					ddq->d_type & XFS_DQTYPE_REC_MASK,
+					be32_to_cpu(ddq->d_id));
+		chkd_flags = 0;
+	}
+
 	if (be64_to_cpu(ddq->d_bcount) != qrec->bcount) {
 		do_warn(_("%s id %u has bcount %llu, expected %"PRIu64"\n"),
 				qflags_typestr(dquots->type), id,
@@ -327,11 +379,11 @@ _("cannot read %s inode %"PRIu64", block %"PRIu64", disk block %"PRIu64", err=%d
 		}
 
 		dqb = bp->b_addr;
-		dqid = map->br_startoff * dqperchunk;
+		dqid = (map->br_startoff + bno) * dqperchunk;
 		for (dqnr = 0;
 		     dqnr < dqperchunk && dqid <= UINT_MAX;
 		     dqnr++, dqb++, dqid++)
-			qc_check_dquot(mp, &dqb->dd_diskdq, dquots);
+			qc_check_dquot(mp, &dqb->dd_diskdq, dquots, dqid);
 		libxfs_buf_relse(bp);
 	}
 

[PATCH 6/6] xfs_scrub: fix weirdness in directory name check code

[Darrick J. Wong]

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

Remove the redundant second check of fd and ISDIR in check_inode_names,
and rework the comment to describe why we can't run phase 5 if we found
other corruptions in the filesystem.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 scrub/phase5.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)


diff --git a/scrub/phase5.c b/scrub/phase5.c
index ee1e5d6c..1ef234bf 100644
--- a/scrub/phase5.c
+++ b/scrub/phase5.c
@@ -278,7 +278,12 @@ check_inode_names(
 			goto out;
 	}
 
-	/* Open the dir, let the kernel try to reconnect it to the root. */
+	/*
+	 * Warn about naming problems in the directory entries.  Opening the
+	 * dir by handle means the kernel will try to reconnect it to the root.
+	 * If the reconnection fails due to corruption in the parents we get
+	 * ESTALE, which is why we skip phase 5 if we found corruption.
+	 */
 	if (S_ISDIR(bstat->bs_mode)) {
 		fd = scrub_open_handle(handle);
 		if (fd < 0) {
@@ -288,10 +293,7 @@ check_inode_names(
 			str_errno(ctx, descr_render(&dsc));
 			goto out;
 		}
-	}
 
-	/* Warn about naming problems in the directory entries. */
-	if (fd >= 0 && S_ISDIR(bstat->bs_mode)) {
 		error = check_dirent_names(ctx, &dsc, &fd, bstat);
 		if (error)
 			goto out;

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

[Christoph Hellwig]

We could also use the "= {}" method for hbuf, but otherwise this looks
good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/6] xfs_scrub: detect infinite loops when scanning inodes

[Christoph Hellwig]

On Mon, Feb 08, 2021 at 08:11:27PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> During an inode scan (aka phase 3) when we're scanning the inode btree
> to find files to check, make sure that each invocation of inumbers
> actually gives us an inobt record with a startino that's at least as
> large as what we asked for so that we always make forward progress.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Christoph Hellwig]

On Mon, Feb 08, 2021 at 08:11:33PM -0800, 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>
> Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Christoph Hellwig]

On Mon, Feb 08, 2021 at 08:11:38PM -0800, 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.

So we get the same name but a different ino?  I guess that can happen
with a replacing rename.  Maybe state that more clearly?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 5/6] xfs_repair: check dquot id and type

[Christoph Hellwig]

On Mon, Feb 08, 2021 at 08:11:44PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Make sure that we actually check the type and id of an ondisk dquot.

This looks copied from the kernel code.

But otherwise looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 6/6] xfs_scrub: fix weirdness in directory name check code

[Christoph Hellwig]

On Mon, Feb 08, 2021 at 08:11:49PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Remove the redundant second check of fd and ISDIR in check_inode_names,
> and rework the comment to describe why we can't run phase 5 if we found
> other corruptions in the filesystem.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Darrick J. Wong]

On Tue, Feb 09, 2021 at 09:30:32AM +0000, Christoph Hellwig wrote:
> On Mon, Feb 08, 2021 at 08:11:38PM -0800, 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.
> 
> So we get the same name but a different ino?  I guess that can happen
> with a replacing rename.  Maybe state that more clearly?

Ok, the paragraph now reads:

"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 if
other processes are performing replacing renames on the directory
simultaneously.  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."

--D

> 
> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 5/6] xfs_repair: check dquot id and type

[Darrick J. Wong]

On Tue, Feb 09, 2021 at 09:31:14AM +0000, Christoph Hellwig wrote:
> On Mon, Feb 08, 2021 at 08:11:44PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Make sure that we actually check the type and id of an ondisk dquot.
> 
> This looks copied from the kernel code.

Yeah, it might be time to refactor the quotacheck code into libxfs,
though I suspect the biggest hurdle is that we'd have to port at least
some of xfs_dquot along with it.

--D

> But otherwise looks fine:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 5/6] xfs_repair: check dquot id and type

[Chandan Babu R]

On 09 Feb 2021 at 09:41, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Make sure that we actually check the type and id of an ondisk dquot.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  repair/quotacheck.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 55 insertions(+), 3 deletions(-)
>
>
> diff --git a/repair/quotacheck.c b/repair/quotacheck.c
> index 55bcc048..0ea2deb5 100644
> --- a/repair/quotacheck.c
> +++ b/repair/quotacheck.c
> @@ -234,12 +234,48 @@ quotacheck_adjust(
>  	libxfs_irele(ip);
>  }
>  
> +/* Check the ondisk dquot's id and type match what the incore dquot expects. */
> +static bool
> +qc_dquot_check_type(
> +	struct xfs_mount	*mp,
> +	xfs_dqtype_t		type,
> +	xfs_dqid_t		id,
> +	struct xfs_disk_dquot	*ddq)
> +{
> +	uint8_t			ddq_type;
> +
> +	ddq_type = ddq->d_type & XFS_DQTYPE_REC_MASK;
> +
> +	if (be32_to_cpu(ddq->d_id) != id)
> +		return false;
> +
> +	/*
> +	 * V5 filesystems always expect an exact type match.  V4 filesystems
> +	 * expect an exact match for user dquots and for non-root group and
> +	 * project dquots.
> +	 */
> +	if (xfs_sb_version_hascrc(&mp->m_sb) || type == XFS_DQTYPE_USER || !id)

The above check should have been "id" instead of "!id".

> +		return ddq_type == type;
> +
> +	/*
> +	 * V4 filesystems support either group or project quotas, but not both
> +	 * at the same time.  The non-user quota file can be switched between
> +	 * group and project quota uses depending on the mount options, which
> +	 * means that we can encounter the other type when we try to load quota
> +	 * defaults.  Quotacheck will soon reset the the entire quota file
> +	 * (including the root dquot) anyway, but don't log scary corruption
> +	 * reports to dmesg.
> +	 */
> +	return ddq_type == XFS_DQTYPE_GROUP || ddq_type == XFS_DQTYPE_PROJ;
> +}
> +
>  /* Compare this on-disk dquot against whatever we observed. */
>  static void
>  qc_check_dquot(
>  	struct xfs_mount	*mp,
>  	struct xfs_disk_dquot	*ddq,
> -	struct qc_dquots	*dquots)
> +	struct qc_dquots	*dquots,
> +	xfs_dqid_t		dqid)
>  {
>  	struct qc_rec		*qrec;
>  	struct qc_rec		empty = {
> @@ -253,6 +289,22 @@ qc_check_dquot(
>  	if (!qrec)
>  		qrec = &empty;
>  
> +	if (!qc_dquot_check_type(mp, dquots->type, dqid, ddq)) {
> +		const char	*dqtypestr;
> +
> +		dqtypestr = qflags_typestr(ddq->d_type & XFS_DQTYPE_REC_MASK);
> +		if (dqtypestr)
> +			do_warn(_("%s id %u saw type %s id %u\n"),
> +					qflags_typestr(dquots->type), dqid,
> +					dqtypestr, be32_to_cpu(ddq->d_id));
> +		else
> +			do_warn(_("%s id %u saw type %x id %u\n"),
> +					qflags_typestr(dquots->type), dqid,
> +					ddq->d_type & XFS_DQTYPE_REC_MASK,
> +					be32_to_cpu(ddq->d_id));
> +		chkd_flags = 0;
> +	}
> +
>  	if (be64_to_cpu(ddq->d_bcount) != qrec->bcount) {
>  		do_warn(_("%s id %u has bcount %llu, expected %"PRIu64"\n"),
>  				qflags_typestr(dquots->type), id,
> @@ -327,11 +379,11 @@ _("cannot read %s inode %"PRIu64", block %"PRIu64", disk block %"PRIu64", err=%d
>  		}
>  
>  		dqb = bp->b_addr;
> -		dqid = map->br_startoff * dqperchunk;
> +		dqid = (map->br_startoff + bno) * dqperchunk;
>  		for (dqnr = 0;
>  		     dqnr < dqperchunk && dqid <= UINT_MAX;
>  		     dqnr++, dqb++, dqid++)
> -			qc_check_dquot(mp, &dqb->dd_diskdq, dquots);
> +			qc_check_dquot(mp, &dqb->dd_diskdq, dquots, dqid);
>  		libxfs_buf_relse(bp);
>  	}
>  

-- 
chandan

Re: [PATCH 6/6] xfs_scrub: fix weirdness in directory name check code

[Chandan Babu R]

On 09 Feb 2021 at 09:41, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Remove the redundant second check of fd and ISDIR in check_inode_names,
> and rework the comment to describe why we can't run phase 5 if we found
> other corruptions in the filesystem.
>

Looks good to me.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  scrub/phase5.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
>
> diff --git a/scrub/phase5.c b/scrub/phase5.c
> index ee1e5d6c..1ef234bf 100644
> --- a/scrub/phase5.c
> +++ b/scrub/phase5.c
> @@ -278,7 +278,12 @@ check_inode_names(
>  			goto out;
>  	}
>  
> -	/* Open the dir, let the kernel try to reconnect it to the root. */
> +	/*
> +	 * Warn about naming problems in the directory entries.  Opening the
> +	 * dir by handle means the kernel will try to reconnect it to the root.
> +	 * If the reconnection fails due to corruption in the parents we get
> +	 * ESTALE, which is why we skip phase 5 if we found corruption.
> +	 */
>  	if (S_ISDIR(bstat->bs_mode)) {
>  		fd = scrub_open_handle(handle);
>  		if (fd < 0) {
> @@ -288,10 +293,7 @@ check_inode_names(
>  			str_errno(ctx, descr_render(&dsc));
>  			goto out;
>  		}
> -	}
>  
> -	/* Warn about naming problems in the directory entries. */
> -	if (fd >= 0 && S_ISDIR(bstat->bs_mode)) {
>  		error = check_dirent_names(ctx, &dsc, &fd, bstat);
>  		if (error)
>  			goto out;


-- 
chandan

[PATCH v4.1 5/6] xfs_repair: check dquot id and type

[Darrick J. Wong]

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

Make sure that we actually check the type and id of an ondisk dquot.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
v4.1: fix backwards logic pointed out by Chaitanya
---
 repair/quotacheck.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 55 insertions(+), 3 deletions(-)

diff --git a/repair/quotacheck.c b/repair/quotacheck.c
index 55bcc048..0a505c9c 100644
--- a/repair/quotacheck.c
+++ b/repair/quotacheck.c
@@ -234,12 +234,48 @@ quotacheck_adjust(
 	libxfs_irele(ip);
 }
 
+/* Check the ondisk dquot's id and type match what the incore dquot expects. */
+static bool
+qc_dquot_check_type(
+	struct xfs_mount	*mp,
+	xfs_dqtype_t		type,
+	xfs_dqid_t		id,
+	struct xfs_disk_dquot	*ddq)
+{
+	uint8_t			ddq_type;
+
+	ddq_type = ddq->d_type & XFS_DQTYPE_REC_MASK;
+
+	if (be32_to_cpu(ddq->d_id) != id)
+		return false;
+
+	/*
+	 * V5 filesystems always expect an exact type match.  V4 filesystems
+	 * expect an exact match for user dquots and for non-root group and
+	 * project dquots.
+	 */
+	if (xfs_sb_version_hascrc(&mp->m_sb) || type == XFS_DQTYPE_USER || id)
+		return ddq_type == type;
+
+	/*
+	 * V4 filesystems support either group or project quotas, but not both
+	 * at the same time.  The non-user quota file can be switched between
+	 * group and project quota uses depending on the mount options, which
+	 * means that we can encounter the other type when we try to load quota
+	 * defaults.  Quotacheck will soon reset the the entire quota file
+	 * (including the root dquot) anyway, but don't log scary corruption
+	 * reports to dmesg.
+	 */
+	return ddq_type == XFS_DQTYPE_GROUP || ddq_type == XFS_DQTYPE_PROJ;
+}
+
 /* Compare this on-disk dquot against whatever we observed. */
 static void
 qc_check_dquot(
 	struct xfs_mount	*mp,
 	struct xfs_disk_dquot	*ddq,
-	struct qc_dquots	*dquots)
+	struct qc_dquots	*dquots,
+	xfs_dqid_t		dqid)
 {
 	struct qc_rec		*qrec;
 	struct qc_rec		empty = {
@@ -253,6 +289,22 @@ qc_check_dquot(
 	if (!qrec)
 		qrec = &empty;
 
+	if (!qc_dquot_check_type(mp, dquots->type, dqid, ddq)) {
+		const char	*dqtypestr;
+
+		dqtypestr = qflags_typestr(ddq->d_type & XFS_DQTYPE_REC_MASK);
+		if (dqtypestr)
+			do_warn(_("%s id %u saw type %s id %u\n"),
+					qflags_typestr(dquots->type), dqid,
+					dqtypestr, be32_to_cpu(ddq->d_id));
+		else
+			do_warn(_("%s id %u saw type %x id %u\n"),
+					qflags_typestr(dquots->type), dqid,
+					ddq->d_type & XFS_DQTYPE_REC_MASK,
+					be32_to_cpu(ddq->d_id));
+		chkd_flags = 0;
+	}
+
 	if (be64_to_cpu(ddq->d_bcount) != qrec->bcount) {
 		do_warn(_("%s id %u has bcount %llu, expected %"PRIu64"\n"),
 				qflags_typestr(dquots->type), id,
@@ -327,11 +379,11 @@ _("cannot read %s inode %"PRIu64", block %"PRIu64", disk block %"PRIu64", err=%d
 		}
 
 		dqb = bp->b_addr;
-		dqid = map->br_startoff * dqperchunk;
+		dqid = (map->br_startoff + bno) * dqperchunk;
 		for (dqnr = 0;
 		     dqnr < dqperchunk && dqid <= UINT_MAX;
 		     dqnr++, dqb++, dqid++)
-			qc_check_dquot(mp, &dqb->dd_diskdq, dquots);
+			qc_check_dquot(mp, &dqb->dd_diskdq, dquots, dqid);
 		libxfs_buf_relse(bp);
 	}
 

Re: [PATCH v4.1 5/6] xfs_repair: check dquot id and type

[Chandan Babu R]

On 11 Feb 2021 at 22:36, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Make sure that we actually check the type and id of an ondisk dquot.
>

Looks good to me.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> v4.1: fix backwards logic pointed out by Chaitanya
> ---
>  repair/quotacheck.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/repair/quotacheck.c b/repair/quotacheck.c
> index 55bcc048..0a505c9c 100644
> --- a/repair/quotacheck.c
> +++ b/repair/quotacheck.c
> @@ -234,12 +234,48 @@ quotacheck_adjust(
>  	libxfs_irele(ip);
>  }
>  
> +/* Check the ondisk dquot's id and type match what the incore dquot expects. */
> +static bool
> +qc_dquot_check_type(
> +	struct xfs_mount	*mp,
> +	xfs_dqtype_t		type,
> +	xfs_dqid_t		id,
> +	struct xfs_disk_dquot	*ddq)
> +{
> +	uint8_t			ddq_type;
> +
> +	ddq_type = ddq->d_type & XFS_DQTYPE_REC_MASK;
> +
> +	if (be32_to_cpu(ddq->d_id) != id)
> +		return false;
> +
> +	/*
> +	 * V5 filesystems always expect an exact type match.  V4 filesystems
> +	 * expect an exact match for user dquots and for non-root group and
> +	 * project dquots.
> +	 */
> +	if (xfs_sb_version_hascrc(&mp->m_sb) || type == XFS_DQTYPE_USER || id)
> +		return ddq_type == type;
> +
> +	/*
> +	 * V4 filesystems support either group or project quotas, but not both
> +	 * at the same time.  The non-user quota file can be switched between
> +	 * group and project quota uses depending on the mount options, which
> +	 * means that we can encounter the other type when we try to load quota
> +	 * defaults.  Quotacheck will soon reset the the entire quota file
> +	 * (including the root dquot) anyway, but don't log scary corruption
> +	 * reports to dmesg.
> +	 */
> +	return ddq_type == XFS_DQTYPE_GROUP || ddq_type == XFS_DQTYPE_PROJ;
> +}
> +
>  /* Compare this on-disk dquot against whatever we observed. */
>  static void
>  qc_check_dquot(
>  	struct xfs_mount	*mp,
>  	struct xfs_disk_dquot	*ddq,
> -	struct qc_dquots	*dquots)
> +	struct qc_dquots	*dquots,
> +	xfs_dqid_t		dqid)
>  {
>  	struct qc_rec		*qrec;
>  	struct qc_rec		empty = {
> @@ -253,6 +289,22 @@ qc_check_dquot(
>  	if (!qrec)
>  		qrec = &empty;
>  
> +	if (!qc_dquot_check_type(mp, dquots->type, dqid, ddq)) {
> +		const char	*dqtypestr;
> +
> +		dqtypestr = qflags_typestr(ddq->d_type & XFS_DQTYPE_REC_MASK);
> +		if (dqtypestr)
> +			do_warn(_("%s id %u saw type %s id %u\n"),
> +					qflags_typestr(dquots->type), dqid,
> +					dqtypestr, be32_to_cpu(ddq->d_id));
> +		else
> +			do_warn(_("%s id %u saw type %x id %u\n"),
> +					qflags_typestr(dquots->type), dqid,
> +					ddq->d_type & XFS_DQTYPE_REC_MASK,
> +					be32_to_cpu(ddq->d_id));
> +		chkd_flags = 0;
> +	}
> +
>  	if (be64_to_cpu(ddq->d_bcount) != qrec->bcount) {
>  		do_warn(_("%s id %u has bcount %llu, expected %"PRIu64"\n"),
>  				qflags_typestr(dquots->type), id,
> @@ -327,11 +379,11 @@ _("cannot read %s inode %"PRIu64", block %"PRIu64", disk block %"PRIu64", err=%d
>  		}
>  
>  		dqb = bp->b_addr;
> -		dqid = map->br_startoff * dqperchunk;
> +		dqid = (map->br_startoff + bno) * dqperchunk;
>  		for (dqnr = 0;
>  		     dqnr < dqperchunk && dqid <= UINT_MAX;
>  		     dqnr++, dqb++, dqid++)
> -			qc_check_dquot(mp, &dqb->dd_diskdq, dquots);
> +			qc_check_dquot(mp, &dqb->dd_diskdq, dquots, dqid);
>  		libxfs_buf_relse(bp);
>  	}
>  


-- 
chandan