From owner-svn-src-stable@freebsd.org Sat May 25 00:22:08 2019 Return-Path: Delivered-To: svn-src-stable@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 8FA5215B8C5D; Sat, 25 May 2019 00:22:08 +0000 (UTC) (envelope-from mckusick@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 2AF7586080; Sat, 25 May 2019 00:22:08 +0000 (UTC) (envelope-from mckusick@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 04489246E1; Sat, 25 May 2019 00:22:08 +0000 (UTC) (envelope-from mckusick@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id x4P0M7i2027599; Sat, 25 May 2019 00:22:07 GMT (envelope-from mckusick@FreeBSD.org) Received: (from mckusick@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id x4P0M77A027598; Sat, 25 May 2019 00:22:07 GMT (envelope-from mckusick@FreeBSD.org) Message-Id: <201905250022.x4P0M77A027598@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: mckusick set sender to mckusick@FreeBSD.org using -f From: Kirk McKusick Date: Sat, 25 May 2019 00:22:07 +0000 (UTC) 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 X-SVN-Group: stable-11 X-SVN-Commit-Author: mckusick X-SVN-Commit-Paths: stable/11/sbin/fsck_ffs X-SVN-Commit-Revision: 348260 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 2AF7586080 X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-2.96 / 15.00]; local_wl_from(0.00)[FreeBSD.org]; NEURAL_HAM_MEDIUM(-1.00)[-0.998,0]; NEURAL_HAM_SHORT(-0.97)[-0.966,0]; ASN(0.00)[asn:11403, ipnet:2610:1c1:1::/48, country:US]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-stable@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for all the -stable branches of the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 25 May 2019 00:22:08 -0000 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: