Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 25 May 2019 00:22:07 +0000 (UTC)
From:      Kirk McKusick <mckusick@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org
Subject:   svn commit: r348260 - stable/11/sbin/fsck_ffs
Message-ID:  <201905250022.x4P0M77A027598@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: mckusick
Date: Sat May 25 00:22:07 2019
New Revision: 348260
URL: https://svnweb.freebsd.org/changeset/base/348260

Log:
  MFC of 348074
  
  Rewrite fsck_readdir() and dircheck() for clarity and correctness.
  
  Approved by: re (gjb)

Modified:
  stable/11/sbin/fsck_ffs/dir.c
Directory Properties:
  stable/11/   (props changed)

Modified: stable/11/sbin/fsck_ffs/dir.c
==============================================================================
--- stable/11/sbin/fsck_ffs/dir.c	Sat May 25 00:07:49 2019	(r348259)
+++ stable/11/sbin/fsck_ffs/dir.c	Sat May 25 00:22:07 2019	(r348260)
@@ -59,7 +59,7 @@ static struct	dirtemplate dirhead = {
 };
 
 static int chgino(struct inodesc *);
-static int dircheck(struct inodesc *, struct direct *);
+static int dircheck(struct inodesc *, struct bufarea *, struct direct *);
 static int expanddir(union dinode *dp, char *name);
 static void freedir(ino_t ino, ino_t parent);
 static struct direct *fsck_readdir(struct inodesc *);
@@ -137,78 +137,70 @@ dirscan(struct inodesc *idesc)
 }
 
 /*
- * get next entry in a directory.
+ * Get and verify the next entry in a directory.
+ * We also verify that if there is another entry in the block that it is
+ * valid, so if it is not valid it can be subsumed into the current entry. 
  */
 static struct direct *
 fsck_readdir(struct inodesc *idesc)
 {
 	struct direct *dp, *ndp;
 	struct bufarea *bp;
-	long size, blksiz, fix, dploc;
-	int dc;
+	long size, blksiz, subsume_ndp;
 
+	subsume_ndp = 0;
 	blksiz = idesc->id_numfrags * sblock.fs_fsize;
+	if (idesc->id_filesize <= 0 || idesc->id_loc >= blksiz)
+		return (NULL);
 	bp = getdirblk(idesc->id_blkno, blksiz);
-	if (idesc->id_loc % DIRBLKSIZ == 0 && idesc->id_filesize > 0 &&
-	    idesc->id_loc < blksiz) {
-		dp = (struct direct *)(bp->b_un.b_buf + idesc->id_loc);
-		if ((dc = dircheck(idesc, dp)) > 0) {
-			if (dc == 2) {
-				/*
-				 * dircheck() cleared unused directory space.
-				 * Mark the buffer as dirty to write it out.
-				 */
-				dirty(bp);
-			}
-			goto dpok;
-		}
-		if (idesc->id_fix == IGNORE)
-			return (0);
-		fix = dofix(idesc, "DIRECTORY CORRUPTED");
-		bp = getdirblk(idesc->id_blkno, blksiz);
-		dp = (struct direct *)(bp->b_un.b_buf + idesc->id_loc);
-		dp->d_reclen = DIRBLKSIZ;
-		dp->d_ino = 0;
-		dp->d_type = 0;
-		dp->d_namlen = 0;
-		dp->d_name[0] = '\0';
-		if (fix)
-			dirty(bp);
-		idesc->id_loc += DIRBLKSIZ;
-		idesc->id_filesize -= DIRBLKSIZ;
-		return (dp);
+	dp = (struct direct *)(bp->b_un.b_buf + idesc->id_loc);
+	/*
+	 * Only need to check current entry if it is the first in the
+	 * the block, as later entries will have been checked in the
+	 * previous call to this function.
+	 */
+	if (idesc->id_loc % DIRBLKSIZ != 0 || dircheck(idesc, bp, dp) != 0) {
+		/*
+		 * Current entry is good, update to point at next.
+		 */
+		idesc->id_loc += dp->d_reclen;
+		idesc->id_filesize -= dp->d_reclen;
+		/*
+		 * If at end of directory block, just return this entry.
+		 */
+		if (idesc->id_filesize <= 0 || idesc->id_loc >= blksiz ||
+		    idesc->id_loc % DIRBLKSIZ == 0)
+			return (dp);
+		/*
+		 * If the next entry good, return this entry.
+		 */
+		ndp = (struct direct *)(bp->b_un.b_buf + idesc->id_loc);
+		if (dircheck(idesc, bp, ndp) != 0)
+			return (dp);
+		/*
+		 * The next entry is bad, so subsume it and the remainder
+		 * of this directory block into this entry.
+		 */
+		subsume_ndp = 1;
 	}
-dpok:
-	if (idesc->id_filesize <= 0 || idesc->id_loc >= blksiz)
-		return NULL;
-	dploc = idesc->id_loc;
-	dp = (struct direct *)(bp->b_un.b_buf + dploc);
-	idesc->id_loc += dp->d_reclen;
-	idesc->id_filesize -= dp->d_reclen;
-	if ((idesc->id_loc % DIRBLKSIZ) == 0)
-		return (dp);
-	ndp = (struct direct *)(bp->b_un.b_buf + idesc->id_loc);
-	if (idesc->id_loc < blksiz && idesc->id_filesize > 0) {
-		if ((dc = dircheck(idesc, ndp)) == 0) {
-			size = DIRBLKSIZ - (idesc->id_loc % DIRBLKSIZ);
-			idesc->id_loc += size;
-			idesc->id_filesize -= size;
-			if (idesc->id_fix == IGNORE)
-				return (0);
-			fix = dofix(idesc, "DIRECTORY CORRUPTED");
-			bp = getdirblk(idesc->id_blkno, blksiz);
-			dp = (struct direct *)(bp->b_un.b_buf + dploc);
-			dp->d_reclen += size;
-			if (fix)
-				dirty(bp);
-		} else if (dc == 2) {
-			/*
-			 * dircheck() cleared unused directory space.
-			 * Mark the buffer as dirty to write it out.
-			 */
-			dirty(bp);
-		}
+	/*
+	 * Current or next entry is bad. Zap current entry or
+	 * subsume next entry into current entry as appropriate.
+	 */
+	size = DIRBLKSIZ - (idesc->id_loc % DIRBLKSIZ);
+	idesc->id_loc += size;
+	idesc->id_filesize -= size;
+	if (idesc->id_fix == IGNORE)
+		return (NULL);
+	if (subsume_ndp) {
+		memset(ndp, 0, size);
+		dp->d_reclen += size;
+	} else {
+		memset(dp, 0, size);
+		dp->d_reclen = size;
 	}
+	if (dofix(idesc, "DIRECTORY CORRUPTED"))
+		dirty(bp);
 	return (dp);
 }
 
@@ -217,65 +209,80 @@ dpok:
  * This is a superset of the checks made in the kernel.
  * Also optionally clears padding and unused directory space.
  *
- * Returns 0 if the entry is bad, 1 if the entry is good and no changes
- * were made, and 2 if the entry is good but modified to clear out padding
- * and unused space and needs to be written back to disk.
+ * Returns 0 if the entry is bad, 1 if the entry is good.
  */
 static int
-dircheck(struct inodesc *idesc, struct direct *dp)
+dircheck(struct inodesc *idesc, struct bufarea *bp, struct direct *dp)
 {
 	size_t size;
 	char *cp;
-	u_char type;
 	u_int8_t namlen;
 	int spaceleft, modified, unused;
 
-	modified = 0;
 	spaceleft = DIRBLKSIZ - (idesc->id_loc % DIRBLKSIZ);
+	size = DIRSIZ(0, dp);
 	if (dp->d_reclen == 0 ||
 	    dp->d_reclen > spaceleft ||
+	    dp->d_reclen < size ||
+	    idesc->id_filesize < size ||
 	    (dp->d_reclen & (DIR_ROUNDUP - 1)) != 0)
 		goto bad;
+	modified = 0;
 	if (dp->d_ino == 0) {
+		if (!zflag || fswritefd < 0)
+			return (1);
 		/*
-		 * Special case of an unused directory entry. Normally
-		 * the kernel would coalesce unused space with the previous
-		 * entry by extending its d_reclen, but there are situations
-		 * (e.g. fsck) where that doesn't occur.
-		 * If we're clearing out directory cruft (-z flag), then make
-		 * sure this entry gets fully cleared as well.
+		 * Special case of an unused directory entry. Normally only
+		 * occurs at the beginning of a directory block when the block
+		 * contains no entries. Other than the first entry in a
+		 * directory block, the kernel coalesces unused space with
+		 * the previous entry by extending its d_reclen. However,
+		 * when cleaning up a directory, fsck may set d_ino to zero
+		 * in the middle of a directory block. If we're clearing out
+		 * directory cruft (-z flag), then make sure that all directory
+		 * space in entries with d_ino == 0 gets fully cleared.
 		 */
-		if (zflag && fswritefd >= 0) {
-			if (dp->d_type != 0) {
-				dp->d_type = 0;
+		if (dp->d_type != 0) {
+			dp->d_type = 0;
+			modified = 1;
+		}
+		if (dp->d_namlen != 0) {
+			dp->d_namlen = 0;
+			modified = 1;
+		}
+		unused = dp->d_reclen - __offsetof(struct direct, d_name);
+		for (cp = dp->d_name; unused > 0; unused--, cp++) {
+			if (*cp != '\0') {
+				*cp = '\0';
 				modified = 1;
 			}
-			if (dp->d_namlen != 0) {
-				dp->d_namlen = 0;
-				modified = 1;
-			}
-			if (dp->d_name[0] != '\0') {
-				dp->d_name[0] = '\0';
-				modified = 1;
-			}
 		}
-		goto good;
+		if (modified)
+			dirty(bp);
+		return (1);
 	}
-	size = DIRSIZ(0, dp);
+	/*
+	 * The d_type field should not be tested here. A bad type is an error
+	 * in the entry itself but is not a corruption of the directory
+	 * structure itself. So blowing away all the remaining entries in the
+	 * directory block is inappropriate. Rather the type error should be
+	 * checked in pass1 and fixed there.
+	 *
+	 * The name validation should also be done in pass1 although the
+	 * check to see if the name is longer than fits in the space
+	 * allocated for it (i.e., the *cp != '\0' fails after exiting the
+	 * loop below) then it really is a structural error that requires
+	 * the stronger action taken here.
+	 */
 	namlen = dp->d_namlen;
-	type = dp->d_type;
-	if (dp->d_reclen < size ||
-	    idesc->id_filesize < size ||
-	    namlen == 0 ||
-	    type > 15)
+	if (namlen == 0 || dp->d_type > 15)
 		goto bad;
-	for (cp = dp->d_name, size = 0; size < namlen; size++)
-		if (*cp == '\0' || (*cp++ == '/'))
+	for (cp = dp->d_name, size = 0; size < namlen; size++) {
+		if (*cp == '\0' || *cp++ == '/')
 			goto bad;
+	}
 	if (*cp != '\0')
 		goto bad;
-
-good:
 	if (zflag && fswritefd >= 0) {
 		/*
 		 * Clear unused directory entry space, including the d_name
@@ -298,11 +305,9 @@ good:
 			}
 		}
 		
-		if (modified) {
-			return 2;
-		}
+		if (modified)
+			dirty(bp);
 	}
-
 	return (1);
 
 bad:



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201905250022.x4P0M77A027598>