xfs: bulkstat error handling is broken

The error propagation is a horror - xfs_bulkstat() returns a rval variable which is only set if there are formatter errors. Any sort of btree walk error or corruption will cause the bulkstat walk to terminate but will not pass an error back to userspace. Worse is the fact that formatter errors will also be ignored if any inodes were correctly formatted into the user buffer. Hence bulkstat can fail badly yet still report success to userspace. This causes significant issues with xfsdump not dumping everything in the filesystem yet reporting success. It's not until a restore fails that there is any indication that the dump was bad and tha bulkstat failed. This patch now triggers xfsdump to fail with bulkstat errors rather than silently missing files in the dump. This now causes bulkstat to fail when the lastino cookie does not fall inside an existing inode chunk. The pre-3.17 code tolerated that error by allowing the code to move to the next inode chunk as the agino target is guaranteed to fall into the next btree record. With the fixes up to this point in the series, xfsdump now passes on the troublesome filesystem image that exposes all these bugs. cc: <stable@vger.kernel.org> Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Brian Foster <bfoster@redhat.com>

xfs: bulkstat error handling is broken
The error propagation is a horror - xfs_bulkstat() returns a rval variable which is only set if there are formatter errors. Any sort of btree walk error or corruption will cause the bulkstat walk to terminate but will not pass an error back to userspace. Worse is the fact that formatter errors will also be ignored if any inodes were correctly formatted into the user buffer. Hence bulkstat can fail badly yet still report success to userspace. This causes significant issues with xfsdump not dumping everything in the filesystem yet reporting success. It's not until a restore fails that there is any indication that the dump was bad and tha bulkstat failed. This patch now triggers xfsdump to fail with bulkstat errors rather than silently missing files in the dump. This now causes bulkstat to fail when the lastino cookie does not fall inside an existing inode chunk. The pre-3.17 code tolerated that error by allowing the code to move to the next inode chunk as the agino target is guaranteed to fall into the next btree record. With the fixes up to this point in the series, xfsdump now passes on the troublesome filesystem image that exposes all these bugs. cc: <stable@vger.kernel.org> Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Brian Foster <bfoster@redhat.com>
Dave Chinner · Dave Chinner
1 parent 6e57c542cb
Showing 1 changed file with 19 additions and 10 deletions Side-by-side Diff
fs/xfs/xfs_itable.c
@@ -236,8 +236,10 @@
 	XFS_WANT_CORRUPTED_RETURN(stat == 1);
  
 	/* Check if the record contains the inode in request */
-	if (irec->ir_startino + XFS_INODES_PER_CHUNK <= agino)
-		return -EINVAL;
+	if (irec->ir_startino + XFS_INODES_PER_CHUNK <= agino) {
+		*icount = 0;
+		return 0;
+	}
  
 	idx = agino - irec->ir_startino + 1;
 	if (idx < XFS_INODES_PER_CHUNK &&
@@ -352,7 +354,6 @@
 	xfs_inobt_rec_incore_t	*irbuf;	/* start of irec buffer */
 	xfs_ino_t		lastino; /* last inode number returned */
 	int			nirbuf;	/* size of irbuf */
-	int			rval;	/* return value error code */
 	int			ubcount; /* size of user's buffer */
 	struct xfs_bulkstat_agichunk ac;
 	int			error = 0;
@@ -388,7 +389,6 @@
 	 * Loop over the allocation groups, starting from the last
 	 * inode returned; 0 means start of the allocation group.
 	 */
-	rval = 0;
 	while (agno < mp->m_sb.sb_agcount) {
 		struct xfs_inobt_rec_incore	*irbp = irbuf;
 		struct xfs_inobt_rec_incore	*irbufend = irbuf + nirbuf;
  
@@ -491,13 +491,16 @@
 					formatter, statstruct_size, &ac,
 					&lastino);
 			if (error)
-				rval = error;
+				break;
  
 			cond_resched();
 		}
  
-		/* If we've run out of space, we are done */
-		if (ac.ac_ubleft < statstruct_size)
+		/*
+		 * If we've run out of space or had a formatting error, we
+		 * are now done
+		 */
+		if (ac.ac_ubleft < statstruct_size || error)
 			break;
  
 		if (end_of_ag) {
  
  
@@ -511,11 +514,17 @@
 	 */
 	kmem_free(irbuf);
 	*ubcountp = ac.ac_ubelem;
+
 	/*
-	 * Found some inodes, return them now and return the error next time.
+	 * We found some inodes, so clear the error status and return them.
+	 * The lastino pointer will point directly at the inode that triggered
+	 * any error that occurred, so on the next call the error will be
+	 * triggered again and propagated to userspace as there will be no
+	 * formatted inodes in the buffer.
 	 */
 	if (ac.ac_ubelem)
-		rval = 0;
+		error = 0;
+
 	if (agno >= mp->m_sb.sb_agcount) {
 		/*
 		 * If we ran out of filesystem, mark lastino as off
@@ -527,7 +536,7 @@
 	} else
 		*lastinop = (xfs_ino_t)lastino;
  
-	return rval;
+	return error;
 }
  
 int
...	...	@@ -236,8 +236,10 @@
236	236	XFS_WANT_CORRUPTED_RETURN(stat == 1);
237	237
238	238	/* Check if the record contains the inode in request */
239		- if (irec->ir_startino + XFS_INODES_PER_CHUNK <= agino)
240		- return -EINVAL;
	239	+ if (irec->ir_startino + XFS_INODES_PER_CHUNK <= agino) {
	240	+ *icount = 0;
	241	+ return 0;
	242	+ }
241	243
242	244	idx = agino - irec->ir_startino + 1;
243	245	if (idx < XFS_INODES_PER_CHUNK &&
...	...	@@ -352,7 +354,6 @@
352	354	xfs_inobt_rec_incore_t irbuf; / start of irec buffer */
353	355	xfs_ino_t lastino; /* last inode number returned */
354	356	int nirbuf; /* size of irbuf */
355		- int rval; /* return value error code */
356	357	int ubcount; /* size of user's buffer */
357	358	struct xfs_bulkstat_agichunk ac;
358	359	int error = 0;
...	...	@@ -388,7 +389,6 @@
388	389	* Loop over the allocation groups, starting from the last
389	390	* inode returned; 0 means start of the allocation group.
390	391	*/
391		- rval = 0;
392	392	while (agno < mp->m_sb.sb_agcount) {
393	393	struct xfs_inobt_rec_incore *irbp = irbuf;
394	394	struct xfs_inobt_rec_incore *irbufend = irbuf + nirbuf;
395	395
...	...	@@ -491,13 +491,16 @@
491	491	formatter, statstruct_size, &ac,
492	492	&lastino);
493	493	if (error)
494		- rval = error;
	494	+ break;
495	495
496	496	cond_resched();
497	497	}
498	498
499		- /* If we've run out of space, we are done */
500		- if (ac.ac_ubleft < statstruct_size)
	499	+ /*
	500	+ * If we've run out of space or had a formatting error, we
	501	+ * are now done
	502	+ */
	503	+ if (ac.ac_ubleft < statstruct_size \|\| error)
501	504	break;
502	505
503	506	if (end_of_ag) {
504	507
505	508
...	...	@@ -511,11 +514,17 @@
511	514	*/
512	515	kmem_free(irbuf);
513	516	*ubcountp = ac.ac_ubelem;
	517	+
514	518	/*
515		- * Found some inodes, return them now and return the error next time.
	519	+ * We found some inodes, so clear the error status and return them.
	520	+ * The lastino pointer will point directly at the inode that triggered
	521	+ * any error that occurred, so on the next call the error will be
	522	+ * triggered again and propagated to userspace as there will be no
	523	+ * formatted inodes in the buffer.
516	524	*/
517	525	if (ac.ac_ubelem)
518		- rval = 0;
	526	+ error = 0;
	527	+
519	528	if (agno >= mp->m_sb.sb_agcount) {
520	529	/*
521	530	* If we ran out of filesystem, mark lastino as off
...	...	@@ -527,7 +536,7 @@
527	536	} else
528	537	*lastinop = (xfs_ino_t)lastino;
529	538
530		- return rval;
	539	+ return error;
531	540	}
532	541
533	542	int